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: [email protected] Closes: https://lore.kernel.org/virtualization/[email protected]/ Fixes: 4c3b54af907e ("virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result") Cc: Jiri Pirko <[email protected]> Signed-off-by: Michael S. Tsirkin <[email protected]> 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
