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.