Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] drm/sched: memory corruption and leak due to union aliasing in drm_sched_entity_modify_sched - [High] drm/amdgpu: sleeping in atomic context during priority override - [High] drm/sched: NULL pointer dereference in drm_sched_entity_init due to missing checks - [Medium] drm/amdgpu: incomplete state rollback on priority update failure --
commit ab2f90841362b0f1b7727691f75f0960403af244 Author: Tvrtko Ursulin <[email protected]> Date: Fri Oct 2 15:47:13 2026 +0000 drm/sched: Untangle the scheduler list mess This commit documents how the scheduler behaves and makes the entity own its own copy of the scheduler list to untangle the API. Some API calls are updated to return success/failure, which percolates mostly through amdgpu priority overrides. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > index acafa8c59cc90..6500e7f69f5e0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c [ ... ] > @@ -811,16 +811,18 @@ static void amdgpu_ctx_set_entity_priority(struct > amdgpu_ctx *ctx, > hw_prio = array_index_nospec(hw_prio, AMDGPU_RING_PRIO_MAX); > scheds = adev->gpu_sched[hw_ip][hw_prio].sched; > num_scheds = adev->gpu_sched[hw_ip][hw_prio].num_scheds; > - drm_sched_entity_modify_sched(&aentity->entity, scheds, > - num_scheds); > + ret = drm_sched_entity_modify_sched(&aentity->entity, scheds, > + num_scheds); > } > + > + return ret; > } > > -void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, > - int32_t priority) > +int amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int32_t priority) > { > int32_t ctx_prio; > unsigned i, j; > + int ret; > > ctx->override_priority = priority; > > @@ -831,10 +833,15 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx > *ctx, > if (!ctx->entities[i][j]) > continue; > > - amdgpu_ctx_set_entity_priority(ctx, ctx->entities[i][j], > - i, ctx_prio); > + ret = amdgpu_ctx_set_entity_priority(ctx, > + > ctx->entities[i][j], > + i, ctx_prio); > + if (ret) > + return ret; > } > } > + > + return 0; > } [Severity: Medium] If drm_sched_entity_modify_sched() fails (e.g. from an -ENOMEM during its allocation), isn't the software priority already modified unconditionally by the earlier call to drm_sched_entity_set_priority()? Additionally, when an error is returned to amdgpu_ctx_priority_override(), the loop exits early, returning the error. Does this leave the context partially updated without rolling back the priorities of the previous entities? > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c > index 8effb1158430d..0efe90767bfba 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c > @@ -51,11 +51,14 @@ static int amdgpu_sched_process_priority_override(struct > amdgpu_device *adev, > > mgr = &fpriv->ctx_mgr; > xa_lock(&mgr->ctx_handles); > - xa_for_each(&mgr->ctx_handles, id, ctx) > - amdgpu_ctx_priority_override(ctx, priority); > + xa_for_each(&mgr->ctx_handles, id, ctx) { > + r = amdgpu_ctx_priority_override(ctx, priority); > + if (r) > + break; > + } > xa_unlock(&mgr->ctx_handles); > > - return 0; > + return r; > } [Severity: High] Does this sequence lead to sleeping in atomic context? amdgpu_sched_process_priority_override() acquires the mgr->ctx_handles spinlock via xa_lock(), entering an atomic context, and calls: amdgpu_ctx_priority_override() -> amdgpu_ctx_set_entity_priority() -> drm_sched_entity_modify_sched() If the engine has multiple schedulers, drm_sched_entity_modify_sched() dynamically allocates memory using GFP_KERNEL, which implies __GFP_DIRECT_RECLAIM and can sleep. Can this trigger a Scheduling while atomic BUG? > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > b/drivers/gpu/drm/scheduler/sched_entity.c > index 79ea20838328d..a2e043df10619 100644 > --- a/drivers/gpu/drm/scheduler/sched_entity.c > +++ b/drivers/gpu/drm/scheduler/sched_entity.c [ ... ] > @@ -119,16 +124,11 @@ int drm_sched_entity_init(struct drm_sched_entity > *entity, > unsigned int num_sched_list, > atomic_t *guilty) > { > - unsigned int i; > + int ret; > > - if (!entity || !sched_list || !num_sched_list) > + if (!entity) > return -EINVAL; > > - for (i = 0; i < num_sched_list; i++) { > - if (!sched_list[i] || !sched_list[i]->sched_rq) > - return -EINVAL; > - } > - > memset(entity, 0, sizeof(struct drm_sched_entity)); > > entity->stats = drm_sched_entity_stats_new(); > @@ -138,8 +138,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, > INIT_LIST_HEAD(&entity->list); > entity->guilty = guilty; > entity->last_user = current->group_leader; > - entity->num_sched_list = num_sched_list; > - entity->sched_list = num_sched_list > 1 ? sched_list : NULL; > RB_CLEAR_NODE(&entity->rb_tree_node); > > if (priority >= sched_list[0]->num_user_rqs) { [Severity: High] The fail-safe checks for sched_list and num_sched_list were moved down to drm_sched_entity_modify_sched(), but sched_list[0] is still dereferenced here before that new validation takes place. If a driver initializes an entity with an empty scheduler list, won't this immediately result in a NULL pointer or out-of-bounds dereference instead of a graceful -EINVAL? > @@ -178,21 +183,53 @@ EXPORT_SYMBOL(drm_sched_entity_init); [ ... ] > +int drm_sched_entity_modify_sched(struct drm_sched_entity *entity, > + struct drm_gpu_scheduler **sched_list, > + unsigned int num_sched_list) > { > - WARN_ON(!num_sched_list || !sched_list); > + struct drm_gpu_scheduler **newscheds, **oldscheds; > + unsigned int i; > + > + if (!sched_list || !num_sched_list) > + return -EINVAL; > + > + for (i = 0; i < num_sched_list; i++) { > + if (!sched_list[i] || !sched_list[i]->sched_rq) > + return -EINVAL; > + } > + > + if (num_sched_list > 1) { > + newscheds = kmemdup_array(sched_list, num_sched_list, > + sizeof(*sched_list), GFP_KERNEL); > + if (!newscheds) > + return -ENOMEM; > + } > > spin_lock(&entity->lock); > - entity->sched_list = sched_list; > + if (num_sched_list == 1) { > + entity->sched = sched_list[0]; > + oldscheds = NULL; > + } else { > + oldscheds = entity->sched_list; > + entity->sched_list = newscheds; > + } > entity->num_sched_list = num_sched_list; > spin_unlock(&entity->lock); > + > + kfree(oldscheds); > + > + return 0; > } [Severity: Critical] The struct drm_sched_entity uses a union for sched_list and sched. Does this logic incorrectly use the new num_sched_list count to determine how to extract the old pointer? When transitioning from 1 scheduler to >1, the else branch executes, and oldscheds is assigned entity->sched_list. Since they share a union, doesn't this alias with the old entity->sched pointer, causing kfree(oldscheds) to incorrectly free a driver-owned scheduler object instead of an array? Conversely, when going from >1 to 1, the if branch sets oldscheds to NULL, which seems to leak the previously allocated entity->sched_list array. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
