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

[sashiko-bot@kernel.org]

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