From: Nguyen Ngoc Thang <[email protected]>
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: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=d93b9064fe5f74af3aa3
Link:
https://lore.kernel.org/all/[email protected]/
Signed-off-by: Nguyen Ngoc Thang <[email protected]>
[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 <[email protected]>
---
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/[email protected]/
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/[email protected]/
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
