virtio: fix callback synchronization and avq cleanup on reset

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

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().

---

Notes (changelog):
unchanged since v2

 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

index b6c9e927bef5..a8588d7ad109 100644
@@ -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

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.

---

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(-)

index 6d8ae2a6a8ca..b4249afd7f58 100644
@@ -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

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

---

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(-)

index d9cbb02b35a1..8115aa39e01e 100644
@@ -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)

index b4249afd7f58..922e2df027de 100644
@@ -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