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

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

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@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?id=d6dd6f86d3aaf7eebe7406e45c1c6e549453f224
>>>> Reported-by: syzbot+908bd910da5dd79b88de4cf7baf376cc873a922e@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?id=908bd910da5dd79b88de4cf7baf376cc873a922e
>>>> Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
>>>> ---
>>>> 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?
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().

-- 
Best regards,
Dmitry