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

[Ryosuke Yasuoka]

Hi Dmitry,
Thank you for the detailed review and follow-up.

On 28/05/2026 07:18, Dmitry Osipenko wrote:
> On 5/26/26 13:24, Dmitry Osipenko wrote:
>> On 5/22/26 11:51, Ryosuke Yasuoka wrote:
>>> Hi Dmitry,
>>> Thank you for your review and the comment.
>>>
>>> On 21/05/2026 22:01, Dmitry Osipenko wrote:
>>>> 21.05.2026 05:19, Ryosuke Yasuoka пишет:
>>>>> virtio_gpu_queue_ctrl_sgs() and virtio_gpu_queue_cursor() use
>>>>> wait_event() without any abort condition when waiting for virtqueue
>>>>> space. If the host device stops processing commands, these waits block
>>>>> indefinitely inside a drm_dev_enter/exit() critical section. Since
>>>>> drm_dev_unplug(), which is called in device removal and system shutdown
>>>>> call path, blocks on synchronize_srcu() until all critical sections
>>>>> complete, device removal and system shutdown also hang.
>>>>>
>>>>> Add a vqs_released flag to virtio_gpu_device and include it in the
>>>>> wait_event() condition. Set the flag and wake up both queues in a new
>>>>> virtio_gpu_release_vqs() helper, called before drm_dev_unplug() in both
>>>>> virtio_gpu_remove() and virtio_gpu_shutdown(). When the flag is set, the
>>>>> wait returns immediately and the command is aborted, following the same
>>>>> cleanup path as drm_dev_enter() failure.
>>>>>
>>>>> Reported-by: syzbot+d6dd6f86d3aaf7eebe7406e45c1c6e549453f224@xxxxxxxxxxxxxxxxxxxxxxxxx
>>>>> Closes: https://syzkaller.appspot.com/bug?id=d6dd6f86d3aaf7eebe7406e45c1c6e549453f224
>>>>> Reported-by: syzbot+908bd910da5dd79b88de4cf7baf376cc873a922e@xxxxxxxxxxxxxxxxxxxxxxxxx
>>>>> Closes: https://syzkaller.appspot.com/bug?id=908bd910da5dd79b88de4cf7baf376cc873a922e
>>>>> Signed-off-by: Ryosuke Yasuoka <ryasuoka@xxxxxxxxxx>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Update the commit message.
>>>>> - Replace wait_event_timeout() with wait_event() using a compound
>>>>> condition that includes a new vqs_released flag.
>>>>> - Add virtio_gpu_release_vqs() helper to set the flag and wake up
>>>>> both queues, called before drm_dev_unplug() in remove and shutdown
>>>>> paths.
>>>>> - Remove the hardcoded 5-second timeout. Recovery is now driven by
>>>>> the driver flag instead of an arbitrary timeout value.
>>>>> ---
>>>>>  drivers/gpu/drm/virtio/virtgpu_drv.c | 15 +++++++++++++++
>>>>>  drivers/gpu/drm/virtio/virtgpu_drv.h |  1 +
>>>>>  drivers/gpu/drm/virtio/virtgpu_vq.c  | 23 +++++++++++++++++++++--
>>>>>  3 files changed, 37 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
>>>>> index a5ce96fb8a1d..e4fe5e0780f9 100644
>>>>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
>>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
>>>>> @@ -119,10 +119,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);
>>>>>  	drm_atomic_helper_shutdown(dev);
>>>>>  	virtio_gpu_deinit(dev);
>>>>> @@ -133,6 +147,7 @@ static void virtio_gpu_shutdown(struct virtio_device *vdev)
>>>>>  {
>>>>>  	struct drm_device *dev = vdev->priv;
>>>>>  
>>>>> +	virtio_gpu_release_vqs(dev);
>>>>>  	/* stop talking to the device */
>>>>>  	drm_dev_unplug(dev);
>>>>>  }
>>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>>>>> index f17660a71a3e..0bd69a40857e 100644
>>>>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>>>>> @@ -235,6 +235,7 @@ struct virtio_gpu_device {
>>>>>  
>>>>>  	struct virtio_gpu_queue ctrlq;
>>>>>  	struct virtio_gpu_queue cursorq;
>>>>> +	bool vqs_released;
>>>>>  	struct kmem_cache *vbufs;
>>>>>  
>>>>>  	atomic_t pending_commands;
>>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
>>>>> index 67865810a2e7..8057a9b7356d 100644
>>>>> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
>>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
>>>>> @@ -396,7 +396,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;
>>>>>  	}
>>>>>  
>>>>> @@ -566,7 +578,14 @@ static void virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
>>>>>  	ret = virtqueue_add_sgs(vq, sgs, outcnt, 0, vbuf, GFP_ATOMIC);
>>>>>  	if (ret == -ENOSPC) {
>>>>>  		spin_unlock(&vgdev->cursorq.qlock);
>>>>> -		wait_event(vgdev->cursorq.ack_queue, vq->num_free >= outcnt);
>>>>> +		wait_event(vgdev->cursorq.ack_queue,
>>>>> +			   vq->num_free >= outcnt || vgdev->vqs_released);
>>>>> +		/* See comment in virtio_gpu_queue_ctrl_sgs(). */
>>>>> +		if (vgdev->vqs_released) {
>>>>> +			free_vbuf(vgdev, vbuf);
>>>>> +			drm_dev_exit(idx);
>>>>> +			return;
>>>>> +		}
>>>>>  		spin_lock(&vgdev->cursorq.qlock);
>>>>>  		goto retry;
>>>>>  	} else {
>>>>
>>>> What about other wait_event in the driver? Why only these?
>>>
>>> There are other wait_event() calls on vgdev->resp_wq in virtgpu_prime.c
>>> and virtgpu_vram.c. These can also cause a stuck process when the host
>>> device stops or does not respond and should be fixed. However, they are
>>> not related to the issue being fixed here. I think they should be
>>> addressed in a separate commit.
>>>
>>> The wait_event(vgdev->resp_wq) calls wait for a host response, not for
>>> virtqueue ring space. They are not inside a drm_dev_enter/exit()
>>> critical section, so they don't block drm_dev_unplug() ->
>>> synchronize_srcu() and are not part of the deadlock reported by syzbot.
>>>
>>> However, if a process is stuck in wait_event(vgdev->resp_wq), there is
>>> no way to recover other than host device recovery itself. We can still
>>> remove the device and destroy the virtqueues while the process is stuck,
>>> so the host response can never arrive. IIUC, the stuck thread holds a
>>> drm_device reference, preventing vgdev from being freed; this is a
>>> resource leak.
>>>
>>> This can be fixed by adding a wake_up_all() and a vqs_released check
>>> similar to what is done for ctrlq/cursorq. However, I think this should
>>> be a separate commit since the root cause and problem are different.
>>>
>>> Alternatively, the resp_wq wait_event() calls could be converted to
>>> wait_event_interruptible() to fix the issue. But as you mentioned
>>> earlier in the v1 comment, we need the wait_event_interruptible()
>>> rework.
>>>
>>> What do you think? Should I include the resp_wq fix as a separate commit
>>> in this patch series, or leave it for the _interruptible() rework?
>> 
>> Let's leave it for the later rework.
>> 
>> I briefly tried testing this patch and it's not apparent what exact
>> problem this patch solves.
>> 
>> With a regular Linux OS running systemd, during normal shutdown, systemd
>> waits for processes to be terminated before it would perform kernel
>> shutdown, hence virtio_gpu_shutdown() doesn't have a chance to be
>> invoked in my scenario.
>> 
>> In the referenced syzkaller reports, I don't see anything related to
>> shutdown or driver removal. Could you please clarify how to reproduce
>> and test the original issue?

You are right that the syzkaller reports show a different issue; a
probe-time lock ordering deadlock between clientlist_mutex and the
dequeue worker, not a device removal hang. I will drop the Reported-by
and Closes tags in the next revision. 

Regarding reproduction: the scenario this patch fixes is as follows:

1. The host device stops processing virtqueue commands (host process
crash, resource exhaustion, or a hypervisor bug)
2. A guest GPU operation fills the virtqueue and blocks in wait_event()
inside virtio_gpu_queue_ctrl_sgs() or virtio_gpu_queue_cursor(),
within a drm_dev_enter/exit() critical section
3. Device removal or system shutdown calls virtio_gpu_remove() or
virtio_gpu_shutdown(), which call drm_dev_unplug()
4. drm_dev_unplug() blocks on synchronize_srcu() because the
drm_dev_enter/exit() critical section from step 2 never completes
5. Both the GPU operation and the removal/shutdown hang indefinitely
with no recovery path

Since wait_event() uses TASK_UNINTERRUPTIBLE, the blocked process
cannot be killed by signals during shutdown, so the hang persists even
after systemd's process cleanup phase.

This is difficult to reproduce because it depends on host-side failure.
In a normal QEMU/KVM environment the virtqueue is always serviced.

> To make it more clear, the patch looks okay, but I'm not convinced that
> it fixes the reported issue. We likely need a full solution with
> handling interrupt signals, using a reasonable timeout and etc for all
> wake_event().

I agree that a full solution with wait_event_interruptible() and proper
signal handling for all wait_event() calls is the right long-term
direction, and I am happy to work on that as a follow-up series.

However, converting to wait_event_interruptible() alone does not fully
solve the device removal case. The remove/shutdown path does not send
signals to waiting processes; it only calls drm_dev_unplug(). So even
with interruptible waits, the driver still needs a mechanism to
explicitly wake up waiters during teardown. The vqs_released flag in
this patch provides that mechanism, and remains necessary even after
the interruptible rework.

I will resubmit with updated tags and commit message, and work on the
syzkaller probe-time deadlock and the interruptible rework as follow-up
patches.

Best regards,
Ryosuke