Re: [PATCH AUTOSEL 6.18-6.1] drm/virtio: reclaim pending vbufs before tearing down vqs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/virtio: missing pending_commands reset in virtio_gpu_reclaim_vbufs()
- [High] drm/virtio: race in virtio_gpu_deinit() flushing work before device reset
- [High] drm/virtio: uninterruptible hang in virtio_gpu_reclaim_vbufs() on imported dmabuf
--

commit f24f642758ec3795a40ce21fd3080771f4fdffb2
Author: Anuj Bolewar <bolewara@gmail.com>

drm/virtio: reclaim pending vbufs before tearing down vqs

This commit drains unused vbuffers from the control and cursor virtqueues in
virtio_gpu_deinit() after resetting the device and before deleting the
virtqueues, releasing any held GEM object references and pending
RESOURCE_UNREF objects before the vbufs kmem_cache is destroyed.

Link: https://patch.msgid.link/20260802-virtio-gpu-reclaim-vbufs-v2-1-5767fb860691@gmail.com

[ ... ]

> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 1c15cbf326b78..59173746c9494 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -286,6 +286,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);
>  	vgdev->vdev->config->del_vqs(vgdev->vdev);
>  }

[Severity: High]
Could flushing the workqueues before virtio_reset_device() in
virtio_gpu_deinit() race with virtio_gpu_reclaim_vbufs() and del_vqs()?

In drivers/gpu/drm/virtio/virtgpu_kms.c:virtio_gpu_deinit():

	flush_work(&vgdev->obj_free_work);
	flush_work(&vgdev->ctrlq.dequeue_work);
	flush_work(&vgdev->cursorq.dequeue_work);
	flush_work(&vgdev->config_changed_work);
	virtio_reset_device(vgdev->vdev);
	virtio_gpu_reclaim_vbufs(vgdev);
	vgdev->vdev->config->del_vqs(vgdev->vdev);

Until virtio_reset_device(vgdev->vdev) completes, the virtio device can still
raise virtqueue and config interrupts (virtio_gpu_ctrl_ack(),
virtio_gpu_cursor_ack(), and virtio_gpu_config_changed()), which call
schedule_work() and can re-queue ctrlq.dequeue_work, cursorq.dequeue_work, and
config_changed_work after flush_work() has already returned. In the
virtio_gpu_probe() -> err_deinit path, drm_dev_unplug() is not called, so
config_changed_work can also queue new buffers into ctrlq.vq.

Since virtio_reset_device() does not cancel or flush already-scheduled work,
virtio_gpu_dequeue_ctrl_func() and virtio_gpu_dequeue_cursor_func() can run
concurrently with virtio_gpu_reclaim_vbufs() and del_vqs():

	spin_lock(&vgdev->ctrlq.qlock);
	do {
		virtqueue_disable_cb(vgdev->ctrlq.vq);
		reclaim_vbufs(vgdev->ctrlq.vq, &reclaim_list);

	} while (!virtqueue_enable_cb(vgdev->ctrlq.vq));
	spin_unlock(&vgdev->ctrlq.qlock);

Because virtio_gpu_reclaim_vbufs() calls virtqueue_detach_unused_buf() without
holding ctrlq.qlock or cursorq.qlock while dequeue_work is calling
virtqueue_get_buf(), can this corrupt virtqueue descriptor state (such as
vq->split.desc_state[i].data, vq->free_head, and vq->vq.num_free), double-free
vbuffers, or access the virtqueues while del_vqs() is freeing them?

Additionally, flush_work(&vgdev->obj_free_work) runs before
flush_work(&vgdev->ctrlq.dequeue_work), even though
virtio_gpu_dequeue_ctrl_func() schedules vgdev->obj_free_work via
virtio_gpu_array_put_free_delayed(). Can virtio_gpu_array_put_free_work() end
up running concurrently with or after virtio_gpu_release() ->
virtio_gpu_free_vbufs() and touch vgdev->vbufs while or after the kmem_cache
is destroyed?

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 8181b22b9b46a..68cf7e4996884 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -207,6 +207,21 @@ 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))) {
> +		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);
> +	}
> +	while ((vbuf = virtqueue_detach_unused_buf(vgdev->cursorq.vq)))
> +		free_vbuf(vgdev, vbuf);
> +}

[Severity: High]
Should virtio_gpu_reclaim_vbufs() also reset vgdev->pending_commands to avoid
a use-after-free on vgdev->ctrlq.vq in virtio_gpu_notify()?

Whenever a control buffer is queued via virtio_gpu_queue_ctrl_sgs() (or
virtio_gpu_panic_queue_ctrl_sgs()), vgdev->pending_commands is incremented:

	ret = virtqueue_add_sgs(vq, sgs, outcnt, incnt, vbuf, GFP_ATOMIC);
	WARN_ON(ret);

	vbuf->seqno = ++vgdev->ctrlq.seqno;
	trace_virtio_gpu_cmd_queue(vq, virtio_gpu_vbuf_ctrl_hdr(vbuf), vbuf->seqno);

	atomic_inc(&vgdev->pending_commands);

