Message ID | a3612104baa9fe72d9e3182e3d108bff96a9ddf4.1713172726.git.mst@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v7] virtio-pci: Fix the crash that the vector was used after released. | expand |
On 15/4/24 11:19, Michael S. Tsirkin wrote: > From: Cindy Lu <lulu@redhat.com> > > During the booting process of the non-standard image, the behavior of the > called function in qemu is as follows: > > 1. vhost_net_stop() was triggered by guest image. This will call the function > virtio_pci_set_guest_notifiers() with assgin= false, > virtio_pci_set_guest_notifiers() will release the irqfd for vector 0 > > 2. virtio_reset() was triggered, this will set configure vector to VIRTIO_NO_VECTOR > > 3.vhost_net_start() was called (at this time, the configure vector is > still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with > assgin=true, so the irqfd for vector 0 is still not "init" during this process > > 4. The system continues to boot and sets the vector back to 0. After that > msix_fire_vector_notifier() was triggered to unmask the vector 0 and meet the crash > > To fix the issue, we need to support changing the vector after VIRTIO_CONFIG_S_DRIVER_OK is set. > MST: coding style and typo fixups > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > Cc: qemu-stable@nongnu.org > Signed-off-by: Cindy Lu <lulu@redhat.com> > Message-Id: <20240412062750.475180-1-lulu@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > To expedite fixed lots of style issues myself. > Completely untested - guys can you pls test and ack? > > > hw/virtio/virtio-pci.c | 41 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index cb6940fc0e..e9edd57339 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1424,6 +1424,34 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, > return offset; > } > > +static void virtio_pci_set_and_change_vector(VirtIODevice *vdev, > + VirtIOPCIProxy *proxy, > + int queue_no, uint16_t old_vector, > + uint16_t new_vector) > +{ Alternatively: if (new_vector == old_vector) { return; } > + bool kvm_irqfd = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && > + msix_enabled(&proxy->pci_dev) && kvm_msi_via_irqfd_enabled(); > + > + /* > + * If the device uses irqfd and the vector changes after DRIVER_OK is > + * set, we need to release the old vector and set up the new one. > + * Otherwise just need to set the new vector on the device. > + */ > + if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR) { > + kvm_virtio_pci_vector_release_one(proxy, queue_no); > + } > + /* Set the new vector on the device. */ > + if (queue_no == VIRTIO_CONFIG_IRQ_IDX) { > + vdev->config_vector = new_vector; > + } else { > + virtio_queue_set_vector(vdev, queue_no, new_vector); > + } > + /* If the new vector changed need to set it up. */ > + if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR) { > + kvm_virtio_pci_vector_use_one(proxy, queue_no); > + } > +} > + > int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, > uint8_t bar, uint64_t offset, uint64_t length, > uint8_t id) > @@ -1570,7 +1598,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, > } else { > val = VIRTIO_NO_VECTOR; > } > - vdev->config_vector = val; > + vector = vdev->config_vector; > + /* Check if we need to change the vector. */ > + if (val != vector) { > + virtio_pci_set_and_change_vector(vdev, proxy, VIRTIO_CONFIG_IRQ_IDX, > + vector, val); > + } > break;
On Mon, Apr 15, 2024 at 11:23:46AM +0200, Philippe Mathieu-Daudé wrote: > On 15/4/24 11:19, Michael S. Tsirkin wrote: > > From: Cindy Lu <lulu@redhat.com> > > > > During the booting process of the non-standard image, the behavior of the > > called function in qemu is as follows: > > > > 1. vhost_net_stop() was triggered by guest image. This will call the function > > virtio_pci_set_guest_notifiers() with assgin= false, > > virtio_pci_set_guest_notifiers() will release the irqfd for vector 0 > > > > 2. virtio_reset() was triggered, this will set configure vector to VIRTIO_NO_VECTOR > > > > 3.vhost_net_start() was called (at this time, the configure vector is > > still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with > > assgin=true, so the irqfd for vector 0 is still not "init" during this process > > > > 4. The system continues to boot and sets the vector back to 0. After that > > msix_fire_vector_notifier() was triggered to unmask the vector 0 and meet the crash > > > > To fix the issue, we need to support changing the vector after VIRTIO_CONFIG_S_DRIVER_OK is set. > > > > MST: coding style and typo fixups > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > Message-Id: <20240412062750.475180-1-lulu@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > To expedite fixed lots of style issues myself. > > Completely untested - guys can you pls test and ack? > > > > > > hw/virtio/virtio-pci.c | 41 +++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 39 insertions(+), 2 deletions(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index cb6940fc0e..e9edd57339 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1424,6 +1424,34 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, > > return offset; > > } > > +static void virtio_pci_set_and_change_vector(VirtIODevice *vdev, > > + VirtIOPCIProxy *proxy, > > + int queue_no, uint16_t old_vector, > > + uint16_t new_vector) > > +{ > > Alternatively: > > if (new_vector == old_vector) { > return; > } True. And in fact callers do not need check this. Let me do v8. > > + bool kvm_irqfd = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && > > + msix_enabled(&proxy->pci_dev) && kvm_msi_via_irqfd_enabled(); > > + > > + /* > > + * If the device uses irqfd and the vector changes after DRIVER_OK is > > + * set, we need to release the old vector and set up the new one. > > + * Otherwise just need to set the new vector on the device. > > + */ > > + if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR) { > > + kvm_virtio_pci_vector_release_one(proxy, queue_no); > > + } > > + /* Set the new vector on the device. */ > > + if (queue_no == VIRTIO_CONFIG_IRQ_IDX) { > > + vdev->config_vector = new_vector; > > + } else { > > + virtio_queue_set_vector(vdev, queue_no, new_vector); > > + } > > + /* If the new vector changed need to set it up. */ > > + if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR) { > > + kvm_virtio_pci_vector_use_one(proxy, queue_no); > > + } > > +} > > + > > int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, > > uint8_t bar, uint64_t offset, uint64_t length, > > uint8_t id) > > @@ -1570,7 +1598,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, > > } else { > > val = VIRTIO_NO_VECTOR; > > } > > - vdev->config_vector = val; > > + vector = vdev->config_vector; > > + /* Check if we need to change the vector. */ > > + if (val != vector) { > > + virtio_pci_set_and_change_vector(vdev, proxy, VIRTIO_CONFIG_IRQ_IDX, > > + vector, val); > > + } > > break;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index cb6940fc0e..e9edd57339 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1424,6 +1424,34 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, return offset; } +static void virtio_pci_set_and_change_vector(VirtIODevice *vdev, + VirtIOPCIProxy *proxy, + int queue_no, uint16_t old_vector, + uint16_t new_vector) +{ + bool kvm_irqfd = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && + msix_enabled(&proxy->pci_dev) && kvm_msi_via_irqfd_enabled(); + + /* + * If the device uses irqfd and the vector changes after DRIVER_OK is + * set, we need to release the old vector and set up the new one. + * Otherwise just need to set the new vector on the device. + */ + if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR) { + kvm_virtio_pci_vector_release_one(proxy, queue_no); + } + /* Set the new vector on the device. */ + if (queue_no == VIRTIO_CONFIG_IRQ_IDX) { + vdev->config_vector = new_vector; + } else { + virtio_queue_set_vector(vdev, queue_no, new_vector); + } + /* If the new vector changed need to set it up. */ + if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR) { + kvm_virtio_pci_vector_use_one(proxy, queue_no); + } +} + int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, uint8_t bar, uint64_t offset, uint64_t length, uint8_t id) @@ -1570,7 +1598,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, } else { val = VIRTIO_NO_VECTOR; } - vdev->config_vector = val; + vector = vdev->config_vector; + /* Check if we need to change the vector. */ + if (val != vector) { + virtio_pci_set_and_change_vector(vdev, proxy, VIRTIO_CONFIG_IRQ_IDX, + vector, val); + } break; case VIRTIO_PCI_COMMON_STATUS: if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { @@ -1610,7 +1643,11 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, } else { val = VIRTIO_NO_VECTOR; } - virtio_queue_set_vector(vdev, vdev->queue_sel, val); + /* Check if we need to change the vector. */ + if (val != vector) { + virtio_pci_set_and_change_vector(vdev, proxy, vdev->queue_sel, + vector, val); + } break; case VIRTIO_PCI_COMMON_Q_ENABLE: if (val == 1) {