Re: [PATCH AUTOSEL 6.18] drm/virtio: abort virtqueue wait on device removal to avoid hung task

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/virtio: un-emitted fence on abort in virtio_gpu_queue_ctrl_sgs()
- [Medium] drm/virtio: leak of vbuf->objs on abort in virtio_gpu_queue_ctrl_sgs()
- [Medium] drm/virtio: unreclaimed vbufs in virtio_gpu_deinit() on device removal
--

commit 81841bb63e995940f01c6fe877305de441ff0e80
Author: Ryosuke Yasuoka <ryasuoka@redhat.com>

drm/virtio: abort virtqueue wait on device removal to avoid hung task

This commit adds a vqs_released flag to struct virtio_gpu_device and wakes
up the control and cursor virtqueue wait queues during virtio_gpu_remove()
and virtio_gpu_shutdown() so that threads waiting for virtqueue space exit
their drm_dev_enter() critical sections and unblock drm_dev_unplug().

Link: https://patch.msgid.link/20260601-virtio-gpu_wait_event-v3-1-89530517a98a@redhat.com

> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index d9556e1b67b10..78cc388b434d9 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -118,10 +118,24 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
>  	return ret;
>  }
>  
> +/*
> + * Release pending virtqueue waits so the drm_dev_enter/exit() critical
> + * sections complete before drm_dev_unplug() blocks on synchronize_srcu().
> + */
> +static void virtio_gpu_release_vqs(struct drm_device *dev)
> +{
> +	struct virtio_gpu_device *vgdev = dev->dev_private;
> +
> +	vgdev->vqs_released = true;
> +	wake_up_all(&vgdev->ctrlq.ack_queue);
> +	wake_up_all(&vgdev->cursorq.ack_queue);
> +}
> +
>  static void virtio_gpu_remove(struct virtio_device *vdev)
>  {
>  	struct drm_device *dev = vdev->priv;
>  
> +	virtio_gpu_release_vqs(dev);
>  	drm_dev_unplug(dev);
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC))

[Severity: Medium]
When virtio_gpu_release_vqs() wakes a thread blocked on a full virtqueue,
the virtqueue itself still contains all of the previously queued
struct virtio_gpu_vbuffer entries that the host stopped processing.

Once drm_dev_unplug() unblocks, virtio_gpu_remove() proceeds to
virtio_gpu_deinit() and drm_dev_put() -> virtio_gpu_release():

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);
	vgdev->vdev->config->del_vqs(vgdev->vdev);
}

drivers/gpu/drm/virtio/virtgpu_kms.c:virtio_gpu_release() {
	...
	virtio_gpu_modeset_fini(vgdev);
	virtio_gpu_free_vbufs(vgdev);
	...
}

Because flush_work() on dequeue_work only reclaims buffers that the host
placed into the used ring via virtqueue_get_buf(), the unconsumed vbufs
left in ctrlq.vq and cursorq.vq are never detached via
virtqueue_detach_unused_buf() and freed before del_vqs() and
virtio_gpu_free_vbufs() -> kmem_cache_destroy(vgdev->vbufs).

Will this leak those in-flight vbufs (along with their resp_buf, data_buf,
and vbuf->objs references) and trigger a "Objects remaining on
__kmem_cache_shutdown()" warning in kmem_cache_destroy()?

[ ... ]

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 592ad4f9b100d..244a791ab915e 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -422,7 +422,19 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
>  	if (vq->num_free < elemcnt) {
>  		spin_unlock(&vgdev->ctrlq.qlock);
>  		virtio_gpu_notify(vgdev);
> -		wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
> +		wait_event(vgdev->ctrlq.ack_queue,
> +			   vq->num_free >= elemcnt || vgdev->vqs_released);
> +		/*
> +		 * Set by virtio_gpu_release_vqs() to unblock
> +		 * synchronize_srcu() wait in drm_dev_unplug().
> +		 */
> +		if (vgdev->vqs_released) {
> +			if (fence && vbuf->objs)
> +				virtio_gpu_array_unlock_resv(vbuf->objs);
> +			free_vbuf(vgdev, vbuf);
> +			drm_dev_exit(idx);
> +			return -ENODEV;
> +		}
>  		goto again;
>  	}

