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 vp_del_vqs, which runs after callbacks have been synchronized - and is where buffer teardown conceptually belongs.

Changes v2->v3:
patch 1: unchanged
patch 2: split from v2 patch 2 - legacy part only patch 3: new - v2 just dropped the sync from modern vp_reset,
leaving avq_cleanup there before the removed sync. v3 moves avq_cleanup out of vp_reset entirely into vp_del_vqs, fixing the race. Adds NULL check for admin_vq.info needed because find_vqs error paths call vp_del_vqs before it is allocated.

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

 drivers/virtio/virtio.c            |  2 ++
 drivers/virtio/virtio_pci_common.c |  2 ++
 drivers/virtio/virtio_pci_common.h |  1 +
 drivers/virtio/virtio_pci_legacy.c |  2 --
 drivers/virtio/virtio_pci_modern.c | 10 ++++------
 5 files changed, 9 insertions(+), 8 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().

---
Changes v2->v3: unchanged

 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

I ran 1,200 instrumented virtio-input reset cycles on x86-64 PCI and arm64 virtio-mmio under KASAN, KCSAN and lockdep. All 173 callback/reset overlaps completed before reset returned.

Thanks,
Karl

The virtio core now synchronizes callbacks after reset, so the legacy PCI transport no longer needs to do it itself.

---
Changes v2->v3: split from v2 patch 2 (legacy part only)

 drivers/virtio/virtio_pci_legacy.c | 2 --
 1 file changed, 2 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)

MST

I ran 200 instrumented virtio-input reset cycles over legacy INTx under KASAN and lockdep. All 19 callback/reset overlaps completed before reset returned.

Thanks,
Karl

vp_modern_avq_cleanup() detaches unused buffers from the admin virtqueue and completes pending commands with -EIO. 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, in del_vqs.

Move the call to vp_del_vqs(), which runs after virtio_synchronize_cbs() has already guaranteed that no interrupt handler is in progress, eliminating the race.

---
New in v3. v2 dropped sync from modern vp_reset in one combined patch, leaving avq_cleanup in vp_reset. v3 moves avq_cleanup out of vp_reset entirely into vp_del_vqs, fixing the race. Adds NULL check for admin_vq.info for find_vqs error paths.

 drivers/virtio/virtio_pci_common.c |  2 ++
 drivers/virtio/virtio_pci_common.h |  1 +
 drivers/virtio/virtio_pci_modern.c | 10 ++++------
 3 files changed, 7 insertions(+), 6 deletions(-)

index b90c174450b2..28b254ee4726 100644
@@ -270,6 +270,8 @@ void vp_del_vqs(struct virtio_device *vdev)
 	struct virtqueue *vq, *n;
 	int i;
 
+	vp_modern_avq_cleanup(vdev);
+
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		info = vp_is_avq(vdev, vq->index) ? vp_dev->admin_vq.info :
 						    vp_dev->vqs[vq->index];

index 8cd01de27baf..a4ff6ec903a3 100644
@@ -194,6 +194,7 @@ struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
 #endif
 
 bool vp_is_avq(struct virtio_device *vdev, unsigned int index);
+void vp_modern_avq_cleanup(struct virtio_device *vdev);
 void vp_modern_avq_done(struct virtqueue *vq);
 int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
 			     struct virtio_admin_cmd *cmd);

index 6d8ae2a6a8ca..ef76f35c6b2c 100644
@@ -345,7 +345,7 @@ static void vp_modern_avq_activate(struct virtio_device *vdev)
 	virtio_pci_admin_cmd_cap_init(vdev);
 }
 
-static void vp_modern_avq_cleanup(struct virtio_device *vdev)
+void vp_modern_avq_cleanup(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_admin_cmd *cmd;

@@ -354,6 +354,9 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
 	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
 		return;
 
+	if (!vp_dev->admin_vq.info)
+		return;
+
 	vq = vp_dev->admin_vq.info->vq;
 	if (!vq)
 		return;

@@ -557,11 +560,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);
 }
 
 static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)

MST

I think patch 3 introduces a use-after-free in the MSI-X setup error path.

With VP_VQ_VECTOR_POLICY_EACH, vp_setup_vq() stores the admin queue's info pointer before request_irq(). If request_irq() fails, vp_del_vq() frees info but leaves admin_vq.info set.

vp_find_vqs_msix() then calls vp_del_vqs(). vp_del_vqs() now calls vp_modern_avq_cleanup() first and dereferences the freed pointer:

    vq = vp_dev->admin_vq.info->vq;

No interrupt needs to fire.

I reproduced this on v3 with KASAN by returning -ENOMEM only for the admin queue's request_irq():

    BUG: KASAN: slab-use-after-free in vp_modern_avq_cleanup+0xf4/0x110

KASAN shows the allocation in vp_setup_vq(), the free in vp_find_one_vq_msix(), and the access in vp_modern_avq_cleanup(). Adding *p_info = NULL after vp_del_vq() made the same test fall back and boot without a KASAN report.

Thanks,
Karl

Right. Besides, poking at modern from common is ugly. I'll rework this, thanks a lot for the testing!

MST