[PATCH] drm/sched: Free the run queues at the end of drm_sched_fini()

[PATCH] drm/sched: Free the run queues at the end of drm_sched_fini()

[Donggeun Yoo]

drm_sched_fini() frees the run queues at the top of teardown but the array
holding them at the bottom. The early half is on the wrong side of
cancel_delayed_work_sync(&sched->work_tdr), which waits for a timeout
handler that can still walk sched->sched_rq[i] through
drm_sched_increase_karma().

No correct driver can be there, since every fence returned from run_job()
must be signaled before drm_sched_fini() is called. Free the entries next
to the array anyway, so run-queue teardown happens in one place.

Link: https://lore.kernel.org/dri-devel/20260910054605.634135-1-donggeunyoo.kernel@gmail.com/
Assisted-by: Claude:claude-fable-5
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
Targets drm-misc-next (0878e6053d01).

A cleanup - no Fixes:, no Cc: stable.

The KUnit case and how to run it:
https://github.com/donggeunyoo/drm-sched-fini-uaf-repro

x86_64 under QEMU, KUNIT + KASAN + lockdep, whole drm_sched suite, three
runs per arm:

  before  38-41 KASAN slab-use-after-free reports, all from
          drm_sched_increase_karma() on the timeout worker
  after   0

 drivers/gpu/drm/scheduler/sched_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 6cb6f9546493..fec04c944c5e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1210,9 +1210,6 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
 
 	drm_sched_wqueue_stop(sched);
 
-	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);
 
@@ -1226,6 +1223,9 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
 	sched->ready = false;
+
+	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
+		kfree(sched->sched_rq[i]);
 	kfree(sched->sched_rq);
 	sched->sched_rq = NULL;
 
-- 
2.53.0

Re: [PATCH] drm/sched: Free the run queues at the end of drm_sched_fini()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Potential Use-After-Free of `sched` and `sched->sched_rq` if `drm_sched_fini()` is called with pending jobs and `own_submit_wq` is false.
--

commit f30738456b70961d8d90c374f4f4803079a2fecd
Author: Donggeun Yoo <donggeunyoo.kernel@gmail.com>

drm/sched: Free the run queues at the end of drm_sched_fini()

This commit moves the freeing of the scheduler run queues from the top of
drm_sched_fini() to the bottom. This prevents a use-after-free where the
timeout handler might still be walking the run queues before
cancel_delayed_work_sync() completes.

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

> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 6cb6f95464938..fec04c944c5e7 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
[ ... ]
> @@ -1226,6 +1223,9 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
>  	if (sched->own_submit_wq)
>  		destroy_workqueue(sched->submit_wq);

[Severity: Critical]
This is a pre-existing issue, but does skipping the workqueue drain when
own_submit_wq is false leave us exposed to a use-after-free here?

If a job timeout occurs concurrently with context teardown, drm_sched_fini()
calls cancel_delayed_work_sync(&sched->work_tdr), which waits for the timeout
handler to complete.

Drivers like Panthor and PVR implement the timeout handler by resetting the
hardware and calling drm_sched_start(), which calls drm_sched_wqueue_start().
This unconditionally queues the work item back onto the shared workqueue:

drivers/gpu/drm/scheduler/sched_main.c:drm_sched_wqueue_start() {
    ...
    queue_work(sched->submit_wq, &sched->work_run_job);
    ...
}

After the timeout handler returns, cancel_delayed_work_sync() finishes.
Because these drivers use a shared workqueue, own_submit_wq is false, so
the workqueue is neither drained nor destroyed.

When drm_sched_fini() then frees sched->sched_rq and returns, the driver can
subsequently free the memory containing the scheduler. Can the shared
submit_wq then execute the re-queued work_run_job, accessing the freed
sched and sched->sched_rq?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910121601.805032-1-donggeunyoo.kernel@gmail.com?part=1

Re: [PATCH] drm/sched: Free the run queues at the end of drm_sched_fini()

[Donggeun Yoo]

On 9/10/26 21:37, sashiko-bot@kernel.org wrote:
> [Critical] Potential Use-After-Free of `sched` and `sched->sched_rq` if
> `drm_sched_fini()` is called with pending jobs and `own_submit_wq` is false.

The chain is there in the scheduler. drm_sched_start() queues work_run_job
unconditionally, and drm_sched_fini() destroys submit_wq only when it owns it,
so a timeout handler restarting the scheduler during teardown could leave work
behind that later reads sched->sched_rq[i].

No in-tree driver reaches it. Of the four that pass their own submit_wq:

  - panthor sets .timeout = MAX_SCHEDULE_TIMEOUT on both its schedulers, which
    drm_sched_start_timeout() tests before anything else, so the TDR never arms;
  - powervr refcounts the context, and pvr_queue_destroy() runs only after the
    last reference is dropped, which in-flight jobs hold;
  - nouveau waits on nouveau_sched_job_list_empty() before drm_sched_fini();
  - xe never calls drm_sched_start().

Both drivers named in the report are covered, for different reasons. So there
is nothing here for this patch, which only moves the run queue frees within
drm_sched_fini().

The one thing worth recording is that the protection is per-driver rather than
structural. A driver combining its own submit_wq, a finite timeout, and
drm_sched_start() in timedout_job would have to drain before teardown itself.

Regards,
Donggeun