Re: [PATCH v2] drm/virtio: fix deadlock in display_info_cb by removing hotplug from dequeue worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Moving the acknowledgment of the VIRTIO_GPU_EVENT_DISPLAY event (`events_clear` write) to the end of the work function, after a potential 5-second `wait_event_timeout`, introduces a race window where subsequent hardware hotplug events can be lost.
- [High] Userspace permanently misses hotplug events if the virtqueue response takes longer than 5 seconds.
- [High] The patch introduces a 5-second stall during device removal or resource exhaustion because it unconditionally waits for a response without checking if the command was successfully queued.

Pre-existing issues:
- [High] Flushing virtqueue dequeue workers before resetting the device leaves a window for new interrupts to reschedule the workers, leading to Use-After-Free on teardown.
--

--- Patch [1]: [PATCH v2] drm/virtio: fix deadlock in display_info_cb by removing hotplug from dequeue worker ---
commit 22579633647788efae56c2e855be2b742b38fc3c
Author: Ryosuke Yasuoka <ryasuoka@redhat.com>

    drm/virtio: fix deadlock in display_info_cb by removing hotplug from dequeue worker
    
    A probe-time deadlock can occur between the dequeue worker and
    drm_client_register(). During probe, drm_client_register() holds
    clientlist_mutex and calls the fbdev hotplug callback, which triggers an
    atomic commit that ends up sleeping in virtio_gpu_queue_ctrl_sgs()
    waiting for virtqueue space.

    Closes: https://syzkaller.appspot.com/bug?id=d6dd6f86d3aaf7eebe7406e45c1c6e549453f224
    Closes: https://syzkaller.appspot.com/bug?id=908bd910da5dd79b88de4cf7baf376cc873a922e
    Link: https://lore.kernel.org/r/20260630-virtiogpu_syzbot-v1-1-0aa06630750e@redhat.com

> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index cfde9f573df6e..b4329f28e9766 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -49,7 +49,10 @@ static void virtio_gpu_config_changed_work_func(struct work_struct *work)
>  				virtio_gpu_cmd_get_edids(vgdev);
>  			virtio_gpu_cmd_get_display_info(vgdev);
>  			virtio_gpu_notify(vgdev);
> -			drm_helper_hpd_irq_event(vgdev->ddev);
> +			wait_event_timeout(vgdev->resp_wq,
> +					   !vgdev->display_info_pending,
> +					   5 * HZ);
> +			drm_kms_helper_hotplug_event(vgdev->ddev);

[Severity: High]
If virtio_gpu_cmd_get_display_info() fails to queue the buffer (for example,
if drm_dev_enter() fails during a hot-unplug), virtio_gpu_queue_ctrl_buffer()
will return an error but display_info_pending remains true.

Does this cause wait_event_timeout() to stall for the full 5 seconds during
device removal?

[Severity: High]
If the host takes longer than 5 seconds to respond, wait_event_timeout() will
expire and drm_kms_helper_hotplug_event() will fire using the old connector
data.

When the delayed response eventually arrives, the callback in
virtio_gpu_cmd_get_display_info_cb() updates the internal driver state but no
longer triggers a hotplug event because it was removed in this patch.

Will this leave userspace permanently out of sync with the hardware state?

>  		}
>  		events_clear |= VIRTIO_GPU_EVENT_DISPLAY;
>  	}

[Severity: High]
Does moving the wait_event_timeout() before the virtio_cwrite_le() call open a
race condition?

If a hardware hotplug event is asserted while this function is sleeping, the
subsequent write of events_clear to the device config space will clear the
newly asserted event without processing it, causing the event to be lost.

[ ... ]

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index c8b9475a7472e..e5e1af8b8e8a0 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -840,9 +840,6 @@ static void virtio_gpu_cmd_get_display_info_cb(struct virtio_gpu_device *vgdev,
>  	vgdev->display_info_pending = false;
>  	spin_unlock(&vgdev->display_info_lock);
>  	wake_up(&vgdev->resp_wq);
> -
> -	if (!drm_helper_hpd_irq_event(vgdev->ddev))
> -		drm_kms_helper_hotplug_event(vgdev->ddev);
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free during
device removal in virtio_gpu_deinit():

drivers/gpu/drm/virtio/virtgpu_kms.c:virtio_gpu_deinit() {
    ...
	flush_work(&vgdev->config_changed_work);
	virtio_reset_device(vgdev->vdev);
    ...
}

Are we at risk of rescheduling workers if a virtqueue interrupt or config
change fires immediately after flush_work() returns, but before
virtio_reset_device() stops the interrupts? If the driver frees vgdev
shortly after, the rescheduled worker might access freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-virtiogpu_syzbot-v2-1-2958fa37d46d@redhat.com?part=1