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

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

[Michael S. Tsirkin]

virtio code synchronizes callbacks on reset now,
no need to do it in virtio pci.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci_legacy.c | 2 --
 drivers/virtio/virtio_pci_modern.c | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index d9cbb02b35a1..8115aa39e01e 100644
--- 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);
 }
 
 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
index 6d8ae2a6a8ca..c9e21317c51a 100644
--- 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);
-
-	/* Flush pending VQ/configuration callbacks. */
-	vp_synchronize_vectors(vdev);
 }
 
 static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)
-- 
MST

[PATCH 1/2] virtio: synchronize callbacks after device reset

[Michael S. Tsirkin]

virtio_reset_device says:
	Note: this guarantees that vq callbacks are not in progress
but in practice, only virtio pci correctly synchronizes the cbs.

On other transports, a callback that is already executing, keeps running
while the driver tears down the state it uses.

Add virtio_synchronize_cbs to virtio_reset_device fixing this for all
transports that correctly implement synchronize_cbs().

Reported-by: Karl Mehltretter <kmehltretter@gmail.com>
Link: https://lore.kernel.org/all/20260818040433.66986-1-kmehltretter@gmail.com/
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index b6c9e927bef5..a8588d7ad109 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -264,6 +264,8 @@ void virtio_reset_device(struct virtio_device *dev)
 #endif
 
 	dev->config->reset(dev);
+	/* Flush pending VQ/configuration callbacks. */
+	virtio_synchronize_cbs(dev);
 }
 EXPORT_SYMBOL_GPL(virtio_reset_device);
 
-- 
MST

Re: [PATCH 1/2] virtio: synchronize callbacks after device reset

[sashiko-bot]

> virtio_reset_device says:
> 	Note: this guarantees that vq callbacks are not in progress
> but in practice, only virtio pci correctly synchronizes the cbs.
> 
> On other transports, a callback that is already executing, keeps running
> while the driver tears down the state it uses.
> 
> Add virtio_synchronize_cbs to virtio_reset_device fixing this for all
> transports that correctly implement synchronize_cbs().
> 
> Reported-by: Karl Mehltretter <kmehltretter@gmail.com>
> Link: https://lore.kernel.org/all/20260818040433.66986-1-kmehltretter@gmail.com/
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/8f71425918a148dcad1be993ccabd39582d246ee.1789130213.git.mst@redhat.com?part=1

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

[sashiko-bot]

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?

>  }
>  
>  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?

> -
> -	/* 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

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

[Michael S. Tsirkin]

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