Re: drm/sched: run queues freed before the TDR that drm_sched_fini() waits for

[Philipp Stanner]

+Cc Tvrtko

On Thu, 2026-09-10 at 14:46 +0900, Donggeun Yoo wrote:
> Hi Philipp,

Hello,

> 
> drm_sched_fini() frees the run queues above the two steps that wait for
> users of them:
> 
>  for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
>  kfree(sched->sched_rq[i]);
> 
>  /* Wakeup everyone stuck in drm_sched_entity_flush for this scheduler */
>  wake_up_all(&sched->job_scheduled);

If we look at that, it indeed seems broken:

long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) { struct drm_gpu_scheduler *sched; struct task_struct *last_user; long ret = timeout; if (!entity->rq) return 0; sched = entity->rq->sched;


At least from the commentary; one would free the pointer of the very
party we say we're waiting for.

Maybe @Tvrtko can take a look, who has a sophisticated understanding of
runqueue management.


Let me mention, though, that the entire RQ pointer handling is
basically undefined behavior since forever:

 /** * @rq: * * Runqueue on which this entity is currently scheduled. * * FIXME: Locking is very unclear for this. Writers are protected by * @lock, but readers are generally lockless and seem to just race with * not even a READ_ONCE. */ struct drm_sched_rq *rq;

Fixing that is also on our TODO list for interested contributors:

https://docs.kernel.org/gpu/todo.html#add-locking-for-runqueues

> 
>  /* Confirm no work left behind accessing device structures */
>  cancel_delayed_work_sync(&sched->work_tdr);
> 
> 4827d6d83f07 ("drm/sched: Remove racy hack from drm_sched_fini()") did not
> change that ordering - the kfree() was above the wakeup before it as well,
> and has been since 56e449603f0a ("drm/sched: Convert the GPU scheduler to
> variable number of run-queues") made the run queues separately allocated.
> But with the loop body gone there no longer seems to be anything holding
> the free up there.

Then it would have been a race condition, where the occurrence of the
bug depends on timing.

> 
> A KUnit case that keeps the TDR inside timedout_job() while drm_sched_fini()
> runs, with the callback calling drm_sched_increase_karma() as amdgpu does:

How precisely are you reproducing the problem? I'm wondering why it
wasn't observed sooner.

Since you mention amdgpu, many of the twirks in the drm_sched code base
were added to work around ordering problems in that driver.

The drm_sched life time rules are:

   1. All fences returned through ops->run_job() must be signaled by
      the driver before calling drm_sched_fini().
   2. Entities must be torn down before the scheduler.

IOW, I would dare to say that the wake_up_all() maybe shouldn't even
exist in an ideal world, because the driver would never call
drm_sched_fini() while it's still blocking in drm_sched_entity_flush().
Maybe Christian has some wisdom on the background.

> 
>   BUG: KASAN: slab-use-after-free in _raw_spin_lock+0x2b/0x40
>   Workqueue: events drm_sched_job_timedout
>    drm_sched_increase_karma+0x138/0x3e0
>    fini_uaf_timedout_job+0x4c/0x140
>    drm_sched_job_timedout+0x1b4/0x620
>   allocated by drm_sched_init+0x49c, freed by drm_sched_fini+0xec
> 
> Moving the loop down beside kfree(sched->sched_rq) silences it, and nothing
> between the two positions reads the run queues. Is that the right fix, or is
> the intended rule that the TDR can never still be running at that point?

Can you post a patch or RFC patch for fixing it? It's easier to discuss
then. I think I get what you mean, and it's probably the best cost-
benefit-ratio fix. I'd then just put some brain power into
understanding the ordering between threads though



Regards
P.