Re: [PATCH v4 3/3] virtio_pci: drop callback sync on reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of callback synchronization from `vp_reset()` introduces a race condition during system shutdown, as `virtio_device_shutdown()` fails to synchronize callbacks after device reset.
--

commit a9d5f056fe92ddf169045b3557dd34b627d1eb0b
Author: Michael S. Tsirkin <mst@redhat.com>

virtio_pci: drop callback sync on reset

The virtio core now synchronizes callbacks after reset, so the PCI transports
no longer need to do it themselves.

> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index d9cbb02b35a11..8115aa39e01e9 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);

[Severity: High]
Does removing this synchronization introduce a race condition during system
shutdown?

During system shutdown, virtio_device_shutdown() in drivers/virtio/virtio.c
calls dev->config->reset(dev) directly without calling virtio_reset_device():

drivers/virtio/virtio.c:virtio_device_shutdown() {
	...
	dev->config->reset(dev);
	/* Missing virtio_synchronize_cbs(dev) here */
}

The vp_reset() implementation flushes pending MSI-X interrupts, which can
cause a configuration interrupt to fire. Because this patch removes the
internal synchronization from vp_reset(), virtio_device_shutdown() returns
immediately. 

The configuration interrupt (vp_config_changed()) could then run concurrently
on another CPU, invoking driver callbacks while the system is tearing down the
IOMMU or jumping into a kexec kernel.

>  }
>  
>  static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)

> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index b4249afd7f580..922e2df027de9 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
[ ... ]
> @@ -563,9 +563,6 @@ static void vp_reset(struct virtio_device *vdev)
>  	 */
>  	while (vp_modern_get_status(mdev))
>  		msleep(1);
> -
> -	/* Flush pending VQ/configuration callbacks. */
> -	vp_synchronize_vectors(vdev);

[Severity: High]
Does removing the synchronization here present the same issue?

This breaks the guarantee that interrupts flushed during the reset loop have
finished before returning, which virtio_device_shutdown() relies upon to
safely stop the device.

>  }
>  
>  static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789204858.git.mst@redhat.com?part=3