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 <[email protected]> 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/[email protected] > 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/[email protected]?part=1
