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

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

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