Several command submission paths (such as virtio_gpu_object_create(),
virtio_gpu_vram_create(), virtio_gpu_mode_dumb_create(),
virtio_gpu_resource_create_ioctl(), virtio_gpu_resource_create_blob_ioctl(),
virtio_gpu_resource_assign_uuid(), virtio_gpu_prepare_imported_obj(), and
virtio_gpu_create_context()) queue vbuffers into vgdev->ctrlq.vq without
immediately calling virtio_gpu_notify(vgdev), leaving vgdev->pending_commands
non-zero.

When virtio_gpu_reclaim_vbufs() detaches those vbuffers, it does not reset or
decrement vgdev->pending_commands, and virtio_gpu_deinit() immediately frees
vgdev->ctrlq.vq via vgdev->vdev->config->del_vqs(vgdev->vdev).

If an open drm_file later releases a GEM object or closes (calling
virtio_gpu_gem_object_close(), virtio_gpu_free_object(),
virtio_gpu_vram_free(), virtgpu_dma_buf_free_obj(), or
virtio_gpu_driver_postclose() -> virtio_gpu_notify()), virtio_gpu_notify()
has no drm_dev_enter() guard and only checks vgdev->pending_commands:

void virtio_gpu_notify(struct virtio_gpu_device *vgdev)
{
	bool notify;

	if (!atomic_read(&vgdev->pending_commands))
		return;

	spin_lock(&vgdev->ctrlq.qlock);
	atomic_set(&vgdev->pending_commands, 0);
	notify = virtqueue_kick_prepare(vgdev->ctrlq.vq);
	spin_unlock(&vgdev->ctrlq.qlock);

	if (notify)
		virtqueue_notify(vgdev->ctrlq.vq);
}

Won't virtio_gpu_notify() see the stale non-zero vgdev->pending_commands and
call virtqueue_kick_prepare() and virtqueue_notify() on the freed
vgdev->ctrlq.vq? Also, if virtio_gpu_array_put_free(vbuf->objs) inside
virtio_gpu_reclaim_vbufs() drops the final reference to a created GEM object,
virtio_gpu_free_object() synchronously calls virtio_gpu_notify(vgdev), which
would kick vgdev->ctrlq.vq after virtio_reset_device() has already reset the
device while virtqueue_detach_unused_buf() is draining the ring.

[Severity: High]
Can calling virtio_gpu_array_put_free(vbuf->objs) from
virtio_gpu_reclaim_vbufs() hang uninterruptibly in dma_fence_wait() when
releasing an imported DMA-BUF GEM object?

When VIRTGPU_PARAM_RESOURCE_BLOB is enabled without virgl 3D
(vgdev->has_resource_blob && !vgdev->has_virgl_3d), importing a DMA-BUF via
virtgpu_gem_prime_import() creates a GEM object with
obj->funcs = &virtgpu_gem_dma_buf_funcs (.free = virtgpu_dma_buf_free_obj) and
bo->attached = true. If that object is used on the primary plane via
virtio_gpu_primary_plane_update() -> virtio_gpu_resource_flush() and the
50ms wait times out, the vbuf remains in ctrlq.vq holding a reference to the
imported GEM object in vbuf->objs.

During virtio_gpu_remove(), drm_dev_unplug(dev) marks the DRM device
unplugged, drm_atomic_helper_shutdown(dev) drops the plane state's framebuffer
reference, and virtio_gpu_deinit() resets the device and calls
virtio_gpu_reclaim_vbufs(). When virtio_gpu_array_put_free(vbuf->objs) drops
the last reference to the imported object, it triggers
virtgpu_dma_buf_free_obj() -> virtgpu_dma_buf_unmap() ->
virtio_gpu_detach_object_fenced(bo):

int virtio_gpu_detach_object_fenced(struct virtio_gpu_object *bo)
{
	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
	struct virtio_gpu_fence *fence;

	if (!bo->attached)
		return 0;

	fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
	if (!fence)
		return -ENOMEM;

	virtio_gpu_object_detach(vgdev, bo, fence);
	virtio_gpu_notify(vgdev);

	dma_fence_wait(&fence->f, false);
	dma_fence_put(&fence->f);

	return 0;
}

Inside virtio_gpu_object_detach() -> virtio_gpu_cmd_resource_detach_backing()
-> virtio_gpu_queue_fenced_ctrl_buffer() -> virtio_gpu_queue_ctrl_sgs(),
drm_dev_enter() returns false because the device is unplugged:

	if (!drm_dev_enter(vgdev->ddev, &idx)) {
		if (fence && vbuf->objs)
			virtio_gpu_array_unlock_resv(vbuf->objs);
		free_vbuf(vgdev, vbuf);
		return -ENODEV;
	}

Since virtio_gpu_queue_ctrl_sgs() returns -ENODEV before
virtio_gpu_fence_emit() is called, and virtio_gpu_object_detach() ignores this
error, won't dma_fence_wait(&fence->f, false) in
virtio_gpu_detach_object_fenced() sleep forever waiting on a fence that was
never emitted and can never be signaled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-179-sashal@kernel.org?part=1