[PATCH 0/2] drm/virtio: fix control-buffer leaks on queueing errors

[PATCH 0/2] drm/virtio: fix control-buffer leaks on queueing errors

[Yao Kai]

Fix two independent cleanup omissions for control buffers that do not
reach the virtqueue.

Patch 1 fixes the reproduced GEM/VMA leak when fbdev damage work races
with device unplug.

Patch 2 fixes the vmalloc_to_sgt() allocation-failure path, found by
code inspection.

Testing:
- Repeated virtio-gpu unbind/rebind no longer triggers the
  drm_mm_takedown warning.
- Built drivers/gpu/drm/virtio/virtgpu_vq.o with W=1.

Yao Kai (2):
  drm/virtio: release object array when device is unplugged
  drm/virtio: free control buffer when scatterlist allocation fails

 drivers/gpu/drm/virtio/virtgpu_vq.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 8ba098e6b6ff0db8edf28528d1552be261af30d4
-- 
2.43.0

[PATCH 1/2] drm/virtio: release object array when device is unplugged

[Yao Kai]

Unbinding virtio-gpu while fbdev damage work is active can leave the VMA
offset manager non-empty when the DRM device is released:

  Console: switching to colour VGA+ 80x25
  ------------[ cut here ]------------
  Memory manager not clean during takedown.
  WARNING: drivers/gpu/drm/drm_mm.c:965 at drm_mm_takedown+0x28/0xc0
  Call Trace:
   drm_managed_release+0x95/0x130
   drm_dev_put+0x54/0x70
   virtio_dev_remove+0x3c/0x90
   device_release_driver_internal+0x19a/0x200
   unbind_store+0x9c/0xb0

The remaining node belongs to the fbdev GEM object:

  [drm:drm_mm_takedown] *ERROR* node [00100000 + 00000300]:
   drm_mm_insert_node_in_range+0x2bf/0x530
   drm_vma_offset_add+0x58/0x60
   __drm_gem_shmem_create+0x71/0x150
   virtio_gpu_object_create+0x41/0x3a0
   virtio_gpu_mode_dumb_create+0xdc/0x190
   drm_client_buffer_create_dumb+0x75/0xf0
   drm_fbdev_shmem_driver_fbdev_probe+0x8b/0x230
   __drm_fb_helper_initial_config_and_unlock+0x359/0x600
   drm_fbdev_client_hotplug+0x61/0xb0
   drm_client_register+0x5c/0x90
   drm_fbdev_client_setup+0xd6/0x1d0
   drm_client_setup+0x81/0xb0
   virtio_gpu_probe+0xd6/0x180

fbdev damage work can race with drm_dev_unplug(). It may build
vbuf->objs and take GEM references after the device has been marked
unplugged. drm_dev_enter() then fails, and the existing failure path
unlocks fenced reservations and frees the vbuffer without releasing the
object array. The unreachable array keeps the GEM object, and therefore
its VMA offset node, alive through device release.

Release the object array before freeing a control buffer rejected by
drm_dev_enter().

Fixes: b1df3a2b24a9 ("drm/virtio: add drm_driver.release callback.")
Cc: stable@vger.kernel.org
Reported-by: Ji Fa <jifa@huawei.com>
Signed-off-by: Yao Kai <yaokai34@huawei.com>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index e5e1af8b8e8a..a470339b6747 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -383,6 +383,7 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
 	if (!drm_dev_enter(vgdev->ddev, &idx)) {
 		if (fence && vbuf->objs)
 			virtio_gpu_array_unlock_resv(vbuf->objs);
+		virtio_gpu_array_put_free(vbuf->objs);
 		free_vbuf(vgdev, vbuf);
 		return -ENODEV;
 	}
-- 
2.43.0

Re: [PATCH 1/2] drm/virtio: release object array when device is unplugged

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] When `vmalloc_to_sgt()` fails in `virtio_gpu_queue_fenced_ctrl_buffer()`, it returns `-ENOMEM` but leaks `vbuf`, `vbuf->data_buf`, `vbuf->objs`, and leaves the `fence` unsignaled.
- [High] If `virtqueue_add_sgs()` fails in `virtio_gpu_queue_ctrl_sgs()`, the error is ignored, leaving the driver in an inconsistent state and permanently leaking resources.
- [High] Missing NULL check for `virtio_gpu_fence_alloc()` in `virtio_gpu_init_submit()` leads to NULL pointer dereference or permanent GEM object locking.
- [High] Unsignaled `dma_fence` on the `drm_dev_enter` error path causes userspace deadlock.
--

