[PATCH v4 0/3] virtio: fix callback synchronization and avq cleanup on reset

[PATCH v4 0/3] virtio: fix callback synchronization and avq cleanup on reset

[Michael S. Tsirkin]

Two issues with virtio device reset:

1. Karl Mehltretter reported that virtio_reset_device() promises
   callbacks are not in progress after reset, but only PCI transports
   actually synchronize callbacks - other transports leave a window
   where a handler already executing keeps running while the driver
   tears down state.

2. sashiko reported a race in virtio_pci_modern: the avq interrupt
   handler calls virtqueue_get_buf concurrently with
   virtqueue_detach_unused_buf in vp_modern_avq_cleanup, and there
   is no synchronize_irq between reset and cleanup.

Fix 1 by adding virtio_synchronize_cbs in the core after reset,
then dropping the now-redundant per-transport sync calls. Fix 2 by
moving avq cleanup from vp_reset to a modern-specific del_vqs
wrapper, which runs after callbacks have been synchronized - and is
where buffer teardown conceptually belongs.

Changes v3->v4:
    patch 1: add Tested-by and Acked-by from Karl
    patch 2: was patch 3 in v3; instead of moving
        vp_modern_avq_cleanup() to the common vp_del_vqs(),
        add a vp_modern_del_vqs() wrapper in
        virtio_pci_modern.c. Split out callback sync removal
        into a separate patch.
    patch 3: was patch 2 in v3 (legacy only); now includes
        modern transport too.

Changes v2->v3:
    patch 1: unchanged
    patch 2: split from v2 patch 2 - legacy part only
    patch 3: new in v3

Michael S. Tsirkin (3):
  virtio: synchronize callbacks after device reset
  virtio_pci_modern: move avq cleanup from reset to del_vqs
  virtio_pci: drop callback sync on reset

 drivers/virtio/virtio.c            |  2 ++
 drivers/virtio/virtio_pci_legacy.c |  2 --
 drivers/virtio/virtio_pci_modern.c | 15 ++++++++-------
 3 files changed, 10 insertions(+), 9 deletions(-)

-- 
MST

[PATCH v4 1/3] 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/
Fixes: c46eccdaadab ("virtio: document virtio_reset_device")
Tested-by: Karl Mehltretter <kmehltretter@gmail.com>
Acked-by: Karl Mehltretter <kmehltretter@gmail.com>
Assisted-by: LLM
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    unchanged since v2

 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 v4 1/3] 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/
> Fixes: c46eccdaadab ("virtio: document virtio_reset_device")
> Tested-by: Karl Mehltretter <kmehltretter@gmail.com>
> Acked-by: Karl Mehltretter <kmehltretter@gmail.com>
> Assisted-by: LLM
> 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/cover.1789204858.git.mst@redhat.com?part=1

[PATCH v4 2/3] virtio_pci_modern: move avq cleanup from reset to del_vqs

[Michael S. Tsirkin]

vp_modern_avq_cleanup() detaches unused buffers from the admin
virtqueue. Calling it from vp_reset() is incorrect:
virtqueue_get_buf in the avq interrupt handler can race with
virtqueue_detach_unused_buf in cleanup, and get_buf after
detach is not documented as valid.

The root cause is that detaching buffers does not belong in
reset at all - reset quiesces the device, while cleanup belongs
where the virtqueue is about to be destroyed, from del_vqs.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/virtualization/20260911125745.E0A2F1F00899@smtp.kernel.org/
Fixes: 4c3b54af907e ("virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result")
Cc: Jiri Pirko <jiri@resnulli.us>
Assisted-by: LLM
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    v3->v4:
        split patch 3: avq cleanup move is now a separate
        patch from callback sync removal. Callback sync
        removal (previously modern-only in patch 3) merged
        with legacy removal into a single patch.
    
    v2->v3:
        new in v3

 drivers/virtio/virtio_pci_modern.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 6d8ae2a6a8ca..b4249afd7f58 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -364,6 +364,12 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
 	}
 }
 
+static void vp_modern_del_vqs(struct virtio_device *vdev)
+{
+	vp_modern_avq_cleanup(vdev);
+	vp_del_vqs(vdev);
+}
+
 static void vp_transport_features(struct virtio_device *vdev, u64 features)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -558,8 +564,6 @@ static void vp_reset(struct virtio_device *vdev)
 	while (vp_modern_get_status(mdev))
 		msleep(1);
 
-	vp_modern_avq_cleanup(vdev);
-
 	/* Flush pending VQ/configuration callbacks. */
 	vp_synchronize_vectors(vdev);
 }
@@ -1232,7 +1236,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.set_status	= vp_set_status,
 	.reset		= vp_reset,
 	.find_vqs	= vp_modern_find_vqs,
-	.del_vqs	= vp_del_vqs,
+	.del_vqs	= vp_modern_del_vqs,
 	.synchronize_cbs = vp_synchronize_vectors,
 	.get_extended_features = vp_get_features,
 	.finalize_features = vp_finalize_features,
@@ -1252,7 +1256,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.set_status	= vp_set_status,
 	.reset		= vp_reset,
 	.find_vqs	= vp_modern_find_vqs,
