Re: [PATCH] drm/virtio: reclaim pending vbufs before tearing down vqs

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

New issues:
- [High] Data race and potential Use-After-Free between `dequeue_work` and `virtio_gpu_reclaim_vbufs` / `del_vqs`.

Pre-existing issues:
- [High] Memory leak of GEM objects (bo) due to missing cleanup of `vbuf->resp_cb_data` in `virtio_gpu_reclaim_vbufs`.
- [High] Permanent leak of `dma_fence` objects and potential infinite wait for stranded commands.
--

--- Patch [1]: [PATCH] drm/virtio: reclaim pending vbufs before tearing down vqs ---
commit 9dfeaad280a2a9a1d77041d3cd3a7b47ba0d4e3a
Author: Anuj Bolewar <[email protected]>

drm/virtio: reclaim pending vbufs before tearing down vqs

This commit attempts to drain any buffers still sitting in the control and
cursor virtqueues in virtio_gpu_deinit() after the device has been reset
and before the virtqueues are deleted to prevent a cache shutdown warning.

Link: https://syzkaller.appspot.com/bug?extid=06f9b2a53ba4a5a47644

> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index b4329f28e9766..e5a6ae679f363 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -298,6 +298,7 @@ void virtio_gpu_deinit(struct drm_device *dev)
>  	flush_work(&vgdev->cursorq.dequeue_work);
>  	flush_work(&vgdev->config_changed_work);
>  	virtio_reset_device(vgdev->vdev);
> +	virtio_gpu_reclaim_vbufs(vgdev);

[Severity: High]
Could a late interrupt re-queue the work after it has been flushed?

Since flush_work() is called before virtio_reset_device(), if an interrupt
arrives just after flush_work() completes but before virtio_reset_device()
is called, the worker could be re-queued.

Because virtio_reset_device() stops new interrupts but doesn't cancel
pending asynchronous work, could the rescheduled worker execute concurrently
with the newly added virtio_gpu_reclaim_vbufs() and access the virtqueue?

>  	vgdev->vdev->config->del_vqs(vgdev->vdev);
>  }
>  
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index e5e1af8b8e8a0..95d7a9ef0d99d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c

[ ... ]

> @@ -208,6 +208,19 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
>  	kmem_cache_free(vgdev->vbufs, vbuf);
>  }
>  
> +void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev)
> +{
> +	struct virtio_gpu_vbuffer *vbuf;
> +
> +	while ((vbuf = virtqueue_detach_unused_buf(vgdev->ctrlq.vq))) {

[Severity: High]
If the dequeue_work worker is still running as mentioned above, could it
race with this lockless manipulation of the virtqueue?

The worker uses virtqueue_get_buf() under vgdev->ctrlq.qlock, but
virtqueue_detach_unused_buf() is called here without acquiring the lock.
This might lead to concurrent modification of the virtqueue internal state
or a use-after-free if the worker accesses it after the queues are deleted.

> +		if (vbuf->objs)
> +			virtio_gpu_array_put_free(vbuf->objs);

[Severity: High]
This isn't a bug introduced by this patch, but does this loop overlook
vbuf->resp_cb_data?

When the device is reset, pending VIRTIO_GPU_CMD_RESOURCE_UNREF commands are
left in the virtqueue. If they are detached and freed here without executing
their resp_cb callbacks (like virtio_gpu_cmd_unref_cb) or manually cleaning
up vbuf->resp_cb_data, does it permanently leak the underlying GEM objects
and their associated IDAs?

[Severity: High]
This is a pre-existing issue, but does this loop also silently discard
command buffers without signaling their associated fences?

When commands are queued with a fence via
virtio_gpu_queue_fenced_ctrl_buffer(), a reference to the dma_fence is added
to the vgdev->fence_drv.fences list. If virtio_gpu_reclaim_vbufs() detaches
and frees the vbufs without extracting or signaling these fences, could any
waiters hang indefinitely?

> +		free_vbuf(vgdev, vbuf);
> +	}
> +	while ((vbuf = virtqueue_detach_unused_buf(vgdev->cursorq.vq)))
> +		free_vbuf(vgdev, vbuf);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1