In case irqchip_change_notifier isn't added, removing it triggers segfault.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c71b0955d81c..82c4cf4f7609 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
 
 out_deregister:
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
-    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
+    if (vdev->irqchip_change_notifier.notify) {
+        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
+    }
 out_teardown:
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
-- 
2.34.1

On 21/06/2023 09:02, Zhenzhong Duan wrote:
> In case irqchip_change_notifier isn't added, removing it triggers segfault.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c71b0955d81c..82c4cf4f7609 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>  
>  out_deregister:
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    if (vdev->irqchip_change_notifier.notify) {
> +        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    }

If the first patch ends up being pursued (which I am not quite sure) it should
be folded in the previous patch, as the out_deregister is used starting your
patch 1.

>  out_teardown:
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Wednesday, June 21, 2023 7:09 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: alex.williamson@redhat.com; clg@redhat.com; qemu-devel@nongnu.org;
>avihaih@nvidia.com; Peng, Chao P <chao.p.peng@intel.com>
>Subject: Re: [PATCH v3 2/3] vfio/pci: Fix a segfault in vfio_realize
>
>
>
>On 21/06/2023 09:02, Zhenzhong Duan wrote:
>> In case irqchip_change_notifier isn't added, removing it triggers segfault.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  hw/vfio/pci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>> c71b0955d81c..82c4cf4f7609 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>> **errp)
>>
>>  out_deregister:
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>> -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
>> +    if (vdev->irqchip_change_notifier.notify) {
>> +        kvm_irqchip_remove_change_notifier(&vdev-
>>irqchip_change_notifier);
>> +    }
>
>If the first patch ends up being pursued (which I am not quite sure) it should
>be folded in the previous patch, as the out_deregister is used starting your
>patch 1.
Sorry for late response, just back from vacation.

out_deregister isn't only for vfio migration, there are some other jump sites to out_deregister in vfio_realize. Take below code for example:

    if (vdev->display_xres || vdev->display_yres) {
        if (vdev->dpy == NULL) {
            error_setg(errp, "xres and yres properties require display=on");
            goto out_deregister;
        }

I can reproduce a segmentation fault when hotplug a vfio device using below cmd:
(qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1 Connection closed by foreign host.

After fix:
(qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1
Error: vfio 0000:81:11.1: xres and yres properties require display=on
(qemu)

Thanks
Zhenzhong

On 25/06/2023 07:01, Duan, Zhenzhong wrote:
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Wednesday, June 21, 2023 7:09 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>> Cc: alex.williamson@redhat.com; clg@redhat.com; qemu-devel@nongnu.org;
>> avihaih@nvidia.com; Peng, Chao P <chao.p.peng@intel.com>
>> Subject: Re: [PATCH v3 2/3] vfio/pci: Fix a segfault in vfio_realize
>>
>>
>>
>> On 21/06/2023 09:02, Zhenzhong Duan wrote:
>>> In case irqchip_change_notifier isn't added, removing it triggers segfault.
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>  hw/vfio/pci.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>>> c71b0955d81c..82c4cf4f7609 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>>> **errp)
>>>
>>>  out_deregister:
>>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>> -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
>>> +    if (vdev->irqchip_change_notifier.notify) {
>>> +        kvm_irqchip_remove_change_notifier(&vdev-
>>> irqchip_change_notifier);
>>> +    }
>>
>> If the first patch ends up being pursued (which I am not quite sure) it should
>> be folded in the previous patch, as the out_deregister is used starting your
>> patch 1.
> Sorry for late response, just back from vacation.
> 
> out_deregister isn't only for vfio migration, there are some other jump sites to out_deregister in vfio_realize. Take below code for example:
> 
>     if (vdev->display_xres || vdev->display_yres) {
>         if (vdev->dpy == NULL) {
>             error_setg(errp, "xres and yres properties require display=on");
>             goto out_deregister;
>         }
> 
> I can reproduce a segmentation fault when hotplug a vfio device using below cmd:
> (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1 Connection closed by foreign host.
> 
> After fix:
> (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1
> Error: vfio 0000:81:11.1: xres and yres properties require display=on
> (qemu)
> 

Makes sense. Let's keep it separate then.

> Thanks
> Zhenzhong
> 
>