[PATCH 1/2] drm/sched: Guard sched->ready with ACCESS_ONCE()

[PATCH 1/2] drm/sched: Guard sched->ready with ACCESS_ONCE()

[Philipp Stanner]

commit faf6e1a87e07 ("drm/sched: Add boolean to mark if sched is ready to work v5")

moved tracking of the hardware ring's state from the driver (amdgpu in
that case) into drm_sched. To do so, it added a 'ready' flag to the
scheduler.

This flag is currently being accessed through drm_sched_wqueue_ready()
and, even worse, directly through the scheduler struct. Since drm_sched
does not have a consistent locking design, all these accesses are
potentially undefined behavior as they are subject to compiler
optimizations.

Make the code base more robust by guarding access to the 'ready' flag
with ACCESS_ONCE().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_main.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index d2ca01b31ee4..c4e4ac436a86 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -929,7 +929,7 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
 	for (i = 0; i < num_sched_list; ++i) {
 		sched = sched_list[i];
 
-		if (!sched->ready) {
+		if (!READ_ONCE(sched->ready)) {
 			DRM_WARN("scheduler %s is not ready, skipping",
 				 sched->name);
 			continue;
@@ -1143,7 +1143,18 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
 
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
-	sched->ready = false;
+
+	/* The 'ready' flag only exists in drm_sched because amdgpu uses it to
+	 * represent the state of its hardware rings. This problem is related to
+	 * the fundamental issue of drm_sched not having a solid, consistent
+	 * locking design.
+	 *
+	 * Obviously, it does not make sense at all to set this flag to false
+	 * here, but since it's unclear whether it can ever be removed from
+	 * amdgpu's point of view, we guard it here with WRITE_ONCE() to make it
+	 * slightly less broken.
+	 */
+	WRITE_ONCE(sched->ready, false);
 
 	if (!list_empty(&sched->pending_list))
 		dev_warn(sched->dev, "Tearing down scheduler while jobs are pending!\n");
@@ -1195,7 +1206,7 @@ EXPORT_SYMBOL(drm_sched_increase_karma);
  */
 bool drm_sched_wqueue_ready(struct drm_gpu_scheduler *sched)
 {
-	return sched->ready;
+	return READ_ONCE(sched->ready);
 }
 EXPORT_SYMBOL(drm_sched_wqueue_ready);
 

base-commit: 6648301c5bb2ef23f0fb15bcb01d21ff66f36799
-- 
2.54.0

[PATCH 2/2] drm/sched: Deprecate drm_sched_wqueue_ready()

[Philipp Stanner]

drm_sched_wqueue_ready() stems from an old rework, see

commit faf6e1a87e07 ("drm/sched: Add boolean to mark if sched is ready to work v5")

That commit moved tracking of the hardware ring's state was moved into
drm_sched.

This is highly racy and problematic and something that should most
certainly be covered by the driver.

Deprecate drm_sched_wqueue_ready().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index c4e4ac436a86..35a7892c8dc8 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1198,10 +1198,14 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
 EXPORT_SYMBOL(drm_sched_increase_karma);
 
 /**
- * drm_sched_wqueue_ready - Is the scheduler ready for submission
+ * drm_sched_wqueue_ready - Is the scheduler ready for submission (DEPRECATED)
  *
  * @sched: scheduler instance
  *
+ * Deprecated, don't use it in new code. This function was added to have the
+ * scheduler represent the hardware ring's state, which must be represented by
+ * the driver's respective data structures.
+ *
  * Returns true if submission is ready
  */
 bool drm_sched_wqueue_ready(struct drm_gpu_scheduler *sched)
-- 
2.54.0

Re: [PATCH 2/2] drm/sched: Deprecate drm_sched_wqueue_ready()

[Danilo Krummrich]

On 6/29/26 12:40 PM, Philipp Stanner wrote:
> Deprecate drm_sched_wqueue_ready().

AFAICS, this is only used in amdgpu and the implementation is a single line. If
we don't want people to use, I'd rather just remove it and let amdgpu open-code it.

Re: [PATCH 1/2] drm/sched: Guard sched->ready with ACCESS_ONCE()

[sashiko-bot]

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

New issues:
- [Low] The commit message incorrectly references the deprecated ACCESS_ONCE() macro.

Pre-existing issues:
- [Critical] When all schedulers are marked as not ready, drm_sched_pick_best() returns NULL, leading to a fatal NULL pointer dereference during job submission.
- [High] A TOCTOU race in drm_sched_run_job_queue allows work to be queued to a destroyed workqueue during scheduler teardown.
--

commit c1a983b76226585f5c944e3f1f105517183427a9
Author: Philipp Stanner <phasta@kernel.org>
Subject: drm/sched: Guard sched->ready with ACCESS_ONCE()

This commit modifies the tracking of the scheduler's ready state by wrapping
accesses to the 'ready' flag with READ_ONCE() and WRITE_ONCE() to prevent
compiler optimization issues.

> drm/sched: Guard sched->ready with ACCESS_ONCE()
>
> commit faf6e1a87e07 ("drm/sched: Add boolean to mark if sched is ready to work v5")
[ ... ]
> Make the code base more robust by guarding access to the 'ready' flag
> with ACCESS_ONCE().

[Severity: Low]
The commit message claims to guard access with ACCESS_ONCE(), but the code
actually uses READ_ONCE() and WRITE_ONCE(). ACCESS_ONCE() was deprecated
and removed from the kernel. Could the commit message be updated to match
the implementation?

> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -929,7 +929,7 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>  	for (i = 0; i < num_sched_list; ++i) {
>  		sched = sched_list[i];
>  
> -		if (!sched->ready) {
> +		if (!READ_ONCE(sched->ready)) {

[Severity: Critical]
This is a pre-existing issue, but does this lead to a fatal page fault if
all schedulers are marked as not ready? 

If all schedulers in entity->sched_list fail this READ_ONCE(sched->ready)
check, drm_sched_pick_best() returns NULL. When this happens,
drm_sched_entity_select_rq() sets entity->rq to NULL. 

Later, when a job is submitted, drm_sched_job_arm() computes a pointer
using container_of(entity->rq, ...), which results in an invalid negative
pointer since entity->rq is NULL. This invalid pointer is then dereferenced
in drm_sched_entity_push_job() via atomic_inc(sched->score).

The documentation for drm_sched_job_init() mentions returning -ENOENT when
the hardware has died (which causes this ready flag to be false), but the
function actually returns 0. Should drm_sched_job_init() or the callers
be updated to properly handle the NULL return from drm_sched_pick_best()?

>  			DRM_WARN("scheduler %s is not ready, skipping",
>  				 sched->name);
>  			continue;
[ ... ]
> @@ -1143,7 +1143,18 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
>  
>  	if (sched->own_submit_wq)
>  		destroy_workqueue(sched->submit_wq);

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
risk when destroying the workqueue here?

Functions like drm_sched_run_job_queue() check !drm_sched_is_stopped(sched)
locklessly before calling queue_work(sched->submit_wq, ...). If a thread is
preempted right after the check, and concurrently drm_sched_fini() calls
drm_sched_wqueue_stop(), the cancel_work_sync() will return immediately
because the work isn't queued yet. 

drm_sched_fini() then proceeds to destroy sched->submit_wq. When the
preempted thread resumes, it calls queue_work() on the destroyed workqueue.
Can the workqueue destruction be safely synchronized against threads that
might be paused in this time-of-check to time-of-use window?

> -	sched->ready = false;
> +
> +	/* The 'ready' flag only exists in drm_sched because amdgpu uses it to
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629104040.2695163-2-phasta@kernel.org?part=1

Re: [PATCH 1/2] drm/sched: Guard sched->ready with ACCESS_ONCE()

[Danilo Krummrich]

> +	WRITE_ONCE(sched->ready, false);

Don't we need smp_store_release() here and

> +	return READ_ONCE(sched->ready);
smp_load_acquire() here?

Also, what about drm_sched_init()? It also seems that this is accessed from
amdgpu without the drm_sched_wqueue_ready() helper about a million times. :)

	$ grep -Rin "sched\.ready" drivers/gpu/drm/amd | wc
	$     119     544   10320

There may be false positives, but from a quick glance at least most of them seem
to actually come from the scheduler.

Re: [PATCH 1/2] drm/sched: Guard sched->ready with ACCESS_ONCE()

[Philipp Stanner]

On Mon, 2026-06-29 at 14:09 +0200, Danilo Krummrich wrote:
> > +	WRITE_ONCE(sched->ready, false);
> 
> Don't we need smp_store_release() here and
> 
> > +	return READ_ONCE(sched->ready);
> smp_load_acquire() here?

Maybe. Not sure what the precise access rules would be. To really get
correctness, all counter-parties you found in amdgpu would have to
remove their layering violations anyways, which is beyond my scope.

> 
> Also, what about drm_sched_init()? It also seems that this is accessed from
> amdgpu without the drm_sched_wqueue_ready() helper about a million times. :)
> 
> 	$ grep -Rin "sched\.ready" drivers/gpu/drm/amd | wc
> 	$     119     544   10320
> 
> There may be false positives, but from a quick glance at least most of them seem
> to actually come from the scheduler.

Correct.

Your ordering comment hints at me that you would rather see the ready-
flag be left as is, if it can't be made right 100% (which it can't).

My idea was more to at least document the UB / race and make it
slightly less broken with a reasonable cost-benefit-ratio


P.

Re: [PATCH 1/2] drm/sched: Guard sched->ready with ACCESS_ONCE()

[Danilo Krummrich]

On Mon Jul 6, 2026 at 10:44 AM CEST, Philipp Stanner wrote:
> On Mon, 2026-06-29 at 14:09 +0200, Danilo Krummrich wrote:
>> > +	WRITE_ONCE(sched->ready, false);
>> 
>> Don't we need smp_store_release() here and
>> 
>> > +	return READ_ONCE(sched->ready);
>> smp_load_acquire() here?
>
> Maybe. Not sure what the precise access rules would be. To really get
> correctness, all counter-parties you found in amdgpu would have to
> remove their layering violations anyways, which is beyond my scope.

Fair enough, let's get it right in the core infrastructure then.

>> Also, what about drm_sched_init()? It also seems that this is accessed from
>> amdgpu without the drm_sched_wqueue_ready() helper about a million times. :)
>> 
>> 	$ grep -Rin "sched\.ready" drivers/gpu/drm/amd | wc
>> 	$     119     544   10320
>> 
>> There may be false positives, but from a quick glance at least most of them seem
>> to actually come from the scheduler.
>
> Correct.
>
> Your ordering comment hints at me that you would rather see the ready-
> flag be left as is, if it can't be made right 100% (which it can't).

No, the change makes sense. Getting it right in the core infrastructure is the
right thing to do, even if you don't want to fix up amdgpu.