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
