On Fri, Sep 11, 2026 at 12:57:45PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> New issues:
> - [High] Removing `vp_synchronize_vectors()` from `vp_reset()` exposes a race
> condition in `virtio_device_shutdown()`, allowing in-flight configuration
> change interrupts to execute concurrently with or after system shutdown.
>
> Pre-existing issues:
> - [High] Pre-existing data race in `vp_modern_avq_cleanup()` due to missing
> `admin_vq` lock, leading to potential virtqueue corruption.
> --
>
> commit 0b14e711d0c9fc7c440dbf71513420ceb19b4bd3
> Author: Michael S. Tsirkin <[email protected]>
> virtio_pci: drop extra callback sync
>
> This patch drops the synchronization of callbacks on reset within virtio pci,
> relying instead on the virtio core to synchronize callbacks when needed.
>
> > --- a/drivers/virtio/virtio_pci_legacy.c
> > +++ b/drivers/virtio/virtio_pci_legacy.c
> [ ... ]
> > @@ -98,8 +98,6 @@ static void vp_reset(struct virtio_device *vdev)
> > /* Flush out the status write, and flush in device writes,
> > * including MSi-X interrupts, if any. */
> > vp_legacy_get_status(&vp_dev->ldev);
> > - /* Flush pending VQ/configuration callbacks. */
> > - vp_synchronize_vectors(vdev);
>
> [Severity: High]
> Does removing this synchronization leave a race condition in the shutdown
> path?
>
> While the core virtio_reset_device() function was updated to synchronize
> after the reset returns, virtio_device_shutdown() appears to lack this
> synchronization:
>
> drivers/virtio/virtio.c:virtio_device_shutdown() {
> ...
> dev->config->reset(dev);
> // No trailing virtio_synchronize_cbs() call
> }
>
> Because vp_reset() writes to the status register and flushes in-flight
> interrupts, returning without synchronization means virtio_device_shutdown()
> can finish while a flushed vp_config_changed interrupt is still executing
> concurrently.
>
> Could this lead to unintended access to device structures or DMA memory
> during system teardown or kexec paths?
shutdown does not free memory so I do not see what is the issue
and why we need to prevent cbs from running.
> > }
> >
> > static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> > diff --git a/drivers/virtio/virtio_pci_modern.c
> > b/drivers/virtio/virtio_pci_modern.c
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> [ ... ]
> > @@ -559,9 +559,6 @@ static void vp_reset(struct virtio_device *vdev)
> > msleep(1);
> >
> > vp_modern_avq_cleanup(vdev);
>
> [Severity: High]
> This is a pre-existing issue, but is there a potential data race here with
> concurrent interrupt handlers?
>
> vp_modern_avq_cleanup() calls virtqueue_detach_unused_buf() without
> acquiring the admin_vq->lock. If CONFIG_VIRTIO_HARDEN_NOTIFICATION is
> disabled (leaving vq->broken as false), an in-flight admin vq interrupt
> flushed during reset might execute concurrently on another CPU:
>
> drivers/virtio/virtio_pci_modern.c:vp_modern_avq_done() {
> spin_lock_irqsave(&admin_vq->lock, flags);
> do {
> virtqueue_disable_cb(vq);
> while ((cmd = virtqueue_get_buf(vq, &len))) {
> ...
> }
>
> Could concurrent modification of the virtqueue's internal vring state cause
> memory corruption or a double complete() execution if the interrupt handler
> and cleanup routine run simultaneously?
This is indeed an issue I think: an obvious fix is for now is to move a
sync before vp_modern_avq_cleanup instead of removing it. The
implication is that core irq sync is not really needed for pci, but hey,
it's inexpensive.
> > -
> > - /* Flush pending VQ/configuration callbacks. */
> > - vp_synchronize_vectors(vdev);
> > }
> >
> > static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/8f71425918a148dcad1be993ccabd39582d246ee.1789130213.git....@redhat.com?part=2