[Severity: Medium]
Does this abort path leak vbuf->objs and the GEM object references it
holds?

Callers that attach a struct virtio_gpu_object_array to vbuf->objs (such
as virtio_gpu_cmd_submit(), virtio_gpu_cmd_transfer_to_host_2d(),
virtio_gpu_cmd_transfer_to_host_3d(), virtio_gpu_cmd_resource_flush(), and
virtio_gpu_cmd_create_resource()) transfer ownership of objs to the vbuf
and ignore the return value of virtio_gpu_queue_fenced_ctrl_buffer() and
virtio_gpu_queue_ctrl_buffer().

On normal completion in virtio_gpu_dequeue_ctrl_func(), vbuf->objs is
released before free_vbuf():

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_dequeue_ctrl_func() {
	...
	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
		if (entry->objs)
			virtio_gpu_array_put_free_delayed(vgdev, entry->objs);
		list_del(&entry->list);
		free_vbuf(vgdev, entry);
	}
	...
}

However, free_vbuf() only frees vbuf->resp_buf, vbuf->data_buf, and vbuf:

drivers/gpu/drm/virtio/virtgpu_vq.c:free_vbuf() {
	if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
		kfree(vbuf->resp_buf);
	kvfree(vbuf->data_buf);
	kmem_cache_free(vgdev->vbufs, vbuf);
}

When vgdev->vqs_released is true, virtio_gpu_queue_ctrl_sgs() unlocks the
reservations on vbuf->objs when fenced, but never calls
virtio_gpu_array_put_free() or virtio_gpu_array_put_free_delayed() on
vbuf->objs.

[Severity: High]
What happens to callers that pass a non-NULL fence when
virtio_gpu_queue_ctrl_sgs() aborts here before calling
virtio_gpu_fence_emit()?

Because virtio_gpu_queue_fenced_ctrl_buffer() and its callers (such as
virtio_gpu_cmd_resource_detach_backing(),
virtio_gpu_cmd_transfer_to_host_2d(), and virtio_gpu_cmd_submit()) have
void return types and drop the -ENODEV error, the caller is not aware that
virtio_gpu_fence_emit() was skipped, leaving fence->f.seqno == 0 and the
fence unqueued in vgdev->fence_drv.fences.

In virtio_gpu_detach_object_fenced(), the caller immediately waits
uninterruptibly on the fence:

drivers/gpu/drm/virtio/virtgpu_object.c:virtio_gpu_detach_object_fenced() {
	...
	virtio_gpu_object_detach(vgdev, bo, fence);
	virtio_gpu_notify(vgdev);

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

Similarly, virtio_gpu_cursor_plane_update() calls
dma_fence_wait(&vgplane_st->fence->f, true) right after
virtio_gpu_cmd_transfer_to_host_2d(). Since the fence was never emitted
and will never be signaled, won't dma_fence_wait() in
virtio_gpu_detach_object_fenced() sleep forever in TASK_UNINTERRUPTIBLE
and cause another hung task?

In addition, in virtio_gpu_execbuffer_ioctl():

drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_execbuffer_ioctl() {
	...
	virtio_gpu_submit(&submit);

	virtio_gpu_install_out_fence_fd(&submit);
	virtio_gpu_process_post_deps(&submit);
	virtio_gpu_complete_submit(&submit);
cleanup:
	if (ret && submit.out_fence && submit.out_fence->e) {
		drm_event_cancel_free(dev, &submit.out_fence->e->base);
		submit.out_fence->e = NULL;
	}

	virtio_gpu_cleanup_submit(&submit);
	return ret;
}

Because virtio_gpu_submit() does not propagate the abort error to ret:
1. submit.out_fence (with f.seqno == 0) is exported to userspace sync_file
   or syncobjs, which triggers WARN_ON_ONCE(f->seqno == 0) in
   virtio_gpu_fence_signaled() when queried.
2. drm_event_cancel_free() at cleanup is skipped because ret is 0, and
   because submit.out_fence is not in vgdev->fence_drv.fences,
   submit.out_fence->e is never sent or freed.
3. virtio_gpu_complete_submit() clears submit.buflist = NULL, so
   virtio_gpu_cleanup_submit() skips freeing submit.buflist (vbuf->objs).

[ ... ]

>
>
>

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