Re: [PATCH 2/2] virtio_pci: drop extra callback sync

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 11, 2026 at 12:57:45PM +0000, sashiko-bot@kernel.org 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 <mst@redhat.com>
> 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.mst@redhat.com?part=2