-	.del_vqs	= vp_del_vqs,
+	.del_vqs	= vp_modern_del_vqs,
 	.synchronize_cbs = vp_synchronize_vectors,
 	.get_extended_features = vp_get_features,
 	.finalize_features = vp_finalize_features,
-- 
MST

Re: [PATCH v4 2/3] virtio_pci_modern: move avq cleanup from reset to del_vqs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Surprise removal of the PF device while a VF is executing an admin command leads to an unresolvable deadlock.
--

commit 77107fdcf13b9589fc326075af29c2e63cc3478d
Author: Michael S. Tsirkin <mst@redhat.com>

virtio_pci_modern: move avq cleanup from reset to del_vqs

This commit moves the admin virtqueue cleanup from the device reset path
to the virtqueue deletion path to prevent a race condition with the interrupt
handler.

Link: https://lore.kernel.org/virtualization/20260911125745.E0A2F1F00899@smtp.kernel.org/

> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 6d8ae2a6a8caf..b4249afd7f580 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c

[ ... ]

> @@ -364,6 +364,12 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
>  	}
>  }
>  
> +static void vp_modern_del_vqs(struct virtio_device *vdev)
> +{
> +	vp_modern_avq_cleanup(vdev);

[Severity: High]
This isn't a bug introduced by this patch, but could this architecture still
lead to an unresolvable deadlock during a surprise removal of the PF device?

If a VF driver executes an admin command, the thread blocks uninterruptibly
in virtqueue_exec_admin_cmd() waiting for the command to complete.

If a surprise removal of the PF occurs while the VF is blocked, the PF
driver calls virtio_pci_remove(), which then calls pci_disable_sriov() to
wait for all VFs to unbind.

However, the VF unbind is blocked because the VF thread is stuck in
wait_for_completion(). Because pci_disable_sriov() never returns, the PF
removal never reaches unregister_virtio_device() and thus never calls this
vp_modern_del_vqs() function to detach the unused buffers and unblock the
VF thread.

Is there a way to abort the pending admin commands earlier in the PF surprise
removal path, before it blocks on disabling SRIOV?

> +	vp_del_vqs(vdev);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789204858.git.mst@redhat.com?part=2

[PATCH v4 3/3] virtio_pci: drop callback sync on reset

[Michael S. Tsirkin]

The virtio core now synchronizes callbacks after reset,
so the PCI transports no longer need to do it themselves.

Assisted-by: LLM
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    v3->v4:
        now includes modern transport (previously only legacy
        in patch 2, modern was in patch 3)
    
    v2->v3:
        split from v2 patch 2 - legacy part only

 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 b4249afd7f58..922e2df027de 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -563,9 +563,6 @@ static void vp_reset(struct virtio_device *vdev)
 	 */
 	while (vp_modern_get_status(mdev))
 		msleep(1);
-
-	/* Flush pending VQ/configuration callbacks. */
-	vp_synchronize_vectors(vdev);
 }
 
 static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)
-- 
MST

Re: [PATCH v4 3/3] virtio_pci: drop callback sync on reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of callback synchronization from `vp_reset()` introduces a race condition during system shutdown, as `virtio_device_shutdown()` fails to synchronize callbacks after device reset.
--

commit a9d5f056fe92ddf169045b3557dd34b627d1eb0b
Author: Michael S. Tsirkin <mst@redhat.com>

virtio_pci: drop callback sync on reset

The virtio core now synchronizes callbacks after reset, so the PCI transports
no longer need to do it themselves.

> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index d9cbb02b35a11..8115aa39e01e9 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);

[Severity: High]
Does removing this synchronization introduce a race condition during system
shutdown?

During system shutdown, virtio_device_shutdown() in drivers/virtio/virtio.c
calls dev->config->reset(dev) directly without calling virtio_reset_device():

drivers/virtio/virtio.c:virtio_device_shutdown() {
	...
	dev->config->reset(dev);
	/* Missing virtio_synchronize_cbs(dev) here */
}

The vp_reset() implementation flushes pending MSI-X interrupts, which can
cause a configuration interrupt to fire. Because this patch removes the
internal synchronization from vp_reset(), virtio_device_shutdown() returns
immediately. 

The configuration interrupt (vp_config_changed()) could then run concurrently
on another CPU, invoking driver callbacks while the system is tearing down the
IOMMU or jumping into a kexec kernel.

>  }
>  
>  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 b4249afd7f580..922e2df027de9 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
[ ... ]
> @@ -563,9 +563,6 @@ static void vp_reset(struct virtio_device *vdev)
>  	 */
>  	while (vp_modern_get_status(mdev))
>  		msleep(1);
> -
> -	/* Flush pending VQ/configuration callbacks. */
> -	vp_synchronize_vectors(vdev);

[Severity: High]
Does removing the synchronization here present the same issue?

This breaks the guarantee that interrupts flushed during the reset loop have
finished before returning, which virtio_device_shutdown() relies upon to
safely stop the device.

>  }
>  
>  static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789204858.git.mst@redhat.com?part=3