Hello, On 5/10/26 08:30, Deepanshu Kartikey wrote: > virtio_gpu_cursor_plane_update() calls virtio_gpu_array_lock_resv() > but ignores its return value. The function can fail in two ways: > > - dma_resv_lock_interruptible() returns -ERESTARTSYS when a signal > is delivered while waiting for the reservation lock. > - dma_resv_reserve_fences() returns -ENOMEM if it fails to allocate > a fence slot; in this case lock_resv unlocks before returning. > > In both cases the resv lock is not held on return. The cursor path > proceeds to queue a fenced transfer command. The queue path then > walks the object array and calls dma_resv_add_fence() on the cursor > BO's reservation. dma_resv_add_fence() requires the resv lock to be > held; with lockdep enabled the missing lock trips > dma_resv_assert_held(): > > WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840 > Call Trace: > virtio_gpu_array_add_fence+0xcd/0x140 > virtio_gpu_queue_ctrl_sgs > virtio_gpu_queue_fenced_ctrl_buffer+0x578/0xfb0 > virtio_gpu_cursor_plane_update+0x411/0xbc0 > drm_atomic_helper_commit_planes+0x497/0xf10 > ... > drm_mode_cursor_ioctl+0xd4/0x110 > drm_ioctl+0x5e6/0xc60 > __x64_sys_ioctl+0x18e/0x210 > > Beyond the WARN, mutating the dma_resv fence list without the lock > races with concurrent readers/writers and can corrupt the list. > > Check the return value of virtio_gpu_array_lock_resv(). On failure, > drop the references taken by virtio_gpu_array_add_obj() with > virtio_gpu_array_put_free() (which does not unlock, matching the > not-locked state) and return without queueing the command. A > skipped cursor frame is harmless; the WARN and the underlying race > are not. > > The bug was reported by syzbot, triggered via fault injection > (fail_nth) on the DRM_IOCTL_MODE_CURSOR path, which forces the > -ENOMEM branch in dma_resv_reserve_fences(). > > Reported-by: [email protected] > Closes: https://syzkaller.appspot.com/bug?extid=72bd3dd3a5d5f39a0271 > Fixes: 5cfd31c5b3a3 ("drm/virtio: fix virtio_gpu_cursor_plane_update().") > Cc: [email protected] > Signed-off-by: Deepanshu Kartikey <[email protected]> > --- > drivers/gpu/drm/virtio/virtgpu_plane.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c > b/drivers/gpu/drm/virtio/virtgpu_plane.c > index a126d1b25f46..ca379b08b9ec 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c > @@ -459,7 +459,10 @@ static void virtio_gpu_cursor_plane_update(struct > drm_plane *plane, > if (!objs) > return; > virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]); > - virtio_gpu_array_lock_resv(objs); > + if (virtio_gpu_array_lock_resv(objs)) { > + virtio_gpu_array_put_free(objs); > + return; > + } > virtio_gpu_cmd_transfer_to_host_2d > (vgdev, 0, > plane->state->crtc_w,
Thanks for the patch. Atomic update shouldn't fail due to non-critical errors like on a signal interrupt. Could you please move this code that may fail in update() to .prepare/cleanup_fb() callbacks? -- Best regards, Dmitry
