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+d6dd6f86d3aaf7eebe7406e45c1c6e549453f...@syzkaller.appspotmail.com >>>>> Closes: >>>>> https://syzkaller.appspot.com/bug?id=d6dd6f86d3aaf7eebe7406e45c1c6e549453f224 >>>>> Reported-by: >>>>> syzbot+908bd910da5dd79b88de4cf7baf376cc873a9...@syzkaller.appspotmail.com >>>>> Closes: >>>>> https://syzkaller.appspot.com/bug?id=908bd910da5dd79b88de4cf7baf376cc873a922e >>>>> Signed-off-by: Ryosuke Yasuoka <[email protected]> >>>>> --- >>>>> 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
