[PATCH] drm/virtio: put GEM objects when dropping a vbuf on unplug

[PATCH] drm/virtio: put GEM objects when dropping a vbuf on unplug

[Nguyen Ngoc Thang]

virtio_gpu_queue_ctrl_sgs() drops the command when drm_dev_enter() fails
or when vqs_released is set. It unlocks the reservation and frees the
vbuf, but never releases the references that the caller handed over in
vbuf->objs. Those references are normally dropped from the dequeue path,
which never sees a command that was not queued.

If a framebuffer flush races with device removal, the GEM object backing
the fbdev buffer keeps a stale reference, is never freed, and its node is
still in the vma offset manager when the drm_device is released:

  Memory manager not clean during takedown.
  WARNING: drivers/gpu/drm/drm_mm.c:965 at drm_mm_takedown
  Call Trace:
   drm_managed_release
   drm_dev_put
   virtio_dev_remove
   virtio_pci_remove
   pci_device_remove
   remove_store

Drop the object references together with the vbuf on both early-exit
paths.

Reported-by: syzbot+d93b9064fe5f74af3aa3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d93b9064fe5f74af3aa3
Fixes: b7170f9457f2 ("drm/virtio: return virtio_gpu_queue errors")
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index c02c03c10d92..b36c4da04b18 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -208,6 +208,17 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
 	kmem_cache_free(vgdev->vbufs, vbuf);
 }
 