--- Patch [1]: [PATCH 1/2] drm/virtio: release object array when device is unplugged ---
commit 4de920e41f6ad59b54950dfcf4081b64911d0f82
Author: Yao Kai <yaokai34@huawei.com>

drm/virtio: release object array when device is unplugged

This commit addresses a memory manager warning that occurs when unbinding
virtio-gpu while fbdev damage work is active. It ensures the object array
is released before freeing a control buffer rejected by drm_dev_enter(),
preventing the GEM object from remaining alive during device release.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index e5e1af8b8e8a0..a470339b67474 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -383,6 +383,7 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
>  	if (!drm_dev_enter(vgdev->ddev, &idx)) {
>  		if (fence && vbuf->objs)
>  			virtio_gpu_array_unlock_resv(vbuf->objs);
> +		virtio_gpu_array_put_free(vbuf->objs);
>  		free_vbuf(vgdev, vbuf);
>  		return -ENODEV;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but returning -ENODEV here
without signaling the fence causes issues. Since callers like
virtio_gpu_cmd_submit() return void and ignore this error, the ioctl
completes successfully and returns a sync_file fd to userspace.

Because the fence is never added to the driver's list or signaled, does
this cause userspace to hang forever waiting on it? Should we signal the
fence with an error before returning?

This is a pre-existing issue, but further down in virtio_gpu_queue_ctrl_sgs()
there is an unchecked error:

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_queue_ctrl_sgs() {
[ ... ]
	ret = virtqueue_add_sgs(vq, sgs, outcnt, incnt, vbuf, GFP_ATOMIC);
	WARN_ON(ret);

	vbuf->seqno = ++vgdev->ctrlq.seqno;
[ ... ]
}

[Severity: High]
If virtqueue_add_sgs() fails (for example, due to -ENOMEM during GFP_ATOMIC
allocation), the error is warned upon but otherwise ignored. The function
proceeds to increment the sequence number and pending_commands.

Since the command is never placed in the virtqueue, does this leave the
driver in an inconsistent state and permanently leak the vbuf and GEM object
references?

This is also a pre-existing issue, but I noticed a similar problem in
virtio_gpu_queue_fenced_ctrl_buffer():

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_queue_fenced_ctrl_buffer() {
[ ... ]
			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);
				return -ENOMEM;
			}
[ ... ]
}

[Severity: High]
When vmalloc_to_sgt() fails, it returns -ENOMEM. Does this leak vbuf,
vbuf->data_buf, and vbuf->objs since free_vbuf() and
virtio_gpu_array_put_free() are not called?

Additionally, since the caller virtio_gpu_cmd_submit() ignores the return
value, the fence remains unsignaled. Can this cause userspace processes
waiting on the fence to deadlock?

Lastly, this is a pre-existing issue, but looking at virtio_gpu_init_submit():

drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_init_submit() {
[ ... ]
	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
	    exbuf->num_out_syncobjs ||
	    exbuf->num_bo_handles ||
	    drm_fence_event)
		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
	else
		out_fence = NULL;

	if (drm_fence_event) {
		err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
[ ... ]
}

[Severity: High]
virtio_gpu_fence_alloc() can return NULL under memory pressure. If
drm_fence_event is true, out_fence is passed to
virtio_gpu_fence_event_create() which unconditionally dereferences it:

drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_fence_event_create() {
[ ... ]
	ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
	if (ret) {
		kfree(e);
		return ret;
	}

	fence->e = e;
[ ... ]
}

Does this cause a NULL pointer dereference?

Alternatively, if drm_fence_event is false but num_bo_handles > 0, the code
proceeds with a NULL out_fence. When submitted, virtio_gpu_queue_ctrl_sgs()
skips unlocking vbuf->objs because fence is NULL, which appears to leave the
GEM objects locked indefinitely. Should virtio_gpu_init_submit() check for
allocation failure here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731092236.2262403-1-yaokai34@huawei.com?part=1

[PATCH 2/2] drm/virtio: free control buffer when scatterlist allocation fails

