Re: [PATCH v3] virtio-pci: Fix the use of an uninitialized irqfd

[Cindy Lu]

On Wed, Jun 26, 2024 at 3:45 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jun 26, 2024 at 10:44:31AM +0800, Cindy Lu wrote:
> > The crash was reported in MAC OS and NixOS, here is the link for this bug
> > https://gitlab.com/qemu-project/qemu/-/issues/2334
> > https://gitlab.com/qemu-project/qemu/-/issues/2321
> >
> > The root cause is the function virtio_pci_set_guest_notifiers() was not 
> > called
> > in the virtio_input device.So the vector_irqfd was not initialized
> >
> > So the fix is add the check for vector_irqfd.
> >
> > Change in V3
>
> Changelog belongs after ---
>
thanks micheal, will fix this
> > 1) Move the vector_irqfd check to virtio_pci_get_notifier().
> > The function virtio_pci_get_notifier() can also be used while vdev->status
> > is not VIRTIO_CONFIG_S_DRIVER_OK. In that case, the vector_irqfd could be 
> > NULL,
> > so also add the status check here.
> > 2) Add the return value check for kvm_virtio_pci_vector_use_one().
> > Since the return value of function virtio_pci_set_vector() is void,
> > just add the error message here.
> >
> > This fix is verified in vyatta,MacOS,NixOS,fedora system.
> >
> > The bt tree for this bug is:
> > Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7c817be006c0 (LWP 1269146)]
> > kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
> > 817         if (irqfd->users == 0) {
> > (gdb) thread apply all bt
> > ...
> > Thread 6 (Thread 0x7c817be006c0 (LWP 1269146) "CPU 0/KVM"):
> > 0  kvm_virtio_pci_vq_vector_use () at 
> > ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
> > 1  kvm_virtio_pci_vector_use_one () at 
> > ../qemu-9.0.0/hw/virtio/virtio-pci.c:893
> > 2  0x00005983657045e2 in memory_region_write_accessor () at 
> > ../qemu-9.0.0/system/memory.c:497
> > 3  0x0000598365704ba6 in access_with_adjusted_size () at 
> > ../qemu-9.0.0/system/memory.c:573
> > 4  0x0000598365705059 in memory_region_dispatch_write () at 
> > ../qemu-9.0.0/system/memory.c:1528
> > 5  0x00005983659b8e1f in flatview_write_continue_step.isra.0 () at 
> > ../qemu-9.0.0/system/physmem.c:2713
> > 6  0x000059836570ba7d in flatview_write_continue () at 
> > ../qemu-9.0.0/system/physmem.c:2743
> > 7  flatview_write () at ../qemu-9.0.0/system/physmem.c:2774
> > 8  0x000059836570bb76 in address_space_write () at 
> > ../qemu-9.0.0/system/physmem.c:2894
> > 9  0x0000598365763afe in address_space_rw () at 
> > ../qemu-9.0.0/system/physmem.c:2904
> > 10 kvm_cpu_exec () at ../qemu-9.0.0/accel/kvm/kvm-all.c:2917
> > 11 0x000059836576656e in kvm_vcpu_thread_fn () at 
> > ../qemu-9.0.0/accel/kvm/kvm-accel-ops.c:50
> > 12 0x0000598365926ca8 in qemu_thread_start () at 
> > ../qemu-9.0.0/util/qemu-thread-posix.c:541
> > 13 0x00007c8185bcd1cf in ??? () at /usr/lib/libc.so.6
> > 14 0x00007c8185c4e504 in clone () at /usr/lib/libc.so.6
> >
> > Fixes: 2ce6cff94d ("virtio-pci: fix use of a released vector")
> > Cc: qemu-stable@nongnu.org
> >
>
> all trailers including s.o.b. should be together, with no empty lines
>
will change this
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index b1d02f4b3d..87307b9061 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -860,6 +860,11 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy 
> > *proxy, int queue_no,
> >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >      VirtQueue *vq;
> >
> > +    if ((proxy->vector_irqfd == NULL) &&
>
> Preferable:
>         if (!proxy->vector_irqfd &&
>
> and brackets not really needed here
>
>
> > +        (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>
> brackets not really needed here
>
sure, will fix this
> > +        return -1;
> > +    }
> > +
> >      if (queue_no == VIRTIO_CONFIG_IRQ_IDX) {
> >          *n = virtio_config_get_guest_notifier(vdev);
> >          *vector = vdev->config_vector;
> > @@ -1452,7 +1457,9 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
> >      }
> >      /* 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);
> > +        if (kvm_virtio_pci_vector_use_one(proxy, queue_no)) {
> > +            virtio_error(vdev, "fail to set the vector %d", new_vector);
> > +        }
> >      }
> >  }
>
>
> Is this an unrelated fix?
>
this is for last version's comment, there is an return value for
function kvm_virtio_pci_vector_use_one, but
we can do nothing here, so I just add an erorr message
Thanks
cindy
> >
> > --
> > 2.45.0
>