* [PATCH 1/2] virtio: synchronize callbacks after device reset @ 2026-09-11 12:37 Michael S. Tsirkin 2026-09-11 12:37 ` [PATCH 2/2] virtio_pci: drop extra callback sync Michael S. Tsirkin 2026-09-11 12:44 ` [PATCH 1/2] virtio: synchronize callbacks after device reset sashiko-bot 0 siblings, 2 replies; 5+ messages in thread From: Michael S. Tsirkin @ 2026-09-11 12:37 UTC (permalink / raw) To: linux-kernel Cc: Karl Mehltretter, Jason Wang, Eugenio Pérez, Xuan Zhuo, virtualization 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] virtio_pci: drop extra callback sync 2026-09-11 12:37 [PATCH 1/2] virtio: synchronize callbacks after device reset Michael S. Tsirkin @ 2026-09-11 12:37 ` Michael S. Tsirkin 2026-09-11 12:57 ` sashiko-bot 2026-09-11 12:44 ` [PATCH 1/2] virtio: synchronize callbacks after device reset sashiko-bot 1 sibling, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2026-09-11 12:37 UTC (permalink / raw) To: linux-kernel Cc: Karl Mehltretter, Jason Wang, Eugenio Pérez, Xuan Zhuo, virtualization 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] virtio_pci: drop extra callback sync 2026-09-11 12:37 ` [PATCH 2/2] virtio_pci: drop extra callback sync Michael S. Tsirkin @ 2026-09-11 12:57 ` sashiko-bot 2026-09-11 19:38 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: sashiko-bot @ 2026-09-11 12:57 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization, Eugenio Perez, Michael S. Tsirkin 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] virtio_pci: drop extra callback sync 2026-09-11 12:57 ` sashiko-bot @ 2026-09-11 19:38 ` Michael S. Tsirkin 0 siblings, 0 replies; 5+ messages in thread From: Michael S. Tsirkin @ 2026-09-11 19:38 UTC (permalink / raw) To: sashiko-reviews; +Cc: virtualization, Eugenio Perez 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] virtio: synchronize callbacks after device reset 2026-09-11 12:37 [PATCH 1/2] virtio: synchronize callbacks after device reset Michael S. Tsirkin 2026-09-11 12:37 ` [PATCH 2/2] virtio_pci: drop extra callback sync Michael S. Tsirkin @ 2026-09-11 12:44 ` sashiko-bot 1 sibling, 0 replies; 5+ messages in thread From: sashiko-bot @ 2026-09-11 12:44 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-11 19:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-11 12:37 [PATCH 1/2] virtio: synchronize callbacks after device reset Michael S. Tsirkin 2026-09-11 12:37 ` [PATCH 2/2] virtio_pci: drop extra callback sync Michael S. Tsirkin 2026-09-11 12:57 ` sashiko-bot 2026-09-11 19:38 ` Michael S. Tsirkin 2026-09-11 12:44 ` [PATCH 1/2] virtio: synchronize callbacks after device reset sashiko-bot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.