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

[PATCH v3 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 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

[PATCH v3 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")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
---
Changes v2->v3: unchanged

 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 v3 1/3] virtio: synchronize callbacks after device reset

[Karl Mehltretter]

On Fri, Sep 11, 2026 at 05:20:09PM +0100, Michael S. Tsirkin wrote:
> Add virtio_synchronize_cbs to virtio_reset_device fixing this for all
> transports that correctly implement synchronize_cbs().
> 

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.

Tested-by: Karl Mehltretter <kmehltretter@gmail.com>
Acked-by: Karl Mehltretter <kmehltretter@gmail.com>

Thanks,
Karl

[PATCH v3 3/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 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.

Reported-by: sashiko-bot@kernel.org
Closes: 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>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
---
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(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index b90c174450b2..28b254ee4726 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -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];
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 8cd01de27baf..a4ff6ec903a3 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -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);
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 6d8ae2a6a8ca..ef76f35c6b2c 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -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

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

[Karl Mehltretter]

On Fri, Sep 11, 2026 at 05:20:15PM +0100, Michael S. Tsirkin wrote:
> 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.
> 

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

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

[Michael S. Tsirkin]

On Sat, Sep 12, 2026 at 12:48:54AM +0200, Karl Mehltretter wrote:
> On Fri, Sep 11, 2026 at 05:20:15PM +0100, Michael S. Tsirkin wrote:
> > 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.
> > 
> 
> 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

[PATCH v3 2/3] virtio_pci_legacy: drop callback sync on reset

[Michael S. Tsirkin]

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

Assisted-by: LLM
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Changes v2->v3: split from v2 patch 2 (legacy part only)

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

Re: [PATCH v3 2/3] virtio_pci_legacy: drop callback sync on reset

[Karl Mehltretter]

On Fri, Sep 11, 2026 at 05:20:18PM +0100, Michael S. Tsirkin wrote:
> The virtio core now synchronizes callbacks after reset,
> so the legacy PCI transport no longer needs to do it itself.
> 

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

Tested-by: Karl Mehltretter <kmehltretter@gmail.com>
Acked-by: Karl Mehltretter <kmehltretter@gmail.com>

Thanks,
Karl