Re: [PATCH 01/10] drm/vblank_work: Add methods to schedule vblank_work in 2 stages

[sashiko-bot@kernel.org]

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 <dev@lankhorst.se>

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/20260702072154.171324-1-dev@lankhorst.se?part=1