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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/sched: NULL entity->rq dereference via drm_sched_entity_select_rq()
- [High] drm/sched: invalid flex-array embedding and layout in drm_sched_entity
- [Medium] drm/sched: drm_sched_entity_set_priority() leaves rq_priority stale
--

commit ea4d5caf1dfadd7f3c593ae875d3ce707b5c11f4
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/sched: Untangle the scheduler list mess

This commit reworks drm_sched_entity so that it owns a copy of its
scheduler list via a flexible array member (with a built-in single
scheduler slot), and updates rocket and amdgpu to provide trailing storage
for multi-scheduler entities.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 4bc4f9c8ee403..e29ec5755280d 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -511,18 +511,21 @@ void rocket_job_fini(struct rocket_core *core)
>  int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  {
>  	struct rocket_device *rdev = rocket_priv->rdev;
> -	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
> -							 rdev->num_cores);
> +	unsigned int num_cores = rdev->num_cores;
>  	unsigned int core;
>  	int ret;
>  
> -	for (core = 0; core < rdev->num_cores; core++)
> -		scheds[core] = &rdev->cores[core].sched;
> +	if (WARN_ON_ONCE(num_cores > ARRAY_SIZE(rocket_priv->sched_list)))
> +		return -EINVAL;
> +
> +	for (core = 0; core < num_cores; core++)
> +		rocket_priv->sched_list[core] = &rdev->cores[core].sched;
>  
> -	ret = drm_sched_entity_init(&rocket_priv->sched_entity,
> -				    DRM_SCHED_PRIORITY_NORMAL,
> -				    scheds,
> -				    rdev->num_cores, NULL);
> +	ret = __drm_sched_entity_init(&rocket_priv->sched_entity,
> +				      DRM_SCHED_PRIORITY_NORMAL,
> +				      rocket_priv->sched_list,
> +				      ARRAY_SIZE(rocket_priv->sched_list),
> +				      num_cores, NULL);

[Severity: High]
Because struct drm_sched_entity already includes one pointer slot in its
union (__sched__ and sched_list[]), rocket_priv->sched_list[0] is located
at &rocket_priv->sched_entity.sched_list[1] rather than at index 0.

When rocket_job_open() populates rocket_priv->sched_list[] and passes it
to __drm_sched_entity_init() -> drm_sched_entity_modify_sched(), memmove()
copies num_cores pointers from &sched_entity.sched_list[1] down into
&sched_entity.sched_list[0], shifting the entries in rocket_priv->sched_list
and leaving the two arrays out of sync.

More broadly, adding DECLARE_FLEX_ARRAY(struct drm_gpu_scheduler *,
sched_list) to struct drm_sched_entity makes it a structure with a
flexible array member, which C99/C11 and GCC 14+
(-Wflex-array-member-not-at-end) disallow from being embedded in the
middle of structs or in arrays. However, struct drm_sched_entity is
embedded in DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE itself, in the middle of
struct amdgpu_ctx_entity (before fences[]), struct
amdgpu_ttm_buffer_entity, and struct amdgpu_vm, as well as in arrays across
several DRM drivers (such as amdgpu_mman::move_entities[],
etnaviv_file_private::sched_entity[], panfrost_jm_ctx::slot_entity[], and
v3d_file_priv::sched_entity[]).

Additionally, when drm_sched_entity_modify_sched() writes more than one
element to entity->sched_list, the write crosses the sub-object boundary
of struct drm_sched_entity into the adjacent listname array, which can
trip FORTIFY_SOURCE or UBSAN bounds checks.

Could DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE use TRAILING_OVERLAP() (or a
different storage representation that avoids a flexible array member in
struct drm_sched_entity) so that struct drm_sched_entity can still be
safely embedded in structs and arrays, and listname[0] aliases
sched_list[0]?

[ ... ]

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 79ea20838328d..d4956c6de6ed1 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c

[ ... ]

> @@ -159,17 +165,20 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>  	entity->priority = priority;
>  	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
>  			      DRM_SCHED_PRIORITY_KERNEL : priority;
> -	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
>  
>  	spin_lock_init(&entity->lock);
>  	spsc_queue_init(&entity->job_queue);
>  
> +	entity->max_sched_list = max_sched_list;
> +	drm_sched_entity_modify_sched(entity, sched_list, num_sched_list);
> +	drm_sched_entity_select_rq(entity);

[Severity: High]
What happens if all schedulers in sched_list have sched->ready == false
when __drm_sched_entity_init() or drm_sched_entity_select_rq() runs (for
example in amdgpu when a ring fails IB ring tests during init/reset/resume
while ring->sched.sched_rq remains allocated)?

