Re: [PATCH RFC] drm/sysfb: Copy damage outside the DIRTYFB ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/sysfb: busy-spin and livelock in drm_sysfb_fb_dirty()
  flush_work() loop
- [High] drm/sysfb: race between drm_sysfb_damage_work() and PM
  suspend/resume
- [Medium] drm/sysfb: lock inversion holding drm_dev_enter() in
  drm_sysfb_damage_work()
- [Medium] drm/sysfb: concurrent frontbuffer mutation and lock stall in
  drm_sysfb_damage_work()
--

commit 092a1b6cbfb2ea4259c79aad2c25c2ec9f2a259a
Author: Joseph Telaak <joseph@telaak.dev>

drm/sysfb: Copy damage outside the DIRTYFB ioctl

This commit defers sysfb framebuffer damage updates from the DIRTYFB ioctl to
a workqueue item that merges damage rectangles and runs
drm_atomic_helper_dirtyfb() asynchronously.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index 13df0925163a5..e77f0c0ffc272 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
[ ... ]
> +static void drm_sysfb_damage_work(struct work_struct *work)
> +{
> +	struct drm_sysfb_device *sysfb = container_of(work, struct drm_sysfb_device,
> +						      damage_work);
> +	struct drm_framebuffer *fb;
> +	struct drm_clip_rect clip;
> +	int idx, ret;
> +
> +	spin_lock(&sysfb->damage_lock);
> +	fb = sysfb->damage_fb;
> +	sysfb->damage_fb = NULL;
> +	clip.x1 = sysfb->damage.x1;
> +	clip.y1 = sysfb->damage.y1;
> +	clip.x2 = sysfb->damage.x2;
> +	clip.y2 = sysfb->damage.y2;
> +	spin_unlock(&sysfb->damage_lock);
> +	if (!fb)
> +		return;
> +
> +	if (drm_dev_enter(&sysfb->dev, &idx)) {
> +		ret = drm_atomic_helper_dirtyfb(fb, NULL, 0, 0, &clip, 1);
> +		if (ret)
> +			drm_err_ratelimited(&sysfb->dev, "damage update failed: %d\n", ret);
> +		drm_dev_exit(idx);
> +	}
> +	drm_framebuffer_put(fb);
> +}

[Severity: High]
Can drm_sysfb_damage_work() race with system suspend and resume, causing a
use-after-free or NULL pointer dereference on plane->state?

All sysfb drivers use drm_mode_config_helper_suspend() and
drm_mode_config_helper_resume(), which do not flush or cancel
sysfb->damage_work, and system_dfl_wq is not freezable.  During resume,
drm_atomic_helper_resume() calls drm_mode_config_reset() before acquiring
modeset locks, which destroys and reallocates plane->state unlocked:

CPU0                              CPU1 (kworker)
drm_mode_config_helper_resume()
  drm_atomic_helper_resume()
    drm_mode_config_reset()
      ...
        kfree(plane->state)
        plane->state = NULL
                                  drm_sysfb_damage_work()
                                    drm_atomic_helper_dirtyfb()
                                      drm_modeset_lock(&plane->mutex, ...)
                                      plane->state->fb  <-- NULL deref / UAF

If drm_sysfb_damage_work() runs while the device is suspended or resuming, it
can also blit to the hardware framebuffer while the display is suspended.

[Severity: Medium]
Does holding drm_dev_enter() across drm_atomic_helper_dirtyfb() invert the
locking order between drm_unplug_srcu and modeset, dma_resv, and dma_fence
locks?

drm_dev_enter() acquires a read lock on the global drm_unplug_srcu, and
drm_atomic_helper_dirtyfb() then acquires modeset locks, takes dma_resv_lock()
in drm_gem_fb_vmap(), and waits on GPU fences in
drm_atomic_helper_wait_for_fences().  Normal atomic commits acquire modeset
locks, dma_resv_lock(), and dma_fence_map before calling drm_dev_enter() in
drm_sysfb_plane_helper_atomic_update():

CPU0 (kworker)                    CPU1
----                              ----
lock(drm_unplug_srcu)
                                  lock(&plane->mutex)
lock(&plane->mutex)
                                  lock(drm_unplug_srcu)

 *** DEADLOCK ***

CPU0: drm_sysfb_damage_work() -> drm_atomic_helper_dirtyfb()
CPU1: drm_mode_atomic_ioctl() -> ... -> drm_sysfb_plane_helper_atomic_update()

