RE: [PATCH v3 1/3] vfio/pci: Fix resource leak in vfio_realize

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Tuesday, June 27, 2023 6:22 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 1/3] vfio/pci: Fix resource leak in vfio_realize
>
>>>>  out_deregister:
>>>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>>>      if (vdev->irqchip_change_notifier.notify) {
>>>>          kvm_irqchip_remove_change_notifier(&vdev-
>>irqchip_change_notifier);
>>>>      }
>>>> +    vfio_disable_interrupts(vdev);
>>>> +    if (vdev->intx.mmap_timer) {
>>>> +        timer_free(vdev->intx.mmap_timer);
>>>> +    }
>>>
>>> But this one suggests another one as it looks a pre-existing issue?
>> Yes, it's another resource leak I just found.
>> Not sure if it's better to also merge above change to this patch which
>> is targeting resource leak issues, or to PATCH2 which is targeting
>out_deregister path, or to create a new one.
>> Any suggestion?
>
>In general they are all bugs in the same deregistration path, but each resource
>is not being teardown correctly. I tend to prefer 'logical change' per commit,
>so there's would be a fix the irqchip_change notifier and another one for
>mmap_timer teardown. Both with the Fixes tags that introduced each bug.
>Unless everything was introduced by the same change in which case you
>would do everything in one patch.
Clear.

Thanks
Zhenzhong