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

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

New issues:
- [High] Concurrent execution of workqueue and virtio_gpu_reclaim_vbufs causes data races and potential use-after-free.

Pre-existing issues:
- [High] Tasks waiting on vgdev->resp_wq hang forever because stranded vbufs are not responded to.
- [High] Incorrect flush order in virtio_gpu_deinit() can cause vgdev->obj_free_work to execute after driver unload.
- [High] Fences associated with pending commands are never signaled or removed from the fence_drv.fences list on teardown.
- [High] OOM during fence allocation or SG table mapping leaves BOs locked indefinitely and leaks command buffers.
--

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

drm/virtio: reclaim pending vbufs before tearing down vqs

virtio_gpu_free_vbufs() destroys the vbufs kmem_cache after the virtqueues
have already been released. Commands that were queued but never completed
by the device leave their vbuffers stranded in the virtqueue, so the cache
still holds live objects when virtio_gpu_deinit() tears everything down.

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

> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -292,10 +292,11 @@ void virtio_gpu_deinit(struct drm_device *dev)
>  {
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>  
>  	flush_work(&vgdev->obj_free_work);
>  	flush_work(&vgdev->ctrlq.dequeue_work);

[Severity: High]
This is a pre-existing issue, but does this flush order allow
obj_free_work to execute after driver unload?

If dequeue_work runs and processes command completions, it calls
virtio_gpu_array_put_free_delayed() which can schedule new obj_free_work.

Since obj_free_work is flushed first, any new work queued during the
dequeue_work flush will never be waited upon. This delayed work can then
execute after the device is freed, potentially causing a use-after-free on
vgdev->obj_free_lock.

>  	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 this lead to concurrent execution of dequeue_work and
virtio_gpu_reclaim_vbufs()?

Because flush_work() is called before virtio_reset_device(), if a device
interrupt fires in the window after the flush but before the reset,
dequeue_work could be rescheduled. 

virtio_gpu_reclaim_vbufs() then executes and locklessly calls
virtqueue_detach_unused_buf(). If dequeue_work executes concurrently, it
takes vgdev->ctrlq.qlock and operates on the same virtqueue. This might
cause a data race corrupting the virtqueue or a use-after-free if it
accesses it after del_vqs().

>  	vgdev->vdev->config->del_vqs(vgdev->vdev);
>  }

> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
[ ... ]
> +void virtio_gpu_reclaim_vbufs(struct virtio_gpu_device *vgdev)
> +{
> +	struct virtio_gpu_vbuffer *vbuf;
> +
> +	while ((vbuf = virtqueue_detach_unused_buf(vgdev->ctrlq.vq))) {
> +		if (vbuf->objs)
> +			virtio_gpu_array_put_free(vbuf->objs);
> +		if (vbuf->resp_cb_data)
> +			virtio_gpu_cleanup_object(vbuf->resp_cb_data);
> +		free_vbuf(vgdev, vbuf);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but could tasks waiting on
vgdev->resp_wq hang forever because stranded vbufs are not responded to?

The loop manually cleans up objs and resp_cb_data but does not invoke
vbuf->resp_cb or wake up vgdev->resp_wq.

For example, in virtgpu_virtio_get_uuid():

    wait_event(vgdev->resp_wq, bo->uuid_state != STATE_INITIALIZING);

A task waiting here will never be woken up if the device is removed and
the stranded UUID command is reclaimed without calling the response callback.

[Severity: High]
This is also a pre-existing issue, but are fences associated with pending
commands left unsignaled and leaked on teardown?

When commands are queued, virtio_gpu_fence_emit() adds the fence:

    list_add_tail(&fence->node, &drv->fences);

This reclaim loop frees the command buffers but completely ignores the fences.
Does this leave the fences permanently in the driver's list, causing
memory leaks and indefinite hangs for any userspace tasks waiting on them?

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

> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
[ ... ]
>  		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]
This is a pre-existing issue, but can an OOM during fence allocation leave
buffer objects locked indefinitely?

If virtio_gpu_fence_alloc() fails and returns NULL, it proceeds without
checking the error. If drm_fence_event is true, it passes the NULL out_fence
to virtio_gpu_fence_event_create(), causing a NULL pointer dereference.

Otherwise, it locks BOs but later in virtio_gpu_queue_ctrl_sgs():

    if (fence) {
        virtio_gpu_fence_emit(vgdev, virtio_gpu_vbuf_ctrl_hdr(vbuf),
                              fence);
        if (vbuf->objs) {
            virtio_gpu_array_add_fence(vbuf->objs, &fence->f);
            virtio_gpu_array_unlock_resv(vbuf->objs);
        }
    }

Because the fence allocation failed and fence is NULL, the BOs are never
unlocked, causing a permanent deadlock on the next access.

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