If the rendering GPU is unplugged or halted while drm_sysfb_damage_work() is
waiting on its fence, drm_dev_unplug() on the GPU will also block in
synchronize_srcu(&drm_unplug_srcu) before it can reach the fence teardown path
that aborts the in-flight fence.  Since drm_sysfb_plane_helper_atomic_update()
already guards the hardware framebuffer blit with drm_dev_enter(), the outer
drm_dev_enter() in drm_sysfb_damage_work() can be dropped.

[ ... ]

> +static int drm_sysfb_fb_dirty(struct drm_framebuffer *fb, struct drm_file *file,
> +			      unsigned int flags, unsigned int color,
> +			      struct drm_clip_rect *clips, unsigned int num_clips)
> +{
> +	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(fb->dev);
> +	struct drm_rect r;
> +	bool queue = false;
> +
> +	drm_sysfb_damage_bounds(fb, flags, clips, num_clips, &r);
> +
> +	spin_lock(&sysfb->damage_lock);
> +	/* Pending damage of another framebuffer is copied first. */\
> +	while (sysfb->damage_fb && sysfb->damage_fb != fb) {
> +		spin_unlock(&sysfb->damage_lock);
> +		flush_work(&sysfb->damage_work);
> +		spin_lock(&sysfb->damage_lock);
> +	}
> +	if (!sysfb->damage_fb) {
> +		drm_framebuffer_get(fb);
> +		sysfb->damage_fb = fb;
> +		sysfb->damage = r;
> +		queue = true;
> +	} else {
> +		sysfb->damage.x1 = min(sysfb->damage.x1, r.x1);
> +		sysfb->damage.y1 = min(sysfb->damage.y1, r.y1);
> +		sysfb->damage.x2 = max(sysfb->damage.x2, r.x2);
> +		sysfb->damage.y2 = max(sysfb->damage.y2, r.y2);
> +	}
> +	spin_unlock(&sysfb->damage_lock);
> +
> +	if (queue)
> +		queue_work(system_dfl_wq, &sysfb->damage_work);
> +	return 0;
> +}

[Severity: High]
Can a second caller with a different framebuffer busy-spin or livelock in the
flush_work() loop when sysfb->damage_fb is set before queue_work() is called?

sysfb->damage_fb is published under sysfb->damage_lock, but queue_work() is
called only after dropping the lock.  If another thread calls
drm_sysfb_fb_dirty() for a different framebuffer in that window, flush_work()
sees that sysfb->damage_work is not queued and returns false immediately:

CPU0                              CPU1
drm_sysfb_fb_dirty(fb1)
  spin_lock(&sysfb->damage_lock)
  sysfb->damage_fb = fb1
  queue = true
  spin_unlock(&sysfb->damage_lock)
  <preempted>
                                  drm_sysfb_fb_dirty(fb2)
                                    spin_lock(&sysfb->damage_lock)
                                    while (sysfb->damage_fb != fb2)
                                      spin_unlock(&sysfb->damage_lock)
                                      flush_work(&sysfb->damage_work)
                                      spin_lock(&sysfb->damage_lock)
                                      <-- spins forever if RT thread on same CPU
  queue_work(..., &sysfb->damage_work)

In addition, because drm_sysfb_damage_work() clears sysfb->damage_fb to NULL
before calling drm_atomic_helper_dirtyfb(), further DIRTYFB calls on fb1 while
the worker is waiting on fences will re-populate sysfb->damage_fb with fb1
before flush_work() returns, forcing the waiter for fb2 to stall across
multiple fence-wait cycles.  Calling queue_work() while holding
sysfb->damage_lock fixes the unqueued flush_work() race.

[Severity: Medium]
Does returning immediately from drm_sysfb_fb_dirty() allow userspace or the
GPU to overwrite the frontbuffer while drm_sysfb_damage_work() is copying from
it, and stall non-blocking atomic commits on modeset locks?

When userspace renders into a mmap'd dumb buffer with the CPU (where no
dma_fence is attached) or submits the next frame's GPU job to the same
frontbuffer as soon as the previous fence signals, drm_sysfb_damage_work()
reads from the framebuffer concurrently with the new frame's writes, copying
unfinished frames to the screen and removing DIRTYFB upload rate-limiting.
Furthermore, drm_sysfb_damage_work() runs a blocking commit via
drm_atomic_helper_dirtyfb() while holding plane->mutex and crtc->mutex across
drm_atomic_helper_wait_for_fences(), which blocks concurrent
DRM_MODE_ATOMIC_NONBLOCK commits behind the GPU fence wait.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-sysfb-dirtyfb-v1-1-8571d455ccbe@telaak.dev?part=1