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

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

[Donggeun Yoo]

Hi Philipp,

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);

	/* 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.

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:

  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?

Resent: the original did not reach dri-devel - I was not subscribed at the
time. Apologies to those seeing it twice.

Thanks,
Donggeun

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.

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

[Philipp Stanner]

On Thu, 2026-09-10 at 08:52 +0200, Philipp Stanner wrote:
> 
> 
> 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;
> 
>  /** * @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;
> 

Sorry for the formatting horror. No idea why that happens,  layouts
correctly for me before sending.

I've had enough of this now and will setup Mutt or Aerc.


P.

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

[Christian König]

Hi,

On 9/10/26 08:52, Philipp Stanner wrote:
> +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:

No it doesn't. You quoted the wrong code, this is what really matters:

        drm_sched_wqueue_stop(sched);

        for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
                kfree(sched->sched_rq[i]);

>>
>>  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);

That was an extremely ugly hack applied long long time ago because amdgpu was broken at that time and didn't waited for drm_sched_entity_flush()/drm_sched_entity_fini() before calling drm_sched_fini().

>>
>> 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:

Amdgpu shouldn't do that any more.

It is perfectly possible that we still have a bug in the driver teardown during hot remove, but we certainly shouldn't work around that by adding such hacks to the scheduler.

> 
> 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.

Yes, exactly that. This line should potentially just be removed.

Regards,
Christian.

> 
>>
>>   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.

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

[Donggeun Yoo]

On 9/10/26 09:32, Christian König wrote:
> Amdgpu shouldn't do that any more.

Correct, and I should have checked before writing it - 182bdd59be41
("drm/amdgpu: deprecate guilty handling") removed it. The callers left are
etnaviv, lima, panfrost and v3d. v3d is the one I should have named.

> That was an extremely ugly hack applied long long time ago because amdgpu
> was broken at that time and didn't waited for
> drm_sched_entity_flush()/drm_sched_entity_fini() before calling
> drm_sched_fini().

Understood, I am dropping that half of the argument.

> No it doesn't. You quoted the wrong code, this is what really matters:
>
>         drm_sched_wqueue_stop(sched);
>
>         for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
>                 kfree(sched->sched_rq[i]);

I am not sure I follow this one. If the point is that
drm_sched_wqueue_stop() has already quiesced the users of the run queues
by the time the loop runs, I cannot find where it covers the timeout work:

	WRITE_ONCE(sched->pause_submit, true);
	cancel_work_sync(&sched->work_run_job);
	cancel_work_sync(&sched->work_free_job);

work_tdr is queued on sched->timeout_wq and is only canceled by the
cancel_delayed_work_sync() below the loop, so a timeout handler can still
be running while the run queues are freed. Is there something else that
rules that out? And if I have misread your point, please elaborate.

On how I got there: the KUnit case never signals the hardware fence, which
is what keeps the handler inside timedout_job() while drm_sched_fini() runs.
That breaks the rule that all run_job() fences are signaled before
drm_sched_fini(), so a correct driver should not reach this, and I have no
reproducer that does not cheat that way. The same caveat is in the patch.

I am writing up the patch Philipp asked for. The only change is moving the
kfree loop down beside kfree(sched->sched_rq); no new code.

Regards,
Donggeun

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

[Philipp Stanner]

On Thu, 2026-09-10 at 17:44 +0900, Donggeun Yoo wrote:
> On 9/10/26 09:32, Christian König wrote:
> 
> work_tdr is queued on sched->timeout_wq and is only canceled by the
> cancel_delayed_work_sync() below the loop, so a timeout handler can still
> be running while the run queues are freed.

It is not as obvious to me as it is to you why you think that the
timeout handler is exploding.

> On how I got there: the KUnit case never signals the hardware fence, which

Which KUnit test case exactly?

I kindly asked you to provide more details about how and where the bug
occurs. Can you post a longer stacktrace and also run
scrips/decode_stacktrace.sh on it?

> is what keeps the handler inside timedout_job() while drm_sched_fini() runs.
> That breaks the rule that all run_job() fences are signaled before
> drm_sched_fini(), so a correct driver should not reach this, and I have no
> reproducer that does not cheat that way. The same caveat is in the patch.

If the bug only exists because someone does not signal all hardware-
fences (that's what we call the ones returned from run_job()), then I
tend to think that this is not a scheduler bug.

Maybe a kunit test bug that should be fixed.

Though for robustness reasons we _could_ nevertheless stop the timeout
work item before releasing other resources. But that's just a best-
effort nice-to-have change, since the change making a difference would
mean that there are life time violations with ordering issues anyways.


P.

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

[Donggeun Yoo]

On 9/10/26 10:51, Philipp Stanner wrote:
> Which KUnit test case exactly?

A local one. It is not in the tree - I wrote it for this, which is why you
could not find it. I should have said that explicitly.

It is a mock scheduler whose run_job() returns a hardware fence that is
never signaled, so the timeout always fires. timedout_job() then sleeps
long enough for the test thread to get into drm_sched_fini(), and calls
drm_sched_increase_karma() on the way out.

> I kindly asked you to provide more details about how and where the bug
> occurs. Can you post a longer stacktrace and also run
> scrips/decode_stacktrace.sh on it?

drm-misc-next 0878e6053d01, x86_64, KUNIT + KASAN + lockdep, run through
decode_stacktrace.sh (dropping the "? " speculative frames and shortening
the source paths, otherwise as emitted):

BUG: KASAN: slab-use-after-free in _raw_spin_lock (kernel/locking/spinlock.c:173)
Read of size 1 at addr ffff88800198b420 by task kworker/0:2/27

CPU: 0 UID: 0 PID: 27 Comm: kworker/0:2 Tainted: G                 N  7.3.0-rc2-00228-g483f69ec8ca2-dirty #7 PREEMPT(lazy)
Workqueue: events drm_sched_job_timedout
Call Trace:
 <TASK>
 dump_stack_lvl (lib/dump_stack.c:94 lib/dump_stack.c:120)
 print_report (mm/kasan/report.c:378 mm/kasan/report.c:482)
 kasan_report (mm/kasan/report.c:595)
 __kasan_check_byte (mm/kasan/common.c:574)
 lock_acquire (kernel/locking/lockdep.c:5916 kernel/locking/lockdep.c:5899)
 _raw_spin_lock (kernel/locking/spinlock.c:173)
 drm_sched_increase_karma (drivers/gpu/drm/scheduler/sched_main.c:1263)
 fini_uaf_timedout_job (drivers/gpu/drm/scheduler/tests/tests_fini_uaf.c:99)
 drm_sched_job_timedout (drivers/gpu/drm/scheduler/sched_main.c:355)
 process_one_work (kernel/workqueue.c:3396)
 worker_thread (kernel/workqueue.c:3479 kernel/workqueue.c:3560)
 kthread (kernel/kthread.c:436)
 ret_from_fork (arch/x86/kernel/process.c:158)
 ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
 </TASK>

Allocated by task 28:
 __kmalloc_cache_noprof (mm/slub.c:5563)
 drm_sched_init (drivers/gpu/drm/scheduler/sched_main.c:1148)
 drm_sched_fini_frees_rq_before_tdr (drivers/gpu/drm/scheduler/tests/tests_fini_uaf.c:140)
 kunit_try_run_case (lib/kunit/test.c:454 lib/kunit/test.c:499)

Freed by task 28:
 kfree (mm/slub.c:6792)
 drm_sched_fini (drivers/gpu/drm/scheduler/sched_main.c:1214)
 drm_sched_fini_frees_rq_before_tdr (drivers/gpu/drm/scheduler/tests/tests_fini_uaf.c:167)
 kunit_try_run_case (lib/kunit/test.c:454 lib/kunit/test.c:499)

The buggy address belongs to the object at ffff88800198b400
which belongs to the cache kmalloc-128 of size 128
The buggy address is located 32 bytes inside of
freed 128-byte region [ffff88800198b400, ffff88800198b480)

sched_main.c:1148 is sched->sched_rq[i] = kzalloc_obj(*sched->sched_rq[i])
in drm_sched_init(), :1214 is kfree(sched->sched_rq[i]) in
drm_sched_fini(), and :1263 is spin_lock(&rq->lock) in
drm_sched_increase_karma(). Task 28 is the thread in drm_sched_fini();
the reader is the timeout worker on PID 27. The tree is -dirty because the
test case is added to it. I hope this is what you asked for - say the word
if you want the untrimmed log.

> If the bug only exists because someone does not signal all hardware-
> fences (that's what we call the ones returned from run_job()), then I
> tend to think that this is not a scheduler bug.

Agreed.

> Though for robustness reasons we _could_ nevertheless stop the timeout
> work item before releasing other resources.

Right, that's what my patch does: drm_sched_wqueue_stop(), then
cancel_delayed_work_sync(&sched->work_tdr), then the frees. The report is
gone and nothing else in the suite fails.

As you say, that is closer to a cleanup - or to making the teardown order
state its intent - than to a fix. Do you still want the reordering patch?

Regards,
Donggeun

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

[Philipp Stanner]

On Thu, 2026-09-10 at 18:50 +0900, Donggeun Yoo wrote:
> On 9/10/26 10:51, Philipp Stanner wrote:
> > Which KUnit test case exactly?
> 
> A local one. It is not in the tree - I wrote it for this, which is why you
> could not find it. I should have said that explicitly.

OK. For the future please provide the reproducer somehow, for example
through a repo link.

> 
> It is a mock scheduler whose run_job() returns a hardware fence that is
> never signaled,

Alright, so as we agreed on that's then not a bug fix relevant for
upstream.

> 
> Right, that's what my patch does: drm_sched_wqueue_stop(), then
> cancel_delayed_work_sync(&sched->work_tdr), then the frees. The report is
> gone and nothing else in the suite fails.
> 
> As you say, that is closer to a cleanup - or to making the teardown order
> state its intent - than to a fix. Do you still want the reordering patch?

I think it's a good idea. I'd move the deallocations above the related
kfree at the bottom.


What would be more valuable, though, would be a hint in the docu that
the hardware fences must have been signaled before drm_sched_fini() is
called. I thought we had sth like that, but apparently we don't.

@Christian: Could you address this here [1]?


Thanks
P.


[1] https://lore.kernel.org/dri-devel/20260909131808.2201-3-christian.koenig@amd.com/

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

[Christian König]

On 9/10/26 10:44, Donggeun Yoo wrote:
> On 9/10/26 09:32, Christian König wrote:
>> Amdgpu shouldn't do that any more.
> 
> Correct, and I should have checked before writing it - 182bdd59be41
> ("drm/amdgpu: deprecate guilty handling") removed it. The callers left are
> etnaviv, lima, panfrost and v3d. v3d is the one I should have named.
> 
>> That was an extremely ugly hack applied long long time ago because amdgpu
>> was broken at that time and didn't waited for
>> drm_sched_entity_flush()/drm_sched_entity_fini() before calling
>> drm_sched_fini().
> 
> Understood, I am dropping that half of the argument.
> 
>> No it doesn't. You quoted the wrong code, this is what really matters:
>>
>>         drm_sched_wqueue_stop(sched);
>>
>>         for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
>>                 kfree(sched->sched_rq[i]);
> 
> I am not sure I follow this one.

What I wanted to note is that the line you pointed out is irrelevant, it was only added as a (very hacky) workaround for amdgpu and should most likely be entirely removed from drm_sched_fini.

> If the point is that
> drm_sched_wqueue_stop() has already quiesced the users of the run queues
> by the time the loop runs, I cannot find where it covers the timeout work:

Before calling drm_sched_fini the driver must ensure that all HW fences are signaled. Those HW fences then signal the scheduler fence and terminate the timeout handling.

But it is correct that drm_sched_fini() needs to make sure that this terminating the timeout handling has propagated throughout the system.

>         WRITE_ONCE(sched->pause_submit, true);
>         cancel_work_sync(&sched->work_run_job);
>         cancel_work_sync(&sched->work_free_job);
> 
> work_tdr is queued on sched->timeout_wq and is only canceled by the
> cancel_delayed_work_sync() below the loop, so a timeout handler can still
> be running while the run queues are freed. Is there something else that
> rules that out? And if I have misread your point, please elaborate.

That is a really good point, canceling the timeout handler should indeed happen before freeing the runqueues.

> On how I got there: the KUnit case never signals the hardware fence, which
> is what keeps the handler inside timedout_job() while drm_sched_fini() runs.

Oh, that is completely broken behavior of the KUnit test case. I suggest to fix that as well.

> That breaks the rule that all run_job() fences are signaled before
> drm_sched_fini(), so a correct driver should not reach this, and I have no
> reproducer that does not cheat that way. The same caveat is in the patch.
> 
> I am writing up the patch Philipp asked for. The only change is moving the
> kfree loop down beside kfree(sched->sched_rq); no new code.

That sounds reasonable to me as well, yes.

Regards,
Christian.

> 
> Regards,
> Donggeun

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

[Donggeun Yoo]

On 9/10/26 15:58, Christian König wrote:
> That is a really good point, canceling the timeout handler should indeed
> happen before freeing the runqueues.

Posted, in a separate thread:

  https://lore.kernel.org/dri-devel/20260910121601.805032-1-donggeunyoo.kernel@gmail.com/

> Oh, that is completely broken behavior of the KUnit test case. I suggest to
> fix that as well.

Agreed - it never signals the hardware fence, on purpose, only to find out
whether the sequence was reachable at all. A local probe, not something
meant for the tree.

Regards,
Donggeun