Message ID | 2ce6cff94df2650c460f809e5ad263f1d22507c0.1713178348.git.mst@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [PULL,1/1] virtio-pci: fix use of a released vector | expand |
On Mon, 15 Apr 2024 at 11:52, Michael S. Tsirkin <mst@redhat.com> 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. > Hi; Coverity points out what it thinks is a problem in this commit (CID 1543938): > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index cb6940fc0e..cb159fd078 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1424,6 +1424,38 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, > return offset; > } > > +static void virtio_pci_set_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 (new_vector == old_vector) { > + return; > + } > + > + /* > + * 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); > + } Here queue_no can be VIRTIO_CONFIG_IRQ_IDX, which is -1. > + /* 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); Here we pass that through to kvm_virtio_pci_vector_use_one(). In kvm_virtio_pci_vector_use_one()'s error-exit path ("undo") it does vector = virtio_queue_vector(vdev, queue_no); and in virtio_queue_vector() it does: return n < VIRTIO_QUEUE_MAX ? vdev->vq[n].vector : VIRTIO_NO_VECTOR; where 'n' is an int, so if we can get here with queue_no being VIRTIO_CONFIG_IRQ_IDX then we'll index off the front of the vdev->vq[] array. Maybe this is a "can't happen" case, but it does seem odd that virtio_queue_vector() only bounds-checks the "too big" case for its argument and not the "too small" case and/or it doesn't have a special case for VIRTIO_CONFIG_IRQ_IDX. > + } > +} > + thanks -- PMM
On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 15 Apr 2024 at 11:52, Michael S. Tsirkin <mst@redhat.com> 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. > > > > Hi; Coverity points out what it thinks is a problem in this commit > (CID 1543938): > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index cb6940fc0e..cb159fd078 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1424,6 +1424,38 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, > > return offset; > > } > > > > +static void virtio_pci_set_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 (new_vector == old_vector) { > > + return; > > + } > > + > > + /* > > + * 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); > > + } > > Here queue_no can be VIRTIO_CONFIG_IRQ_IDX, which is -1. > > > + /* 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); > > Here we pass that through to kvm_virtio_pci_vector_use_one(). > In kvm_virtio_pci_vector_use_one()'s error-exit path ("undo") > it does > vector = virtio_queue_vector(vdev, queue_no); > and in virtio_queue_vector() it does: > > return n < VIRTIO_QUEUE_MAX ? vdev->vq[n].vector : > VIRTIO_NO_VECTOR; > > where 'n' is an int, so if we can get here with queue_no being > VIRTIO_CONFIG_IRQ_IDX then we'll index off the front of the > vdev->vq[] array. > > Maybe this is a "can't happen" case, but it does seem odd that > virtio_queue_vector() only bounds-checks the "too big" case > for its argument and not the "too small" case and/or it > doesn't have a special case for VIRTIO_CONFIG_IRQ_IDX. > > > + } > > +} > > + > hi peter I think we can simply remove the part vector = virtio_queue_vector(vdev, queue_no); the vector is get from virtio_pci_get_notifier() and don't need to get it again I will send the fix soon thanks cindy > thanks > -- PMM >
On Tue, 16 Apr 2024 at 12:05, Cindy Lu <lulu@redhat.com> wrote: > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > Here we pass that through to kvm_virtio_pci_vector_use_one(). > > In kvm_virtio_pci_vector_use_one()'s error-exit path ("undo") > > it does > > vector = virtio_queue_vector(vdev, queue_no); > > and in virtio_queue_vector() it does: > > > > return n < VIRTIO_QUEUE_MAX ? vdev->vq[n].vector : > > VIRTIO_NO_VECTOR; > > > > where 'n' is an int, so if we can get here with queue_no being > > VIRTIO_CONFIG_IRQ_IDX then we'll index off the front of the > > vdev->vq[] array. > > > > Maybe this is a "can't happen" case, but it does seem odd that > > virtio_queue_vector() only bounds-checks the "too big" case > > for its argument and not the "too small" case and/or it > > doesn't have a special case for VIRTIO_CONFIG_IRQ_IDX. > > > > > + } > > > +} > > > + > > > hi peter > I think we can simply remove the part > vector = virtio_queue_vector(vdev, queue_no); > the vector is get from virtio_pci_get_notifier() and don't need to get it again > I will send the fix soon The error handling in kvm_virtio_pci_vector_use_one() looks a bit odd in other ways, too. The only bit of "undoing" it does as far as I can see is calling kvm_virtio_pci_irqfd_release(), but there is no code path that gets to there where the main codepath's call to kvm_virtio-pci_irqfd_use() succeeded and needs to be undone. So perhaps the entire "undo" code block should be deleted, and the "goto undo" lines replaced by simple "return ret;" ? (The codepath for "kvm_virtio_pci_irqfd_use() failed" already does the "kvm_virtio_pci_vq_vector_release()" by hand there.) thanks -- PMM
On Tue, 16 Apr 2024 at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 16 Apr 2024 at 12:05, Cindy Lu <lulu@redhat.com> wrote: > > > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > Hi; Coverity points out what it thinks is a problem in > > > this commit (CID 1543938): > > > Here we pass that through to kvm_virtio_pci_vector_use_one(). > > > In kvm_virtio_pci_vector_use_one()'s error-exit path ("undo") > > > it does > > > vector = virtio_queue_vector(vdev, queue_no); > > > and in virtio_queue_vector() it does: > > > > > > return n < VIRTIO_QUEUE_MAX ? vdev->vq[n].vector : > > > VIRTIO_NO_VECTOR; > > > > > > where 'n' is an int, so if we can get here with queue_no being > > > VIRTIO_CONFIG_IRQ_IDX then we'll index off the front of the > > > vdev->vq[] array. > > > > > > Maybe this is a "can't happen" case, but it does seem odd that > > > virtio_queue_vector() only bounds-checks the "too big" case > > > for its argument and not the "too small" case and/or it > > > doesn't have a special case for VIRTIO_CONFIG_IRQ_IDX. > > > > > > > + } > > > > +} > > > > + > > > > > hi peter > > I think we can simply remove the part > > vector = virtio_queue_vector(vdev, queue_no); > > the vector is get from virtio_pci_get_notifier() and don't need to get it again > > I will send the fix soon > > The error handling in kvm_virtio_pci_vector_use_one() looks > a bit odd in other ways, too. The only bit of "undoing" > it does as far as I can see is calling kvm_virtio_pci_irqfd_release(), > but there is no code path that gets to there where the > main codepath's call to kvm_virtio-pci_irqfd_use() succeeded > and needs to be undone. So perhaps the entire "undo" code > block should be deleted, and the "goto undo" lines > replaced by simple "return ret;" ? (The codepath > for "kvm_virtio_pci_irqfd_use() failed" already does the > "kvm_virtio_pci_vq_vector_release()" by hand there.) In any case since the error handling in kvm_virtio_pci_vector_use_one() isn't new in this commit (you can get the same problem via kvm_virtio_pci_vector_config_use(), which is CID 1468940 first detected in 2022), I think this is not something we need to rush to fix before we release 9.0. If anybody disagrees now would be a good time to say so :-) Paolo's comment on CID 1468940 was to suggest "virtio_queue_vector should check VIRTIO_CONFIG_IRQ_IDX just like virtio_pci_get_notifier", incidentally. thanks -- PMM
On Tue, Apr 16, 2024 at 7:50 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 16 Apr 2024 at 12:05, Cindy Lu <lulu@redhat.com> wrote: > > > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > Here we pass that through to kvm_virtio_pci_vector_use_one(). > > > In kvm_virtio_pci_vector_use_one()'s error-exit path ("undo") > > > it does > > > vector = virtio_queue_vector(vdev, queue_no); > > > and in virtio_queue_vector() it does: > > > > > > return n < VIRTIO_QUEUE_MAX ? vdev->vq[n].vector : > > > VIRTIO_NO_VECTOR; > > > > > > where 'n' is an int, so if we can get here with queue_no being > > > VIRTIO_CONFIG_IRQ_IDX then we'll index off the front of the > > > vdev->vq[] array. > > > > > > Maybe this is a "can't happen" case, but it does seem odd that > > > virtio_queue_vector() only bounds-checks the "too big" case > > > for its argument and not the "too small" case and/or it > > > doesn't have a special case for VIRTIO_CONFIG_IRQ_IDX. > > > > > > > + } > > > > +} > > > > + > > > > > hi peter > > I think we can simply remove the part > > vector = virtio_queue_vector(vdev, queue_no); > > the vector is get from virtio_pci_get_notifier() and don't need to get it again > > I will send the fix soon > > The error handling in kvm_virtio_pci_vector_use_one() looks > a bit odd in other ways, too. The only bit of "undoing" > it does as far as I can see is calling kvm_virtio_pci_irqfd_release(), > but there is no code path that gets to there where the > main codepath's call to kvm_virtio-pci_irqfd_use() succeeded > and needs to be undone. So perhaps the entire "undo" code > block should be deleted, and the "goto undo" lines > replaced by simple "return ret;" ? (The codepath > for "kvm_virtio_pci_irqfd_use() failed" already does the > "kvm_virtio_pci_vq_vector_release()" by hand there.) > > thanks > -- PMM > Hi peter, Really thanks for your help. I have rewrite this part, the error handling part is incorrect Thanks Cindy
On Tue, Apr 16, 2024 at 8:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 16 Apr 2024 at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 16 Apr 2024 at 12:05, Cindy Lu <lulu@redhat.com> wrote: > > > > > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > Hi; Coverity points out what it thinks is a problem in > > > > this commit (CID 1543938): > > > > > Here we pass that through to kvm_virtio_pci_vector_use_one(). > > > > In kvm_virtio_pci_vector_use_one()'s error-exit path ("undo") > > > > it does > > > > vector = virtio_queue_vector(vdev, queue_no); > > > > and in virtio_queue_vector() it does: > > > > > > > > return n < VIRTIO_QUEUE_MAX ? vdev->vq[n].vector : > > > > VIRTIO_NO_VECTOR; > > > > > > > > where 'n' is an int, so if we can get here with queue_no being > > > > VIRTIO_CONFIG_IRQ_IDX then we'll index off the front of the > > > > vdev->vq[] array. > > > > > > > > Maybe this is a "can't happen" case, but it does seem odd that > > > > virtio_queue_vector() only bounds-checks the "too big" case > > > > for its argument and not the "too small" case and/or it > > > > doesn't have a special case for VIRTIO_CONFIG_IRQ_IDX. > > > > > > > > > + } > > > > > +} > > > > > + > > > > > > > hi peter > > > I think we can simply remove the part > > > vector = virtio_queue_vector(vdev, queue_no); > > > the vector is get from virtio_pci_get_notifier() and don't need to get it again > > > I will send the fix soon > > > > The error handling in kvm_virtio_pci_vector_use_one() looks > > a bit odd in other ways, too. The only bit of "undoing" > > it does as far as I can see is calling kvm_virtio_pci_irqfd_release(), > > but there is no code path that gets to there where the > > main codepath's call to kvm_virtio-pci_irqfd_use() succeeded > > and needs to be undone. So perhaps the entire "undo" code > > block should be deleted, and the "goto undo" lines > > replaced by simple "return ret;" ? (The codepath > > for "kvm_virtio_pci_irqfd_use() failed" already does the > > "kvm_virtio_pci_vq_vector_release()" by hand there.) > > In any case since the error handling in kvm_virtio_pci_vector_use_one() > isn't new in this commit (you can get the same problem via > kvm_virtio_pci_vector_config_use(), which is CID 1468940 > first detected in 2022), I think this is not something we need > to rush to fix before we release 9.0. If anybody disagrees now > would be a good time to say so :-) > > Paolo's comment on CID 1468940 was to suggest "virtio_queue_vector > should check VIRTIO_CONFIG_IRQ_IDX just like virtio_pci_get_notifier", > incidentally. > Hi peter, Really sorry all these mess, but I still have a stuipid question, where can I get this CID result ?maybe there are a mailing list?I just wonder maybe I can fix these code earlier next time, Really thanks for your help thanks cindy > thanks > -- PMM >
On Tue, 16 Apr 2024 at 14:06, Cindy Lu <lulu@redhat.com> wrote: > > On Tue, Apr 16, 2024 at 8:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > Paolo's comment on CID 1468940 was to suggest "virtio_queue_vector > > should check VIRTIO_CONFIG_IRQ_IDX just like virtio_pci_get_notifier", > > incidentally. > > > Hi peter, > Really sorry all these mess, but I still have a stuipid question, > where can I get > this CID result ?maybe there are a mailing list?I just wonder maybe I can fix > these code earlier next time, Really thanks for your help These CID numbers are the identifiers of the issues in the Coverity Scan online UI: https://scan.coverity.com/projects/qemu You need to apply for access but we give that to anybody who's a QEMU developer. Unfortunately we can only do a very limited amount of Coverity Scan runs (about one a day), so we run them only on code that has been committed to QEMU already, and we aren't able to use it to find problems in patches before we apply them. thanks -- PMM
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index cb6940fc0e..cb159fd078 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1424,6 +1424,38 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, return offset; } +static void virtio_pci_set_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 (new_vector == old_vector) { + return; + } + + /* + * 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 +1602,8 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, } else { val = VIRTIO_NO_VECTOR; } - vdev->config_vector = val; + virtio_pci_set_vector(vdev, proxy, VIRTIO_CONFIG_IRQ_IDX, + vdev->config_vector, val); break; case VIRTIO_PCI_COMMON_STATUS: if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { @@ -1610,7 +1643,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, } else { val = VIRTIO_NO_VECTOR; } - virtio_queue_set_vector(vdev, vdev->queue_sel, val); + virtio_pci_set_vector(vdev, proxy, vdev->queue_sel, vector, val); break; case VIRTIO_PCI_COMMON_Q_ENABLE: if (val == 1) {