Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Missing read memory barrier (`smp_rmb` or `smp_load_acquire`) when reading `work->armed` in `drm_handle_vblank_works` breaks transitive ordering. - [Medium] `drm_vblank_work_schedule` early return logic fails to arm a disabled work if the target vblank count is identical. Pre-existing issues: - [High] Use-after-free risk due to calling `list_del_init(&work->node)` after `kthread_queue_work()`. -- commit 2d70bdfd10b5fd90f1bf2ecac514ae3f86ae65ba Author: Maarten Lankhorst <[email protected]> drm/vblank_work: Add methods to schedule vblank_work in 2 stages In case of vblank evasion in intel/display, it's necessary to perform some work in advance, so the critical section will always run in constant time on PREEMPT_RT. > diff --git a/drivers/gpu/drm/drm_vblank_work.c > b/drivers/gpu/drm/drm_vblank_work.c > index 70f0199251ea0..ed62c189fe041 100644 > --- a/drivers/gpu/drm/drm_vblank_work.c > +++ b/drivers/gpu/drm/drm_vblank_work.c > @@ -54,7 +54,8 @@ void drm_handle_vblank_works(struct drm_vblank_crtc *vblank) > assert_spin_locked(&vblank->dev->event_lock); > > list_for_each_entry_safe(work, next, &vblank->pending_work, node) { > - if (!drm_vblank_passed(count, work->count)) > + /* READ_ONCE pairs with WRITE_ONCE in drm_vblank_work_enable() > */ > + if (!READ_ONCE(work->armed) || !drm_vblank_passed(count, > work->count)) > continue; [Severity: High] Does this READ_ONCE() need a corresponding read memory barrier like smp_rmb() or smp_load_acquire() to establish transitive ordering? Since drm_vblank_work_enable() uses an smp_wmb() before writing to work->armed, if the reader just uses READ_ONCE() here without an acquire barrier before transferring the work via kthread_queue_work(), the memory model doesn't guarantee the writer's data updates will be transitively visible to the worker thread. Could the kthread end up reading stale driver state as a result? [ ... ] > @@ -139,6 +118,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work, > rescheduling = true; > } > > + work->armed = armed; > work->count = count; [Severity: Medium] Does the early return logic in __drm_vblank_work_schedule() break if a work item is first scheduled disabled, and then re-scheduled to be armed with the exact same target count? Earlier in __drm_vblank_work_schedule(), if work->count == count, the function takes an early return: } else if (work->count == count) { /* Already scheduled w/ same vbl count */ goto out; } else { If a driver uses drm_vblank_work_schedule_disabled() to schedule the work, and later calls drm_vblank_work_schedule() with the exact same count to arm it, doesn't the early return bypass this new work->armed = armed assignment? Could this leave the work permanently disabled and never executed? [ ... ] > @@ -147,7 +127,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work, > "crtc %d vblank %llu already passed (current > %llu)\n", > vblank->pipe, count, cur_vbl); > > - if (!nextonmiss && passed) { > + if (!nextonmiss && passed && armed) { > drm_vblank_put(dev, vblank->pipe); > ret = kthread_queue_work(vblank->worker, &work->base); > [Severity: High] This is a pre-existing issue, but is there a use-after-free risk here when rescheduling a work item that has already passed? If rescheduling is true, the code right below this in __drm_vblank_work_schedule() does: ret = kthread_queue_work(vblank->worker, &work->base); if (rescheduling) { list_del_init(&work->node); wake = true; } Since kthread_queue_work() immediately places the work on the worker's queue, could the worker thread run concurrently and execute the work before list_del_init() is called on the current CPU? If the work is freed by the executing kthread, the list_del_init() call would then access freed memory. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
