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 <[email protected]> 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/[email protected]?part=1
