Re: [PATCH] virtio-pci: add check for vdev in virtio_pci_isr_read

[Philippe Mathieu-Daudé]

Hi Yuri,

On 2/16/21 6:29 AM, Yuri Benditovich wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1743098

Maybe add backtrace in patch description?

(gdb) bt
#0  0xc5bbdf0d in virtio_pci_notify_write (opaque=0x55b6c6dff170,
addr=0, val=<...>, size=<...>)
    at hw/virtio/virtio-pci.c:1360
#1  0xc59fe596 in memory_region_write_accessor (mr=<...>, addr=<...>,
value=<...>, size=<...>, shift=<...>, mask=<...>, attrs=...)
    at memory.c:530
#2  0xc59fc9e6 in access_with_adjusted_size (addr=addr@entry=0,
value=..., size=size@entry=2, access_size_min=<...>,
access_size_max=<...>, access_fn=<memory_region_write_accessor>,
mr=0x55b6c6df7cd0, attrs=...)
    at memory.c:597
#3  0xc5a0084a in memory_region_dispatch_write (mr=0x55b6c6df7cd0,
addr=0, data=<...>, size=2, attrs=...)
    at memory.c:1474
#4  0xc59aebbc in flatview_write (fv=0x7f1abc0407c0, addr=<...>,
attrs=..., buf=<...>, len=<...>)
    at exec.c:3099
#5  0xc59b3243 in address_space_write (as=<...>, addr=<...>, attrs=...,
buf=<...>, len=<...>)
    at exec.c:3265
#6  0xc5a0f878 in kvm_cpu_exec (cpu=<...>)
    at accel/kvm/kvm-all.c:2004
#7  0xc59ec21e in qemu_kvm_cpu_thread_fn (arg=0x55b6c6bdddd0)
    at cpus.c:1215
#8  0x00007f1adb4732de in start_thread (arg=<...>) at pthread_create.c:486
#9  0x00007f1adb1a4133 in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

> There is missing check for vdev in this procedure.
> QEMU crash happens in it in hot unplug flow.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  hw/virtio/virtio-pci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 094c36aa3e..2f19301267 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1364,7 +1364,13 @@ static uint64_t virtio_pci_isr_read(void *opaque, 
> hwaddr addr,
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> -    uint64_t val = qatomic_xchg(&vdev->isr, 0);
> +    uint64_t val = 0;

       uint64_t val;

> +
> +    if (vdev == NULL) {
> +        return val;

           return 0;

> +    }
> +
> +    val = qatomic_xchg(&vdev->isr, 0);
>      pci_irq_deassert(&proxy->pci_dev);
>  
>      return val;
> 

This pattern is present in other functions of this file.

Can you fix them too, or add an assert() if it is unlikely
to happen?

Thanks,

Phil.