Previously, drm_sched_entity_init() unconditionally set:

	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];

and for single-scheduler entities (num_sched_list == 1), entity->sched_list
was set to NULL so drm_sched_entity_select_rq() returned early without
clearing entity->rq.

With this change, __drm_sched_entity_init() leaves entity->rq as NULL from
memset() and calls drm_sched_entity_select_rq():

	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
	if (rq != entity->rq) {
		drm_sched_rq_remove_entity(entity->rq, entity);
		entity->rq = rq;
	}

Because drm_sched_pick_best() skips any scheduler where !sched->ready, it
returns NULL when no scheduler is ready. __drm_sched_entity_init() then
succeeds (returning 0) with entity->rq == NULL, and
drm_sched_entity_select_rq() can also overwrite an existing non-NULL
entity->rq with NULL.

Won't subsequent calls to drm_sched_entity_destroy() ->
drm_sched_entity_flush():

	sched = entity->rq->sched;

or drm_sched_job_arm():

	drm_sched_entity_select_rq(entity);
	sched = entity->rq->sched;

or amdgpu_ctx_fini_entity() -> amdgpu_xcp_release_sched() trigger a NULL
pointer dereference on entity->rq?

[ ... ]

> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 76637e21284ac..38b50a84a8210 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -121,22 +121,11 @@ struct drm_sched_entity {
>  	struct drm_sched_entity_stats	*stats;
>  
>  	/**
> -	 * @sched_list:
> +	 * @max_sched_list:
>  	 *
> -	 * A list of schedulers (struct drm_gpu_scheduler).  Jobs from this entity can
> -	 * be scheduled on any scheduler on this list.
> -	 *
> -	 * This can be modified by calling drm_sched_entity_modify_sched().
> -	 * Locking is entirely up to the driver, see the above function for more
> -	 * details.
> -	 *
> -	 * This will be set to NULL if &num_sched_list equals 1 and @rq has been
> -	 * set already.
> -	 *
> -	 * FIXME: This means priority changes through
> -	 * drm_sched_entity_set_priority() will be lost henceforth in this case.
> +	 * Maximum number of drm_gpu_schedulers in the @sched_list.
>  	 */
> -	struct drm_gpu_scheduler        **sched_list;
> +	unsigned int                    max_sched_list;

[Severity: Medium]
Does drm_sched_entity_set_priority() actually switch run-queues when
per-priority run-queues (such as DRM_SCHED_POLICY_FIFO or
DRM_SCHED_POLICY_RR) are used?

In drivers/gpu/drm/scheduler/sched_entity.c, __drm_sched_entity_init()
initializes both priority fields:

	entity->priority = priority;
	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
			      DRM_SCHED_PRIORITY_KERNEL : priority;

However, drm_sched_entity_set_priority() only updates entity->priority:

void drm_sched_entity_set_priority(struct drm_sched_entity *entity,
				   enum drm_sched_priority priority)
{
	spin_lock(&entity->lock);
	entity->priority = priority;
	spin_unlock(&entity->lock);
}

When drm_sched_entity_select_rq() later selects a run-queue, it indexes
sched->sched_rq using entity->rq_priority:

	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;

Since entity->rq_priority is never updated by
drm_sched_entity_set_priority(), won't the entity remain on its original
priority's run-queue when drm_sched_policy is not DRM_SCHED_POLICY_FAIR?

[ ... ]

> @@ -245,8 +234,38 @@ struct drm_sched_entity {
>  	 */
>  	struct rb_node			rb_tree_node;
>  
> +	union {
> +		/**
> +		 * @__sched__:
> +		 *
> +		 * Built-in storage for callers who only need a single slot.
> +		 *
> +		 * MUST NOT be accessed.
> +		 */
> +		struct drm_gpu_scheduler 	*__sched__;
> +
> +		/**
> +		 * @sched_list:
> +		 *
> +		 * Flexible array of schedulers this entity can pick from, with
> +		 * the driver reponsibility to size the array correctly at
> +		 * declaration and initialization time, and populate the array
> +		 * via drm_sched_entity_init() and/or
> +		 * drm_sched_entity_modify_sched().
> +		 *
> +		 * It can be modified by calling
> +		 * drm_sched_entity_modify_sched().
> +		 */
> +		DECLARE_FLEX_ARRAY(struct drm_gpu_scheduler *, sched_list);
> +	};
>  };
>  
> +#define DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(name, listname, max_scheds) \
> +struct { \
> +	struct drm_sched_entity (name); \
> +	struct drm_gpu_scheduler *(listname)[(max_scheds)]; \
> +}

[ ... ]

>|}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020154123.33631-1-tvrtko.ursulin@igalia.com?part=8