+static void virtio_gpu_drop_vbuf(struct virtio_gpu_device *vgdev,
+				 struct virtio_gpu_vbuffer *vbuf, bool locked)
+{
+	if (vbuf->objs) {
+		if (locked)
+			virtio_gpu_array_unlock_resv(vbuf->objs);
+		virtio_gpu_array_put_free(vbuf->objs);
+	}
+	free_vbuf(vgdev, vbuf);
+}
+
 void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev)
 {
 	struct virtio_gpu_vbuffer *vbuf;
@@ -410,9 +421,7 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
 	int ret, idx;
 
 	if (!drm_dev_enter(vgdev->ddev, &idx)) {
-		if (fence && vbuf->objs)
-			virtio_gpu_array_unlock_resv(vbuf->objs);
-		free_vbuf(vgdev, vbuf);
+		virtio_gpu_drop_vbuf(vgdev, vbuf, fence);
 		return -ENODEV;
 	}
 
@@ -432,9 +441,7 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
 		 * synchronize_srcu() wait in drm_dev_unplug().
 		 */
 		if (vgdev->vqs_released) {
-			if (fence && vbuf->objs)
-				virtio_gpu_array_unlock_resv(vbuf->objs);
-			free_vbuf(vgdev, vbuf);
+			virtio_gpu_drop_vbuf(vgdev, vbuf, fence);
 			drm_dev_exit(idx);
 			return -ENODEV;
 		}
-- 
2.43.0

Re: [PATCH] drm/virtio: put GEM objects when dropping a vbuf on unplug

[sashiko-bot]

> virtio_gpu_queue_ctrl_sgs() drops the command when drm_dev_enter() fails
> or when vqs_released is set. It unlocks the reservation and frees the
> vbuf, but never releases the references that the caller handed over in
> vbuf->objs. Those references are normally dropped from the dequeue path,
> which never sees a command that was not queued.
> 
> If a framebuffer flush races with device removal, the GEM object backing
> the fbdev buffer keeps a stale reference, is never freed, and its node is
> still in the vma offset manager when the drm_device is released:
> 
>   Memory manager not clean during takedown.
>   WARNING: drivers/gpu/drm/drm_mm.c:965 at drm_mm_takedown
>   Call Trace:
>    drm_managed_release
>    drm_dev_put
>    virtio_dev_remove
>    virtio_pci_remove
>    pci_device_remove
>    remove_store
> 
> Drop the object references together with the vbuf on both early-exit
> paths.
> 
> Reported-by: syzbot+d93b9064fe5f74af3aa3@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d93b9064fe5f74af3aa3
> Fixes: b7170f9457f2 ("drm/virtio: return virtio_gpu_queue errors")
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927040655.14531-1-ngocthang2710.1999@gmail.com?part=1

[PATCH v2] drm/virtio: Put GEM objects when dropping a vbuf without queueing it

[Yu, Fei]

From: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

virtio_gpu_queue_ctrl_sgs() drops the command when drm_dev_enter()
fails or when vqs_released is set during device removal. It unlocks the
reservations and frees the vbuf, but never releases the GEM object
references that the caller transferred in vbuf->objs. Those references
are normally dropped by the dequeue worker once the host completes the
command, which never happens for a command that was not queued.

If a framebuffer flush or transfer (e.g. the fbdev damage worker) races
with device removal, the GEM object backing the buffer keeps a stale
reference and is never freed. GEM objects do not hold a reference on the
drm_device, so the device is still released, with the object's node left
in the vma offset manager:

  Memory manager not clean during takedown.
  WARNING: drivers/gpu/drm/drm_mm.c:965 at drm_mm_takedown+0x62/0x280
  Call Trace:
   drm_managed_release+0x1bb/0x540 drivers/gpu/drm/drm_managed.c:75
   drm_dev_release drivers/gpu/drm/drm_drv.c:924 [inline]
   drm_dev_put+0x1f/0x30 drivers/gpu/drm/drm_drv.c:957
   virtio_dev_remove+0xb8/0x170 drivers/virtio/virtio.c:375
   ...
   virtio_pci_remove+0x7c/0x110 drivers/virtio/virtio_pci_common.c:760
   ...
   remove_store+0x123/0x140 drivers/pci/pci-sysfs.c:524

The vmalloc_to_sgt() failure path in virtio_gpu_queue_fenced_ctrl_buffer()
has the same problem, and additionally leaks the vbuf itself.

Add a helper that releases a vbuf which was never added to the
virtqueue, including the object references it owns, and use it on all
three early-exit paths. None of the callers touches the object array
after handing it to the queue functions, so this cannot double-put.

Fixes: b1df3a2b24a9 ("drm/virtio: add drm_driver.release callback.")
Fixes: a46991b334f6 ("drm/virtio: abort virtqueue wait on device removal to avoid hung task")
Fixes: e1218b8c0cc1 ("drm/virtio: Use vmalloc for command buffer allocations.")
Reported-by: syzbot+d93b9064fe5f74af3aa3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d93b9064fe5f74af3aa3
Link: https://lore.kernel.org/all/20260927040655.14531-1-ngocthang2710.1999@gmail.com/
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
[fei.yu: also handle the vmalloc_to_sgt() failure path, pass the fence
 to the helper instead of a bool, fix the Fixes tags]
Signed-off-by: Fei Yu <fei.yu@intel.com>
---
Testing: syzbot C reproducer (PCI remove/rescan loop) for 120s in QEMU
with fbcon on virtio-gpu: v7.4-rc1 printed "Memory manager not clean
during takedown" in 377 of 441 cycles (kmemleak: 4040 objects); with
this patch, 487 cycles, no warning, no kmemleak report.

v2 (Fei Yu, Nguyen's v1 got no review for weeks):
 - also handle the vmalloc_to_sgt() failure path in
   virtio_gpu_queue_fenced_ctrl_buffer(), which leaked the vbuf as well
 - pass the fence to virtio_gpu_drop_vbuf() instead of a bool
 - replace the Fixes tag: b7170f9457f2 only changed the return type;
   point at the commits that added the three early-exit paths

v1: https://lore.kernel.org/all/20260927040655.14531-1-ngocthang2710.1999@gmail.com/

Note: Hui Peng's "drm/virtio: fix vbuf->objs double-free, fence NULL
deref, and sgt error path" [1] touches the same paths (by moving the put
into free_vbuf()) and conflicts with this patch.

[1] https://lore.kernel.org/all/20260919223440.3884176-1-benquike@gmail.com/

 drivers/gpu/drm/virtio/virtgpu_vq.c | 30 +++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index d99fb9e32..21271966b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -208,6 +208,25 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
 	kmem_cache_free(vgdev->vbufs, vbuf);
 }
 
