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

[sashiko-bot@kernel.org]

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 <mst@redhat.com>

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/20260911125745.E0A2F1F00899@smtp.kernel.org/

> 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/cover.1789160982.git.mst@redhat.com?part=3