Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Removing `vp_synchronize_vectors()` from `vp_reset()` exposes a race condition in `virtio_device_shutdown()`, allowing in-flight configuration change interrupts to execute concurrently with or after system shutdown. Pre-existing issues: - [High] Pre-existing data race in `vp_modern_avq_cleanup()` due to missing `admin_vq` lock, leading to potential virtqueue corruption. -- commit 0b14e711d0c9fc7c440dbf71513420ceb19b4bd3 Author: Michael S. Tsirkin <[email protected]> virtio_pci: drop extra callback sync This patch drops the synchronization of callbacks on reset within virtio pci, relying instead on the virtio core to synchronize callbacks when needed. > --- 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 leave a race condition in the shutdown path? While the core virtio_reset_device() function was updated to synchronize after the reset returns, virtio_device_shutdown() appears to lack this synchronization: drivers/virtio/virtio.c:virtio_device_shutdown() { ... dev->config->reset(dev); // No trailing virtio_synchronize_cbs() call } Because vp_reset() writes to the status register and flushes in-flight interrupts, returning without synchronization means virtio_device_shutdown() can finish while a flushed vp_config_changed interrupt is still executing concurrently. Could this lead to unintended access to device structures or DMA memory during system teardown or kexec paths? > } > > 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 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c [ ... ] > @@ -559,9 +559,6 @@ static void vp_reset(struct virtio_device *vdev) > msleep(1); > > vp_modern_avq_cleanup(vdev); [Severity: High] This is a pre-existing issue, but is there a potential data race here with concurrent interrupt handlers? vp_modern_avq_cleanup() calls virtqueue_detach_unused_buf() without acquiring the admin_vq->lock. If CONFIG_VIRTIO_HARDEN_NOTIFICATION is disabled (leaving vq->broken as false), an in-flight admin vq interrupt flushed during reset might execute concurrently on another CPU: drivers/virtio/virtio_pci_modern.c:vp_modern_avq_done() { spin_lock_irqsave(&admin_vq->lock, flags); do { virtqueue_disable_cb(vq); while ((cmd = virtqueue_get_buf(vq, &len))) { ... } Could concurrent modification of the virtqueue's internal vring state cause memory corruption or a double complete() execution if the interrupt handler and cleanup routine run simultaneously? > - > - /* Flush pending VQ/configuration callbacks. */ > - vp_synchronize_vectors(vdev); > } > > static int vp_active_vq(struct virtqueue *vq, u16 msix_vec) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/8f71425918a148dcad1be993ccabd39582d246ee.1789130213.git....@redhat.com?part=2
