Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Tue, 16 Apr 2024 at 14:06, Cindy Lu wrote: > > On Tue, Apr 16, 2024 at 8:22 PM Peter Maydell > 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
Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Tue, Apr 16, 2024 at 8:22 PM Peter Maydell wrote: > > On Tue, 16 Apr 2024 at 12:50, Peter Maydell wrote: > > > > On Tue, 16 Apr 2024 at 12:05, Cindy Lu wrote: > > > > > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell > > > 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 >
Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Tue, Apr 16, 2024 at 7:50 PM Peter Maydell wrote: > > On Tue, 16 Apr 2024 at 12:05, Cindy Lu wrote: > > > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell > > 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
Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Tue, 16 Apr 2024 at 12:50, Peter Maydell wrote: > > On Tue, 16 Apr 2024 at 12:05, Cindy Lu wrote: > > > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell > > 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
Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Tue, 16 Apr 2024 at 12:05, Cindy Lu wrote: > > On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell > 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
Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Tue, Apr 16, 2024 at 6:01 PM Peter Maydell wrote: > > On Mon, 15 Apr 2024 at 11:52, Michael S. Tsirkin wrote: > > > > From: Cindy Lu > > > > 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(>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 >
Re: [PULL 1/1] virtio-pci: fix use of a released vector
On Mon, 15 Apr 2024 at 11:52, Michael S. Tsirkin wrote: > > From: Cindy Lu > > 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(>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