+/*
+ * Free a vbuf that never made it into the virtqueue.  The caller handed
+ * over the references in vbuf->objs, which would otherwise have been
+ * dropped by the dequeue worker once the host completed the command, so
+ * release them here.  If a fence was passed, the reservations are still
+ * locked and must be unlocked first.
+ */
+static void virtio_gpu_drop_vbuf(struct virtio_gpu_device *vgdev,
+				 struct virtio_gpu_vbuffer *vbuf,
+				 struct virtio_gpu_fence *fence)
+{
+	if (vbuf->objs) {
+		if (fence)
+			virtio_gpu_array_unlock_resv(vbuf->objs);
+		virtio_gpu_array_put_free(vbuf->objs);
+	}
+	free_vbuf(vgdev, vbuf);
+}
+
 void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev)
 {
 	struct virtio_gpu_vbuffer *vbuf;
@@ -437,9 +456,7 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
 	int ret, idx;
 
 	if (!drm_dev_enter(vgdev->ddev, &idx)) {
-		if (fence && vbuf->objs)
-			virtio_gpu_array_unlock_resv(vbuf->objs);
-		free_vbuf(vgdev, vbuf);
+		virtio_gpu_drop_vbuf(vgdev, vbuf, fence);
 		return -ENODEV;
 	}
 
@@ -459,9 +476,7 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
 		 * synchronize_srcu() wait in drm_dev_unplug().
 		 */
 		if (vgdev->vqs_released) {
-			if (fence && vbuf->objs)
-				virtio_gpu_array_unlock_resv(vbuf->objs);
-			free_vbuf(vgdev, vbuf);
+			virtio_gpu_drop_vbuf(vgdev, vbuf, fence);
 			drm_dev_exit(idx);
 			return -ENODEV;
 		}
@@ -542,8 +557,7 @@ static int virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
 			sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size,
 					     &sg_ents);
 			if (!sgt) {
-				if (fence && vbuf->objs)
-					virtio_gpu_array_unlock_resv(vbuf->objs);
+				virtio_gpu_drop_vbuf(vgdev, vbuf, fence);
 				return -ENOMEM;
 			}
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
prerequisite-patch-id: aa41f7089456b709927a2d1d5480fdc2f0f2ec76
prerequisite-patch-id: 78c30e7ee8bc1e8d2c9e0626e8988a021d61b3bc
-- 
2.43.0

Re: [PATCH v2] drm/virtio: Put GEM objects when dropping a vbuf without queueing it

[sashiko-bot]

> From: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> 
> virtio_gpu_queue_ctrl_sgs() drops the command when drm_dev_enter()
> fails or when vqs_released is set during device removal. It unlocks the
> reservations and frees the vbuf, but never releases the GEM object
> references that the caller transferred in vbuf->objs. Those references
> are normally dropped by the dequeue worker once the host completes the
> command, which never happens for a command that was not queued.
> 
> If a framebuffer flush or transfer (e.g. the fbdev damage worker) races
> with device removal, the GEM object backing the buffer keeps a stale
> reference and is never freed. GEM objects do not hold a reference on the
> drm_device, so the device is still released, with the object's node left
> in the vma offset manager:
> 
> [ ... ]
> Link: https://lore.kernel.org/all/20260927040655.14531-1-ngocthang2710.1999@gmail.com/
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> [fei.yu: also handle the vmalloc_to_sgt() failure path, pass the fence
>  to the helper instead of a bool, fix the Fixes tags]
> Signed-off-by: Fei Yu <fei.yu@intel.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103085314.3102802-1-fei.yu@intel.com?part=1