Re: [PATCH 9/9] drm/sched: Untangle the scheduler list mess

[sashiko-bot@kernel.org]

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 <tvrtko.ursulin@igalia.com>
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/20261002154713.77591-1-tvrtko.ursulin@igalia.com?part=9