[Yao Kai]

A vmalloc_to_sgt() failure leaks the control vbuffer, its data buffer,
and any GEM references held by vbuf->objs.

The queueing code owns the vbuffer by this point, but the failure path
only unlocks fenced reservations before returning -ENOMEM. No caller
reclaims the unqueued buffer.

Drop the object-array references and free the vbuffer before returning.

Fixes: e1218b8c0cc1 ("drm/virtio: Use vmalloc for command buffer allocations.")
Cc: stable@vger.kernel.org
Signed-off-by: Yao Kai <yaokai34@huawei.com>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index a470339b6747..7ccc487e3332 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -477,6 +477,8 @@ static int virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
 			if (!sgt) {
 				if (fence && vbuf->objs)
 					virtio_gpu_array_unlock_resv(vbuf->objs);
+				virtio_gpu_array_put_free(vbuf->objs);
+				free_vbuf(vgdev, vbuf);
 				return -ENOMEM;
 			}
 
-- 
2.43.0

Re: [PATCH 2/2] drm/virtio: free control buffer when scatterlist allocation fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Command submission functions swallow the return value of queueing functions, causing silent failures that lead to infinite hangs or WARN_ON triggers.
- [High] Failure to check the return value of `virtio_gpu_fence_alloc()` leads to a NULL pointer dereference or permanently leaked `dma_resv` locks on GEM objects.
--

--- Patch [2]: [PATCH 2/2] drm/virtio: free control buffer when scatterlist allocation fails ---
commit 3738e83e4c6bee348b73dcd2ccca201708bd9797
Author: Yao Kai <yaokai34@huawei.com>

drm/virtio: free control buffer when scatterlist allocation fails

This commit addresses a memory leak that occurs when vmalloc_to_sgt() fails
in virtio_gpu_queue_fenced_ctrl_buffer(). It correctly drops object array
references and frees the vbuffer on the error path before returning -ENOMEM.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index a470339b67474..7ccc487e33322 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -477,6 +477,8 @@ static int virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
>  			if (!sgt) {
>  				if (fence && vbuf->objs)
>  					virtio_gpu_array_unlock_resv(vbuf->objs);

[Severity: High]
This is a pre-existing issue, but does checking the fence before unlocking
leave the GEM objects permanently locked if the fence is NULL?

If virtio_gpu_init_submit() fails to allocate a fence via
virtio_gpu_fence_alloc(), it could still lock the GEM objects and pass
fence = NULL into the queuing functions, which means we skip unlocking them
here.

Additionally, missing the NULL check on the fence returned by
virtio_gpu_fence_alloc() can lead to a NULL pointer dereference in
virtio_gpu_fence_event_create():

drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_init_submit() {
	...
	if (out_fence_fd)
		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
	...
	if (drm_fence_event) {
		err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
	...
}

drivers/gpu/drm/virtio/virtgpu_fence.c:virtio_gpu_fence_event_create() {
	...
	fence->e = e;
	...
}

Would it be better to decouple the unlocking of vbuf->objs from the presence
of a fence?

> +				virtio_gpu_array_put_free(vbuf->objs);
> +				free_vbuf(vgdev, vbuf);
>  				return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but because this function returns
an error here, are there callers that swallow this error?

For example, virtio_gpu_cmd_submit() ignores the return value:

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_cmd_submit() {
	...
	virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence);
}

Similarly, virtio_gpu_cmd_map() ignores the return value of
virtio_gpu_queue_ctrl_buffer():

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_cmd_map() {
	...
	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
	return 0;
}

If the queueing functions fail and return -ENOMEM, the callers return void
or 0, causing higher-level ioctls to proceed as if successful.

Can this cause infinite hangs? If the command is dropped,
virtio_gpu_vram_mmap() might hang indefinitely waiting for a response:

drivers/gpu/drm/virtio/virtgpu_vram.c:virtio_gpu_vram_mmap() {
	...
	wait_event(vgdev->resp_wq, vram->map_state != STATE_INITIALIZING);
}

Also, VIRTGPU_EXECBUFFER might return a sync_file with an unsignaled fence,
which could cause a userspace poll to hang indefinitely.

>  			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731092236.2262403-1-yaokai34@huawei.com?part=2