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

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 <[email protected]>

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/[email protected]?part=1