Re: [PATCH 1/2] drm/sched: Do not restore unsaved virtual runtime

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A killed and stopped scheduler entity can be unconditionally added back to the runqueue's rbtree, leading to a use-after-free when the entity is subsequently freed by the driver.
--

--- Patch [1]: [PATCH 1/2] drm/sched: Do not restore unsaved virtual runtime ---
commit 8feac77c9420bd8a6d250c6a988250fe755b6870
Author: Tvrtko Ursulin <[email protected]>
Subject: drm/sched: Do not restore unsaved virtual runtime

This patch modifies the queue push/pop and runqueue management paths in the
drm scheduler. It extends the scope of entity->lock to encompass both queue
management and rbtree updates. This prevents a race condition where a new job
could be pushed to an entity before its virtual runtime was correctly saved.

> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index 0464d324d98d5..23f46ec610e78 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c

[ ... ]

> @@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
>   */
>  void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>  {
> +	struct drm_sched_rq *rq = entity->rq;
>  	struct drm_sched_job *next_job;
> -	struct drm_sched_rq *rq;
> +
> +	lockdep_assert_held(&entity->lock);
> +
> +	spin_lock(&rq->lock);
>  
>  	/*
>  	 * Update the entity's location in the min heap according to
>  	 * the timestamp of the next job, if any.
>  	 */
> -	spin_lock(&entity->lock);
> -	rq = entity->rq;
> -	spin_lock(&rq->lock);
>  	next_job = drm_sched_entity_queue_peek(entity);
>  	if (next_job) {

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free if the
entity is killed concurrently?

If drm_sched_entity_kill() in drivers/gpu/drm/scheduler/sched_entity.c is
called on an entity with multiple pending jobs concurrently with the scheduler
thread popping a job:

drm_sched_entity_kill() {
    ...
    spin_lock(&entity->lock);
    entity->stopped = true;
    drm_sched_rq_remove_entity(entity->rq, entity);
    spin_unlock(&entity->lock);
    ...
}

Meanwhile, the scheduler thread popping a job calls drm_sched_rq_pop_entity()
in drivers/gpu/drm/scheduler/sched_rq.c. Since next_job is not NULL, the code
takes this path and calls drm_sched_rq_update_fifo_locked(), which
unconditionally adds the entity back to the runqueue's rbtree, ignoring
entity->stopped.

When the killer thread wakes up and finishes, the entity is freed but still
linked in the rbtree, which corrupts the rbtree.

Should drm_sched_rq_pop_entity() check entity->stopped before re-adding the
entity to the runqueue, similar to the check in drm_sched_rq_add_entity()?

>  		ktime_t ts;
> @@ -375,8 +373,8 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>  			drm_sched_entity_save_vruntime(entity, min_vruntime);
>  		}
>  	}
> +
>  	spin_unlock(&rq->lock);
> -	spin_unlock(&entity->lock);
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1