Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Moving `vp_modern_avq_cleanup()` out of `vp_reset()` causes admin command waiters to hang indefinitely during system shutdown. - [High] Use-After-Free in `vp_modern_avq_cleanup()` triggered during `find_vqs` error paths or virtio driver re-binds due to a dangling `admin_vq.info` pointer. --
commit 9f596701bb244c686f63dc19e6844b130e778230 Author: Michael S. Tsirkin <[email protected]> virtio_pci_modern: move avq cleanup from reset to del_vqs vp_modern_avq_cleanup() detaches unused buffers from the admin virtqueue and completes pending commands with -EIO. Calling it from vp_reset() is incorrect because of a race condition. This commit moves the call to vp_del_vqs() to eliminate the race. Closes: https://lore.kernel.org/virtualization/[email protected]/ > diff --git a/drivers/virtio/virtio_pci_common.c > b/drivers/virtio/virtio_pci_common.c > index b90c174450b20..28b254ee47266 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); > + [Severity: High] Could moving the cleanup here leave admin command waiters hanging indefinitely during system shutdown? If a device reset is initiated (e.g., via virtio_device_shutdown() or power management freeze) while an admin command is executing, vp_reset() is called. Previously, this safely aborted pending commands with -EIO. With the cleanup moved to vp_del_vqs(), which is not called during system shutdown or suspend, will any thread blocked on wait_for_completion() in virtqueue_exec_admin_cmd() hang forever since the commands are never aborted? > list_for_each_entry_safe(vq, n, &vdev->vqs, list) { > info = vp_is_avq(vdev, vq->index) ? vp_dev->admin_vq.info : [ ... ] > diff --git a/drivers/virtio/virtio_pci_modern.c > b/drivers/virtio/virtio_pci_modern.c > index 6d8ae2a6a8caf..ef76f35c6b2c0 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c [ ... ] > @@ -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; > + [Severity: High] Does this check fail to prevent a use-after-free if the pointer is left dangling? When queue initialization fails (e.g., IRQ allocation in vp_find_one_vq_msix()), the error path frees the info structure via vp_del_vq() but fails to clear vp_dev->admin_vq.info. Later, the error rollback path in vp_find_vqs_msix() calls vp_del_vqs(), which immediately calls vp_modern_avq_cleanup(): vp_find_vqs_msix() error path: error_find: vp_del_vqs() vp_modern_avq_cleanup() if (!vp_dev->admin_vq.info) Will this result in dereferencing the freed pointer when accessing vp_dev->admin_vq.info->vq on the next line? > vq = vp_dev->admin_vq.info->vq; > if (!vq) > return; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
