* Re: [PATCH] virtio-pci: fix memory leak from device realization failure
2025-02-28 9:24 ` Philippe Mathieu-Daudé
@ 2025-03-03 7:41 ` Zheng Huang
2025-03-10 9:04 ` Zheng Huang
1 sibling, 0 replies; 4+ messages in thread
From: Zheng Huang @ 2025-03-03 7:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel
Hi Philippe,
On 2025/2/28 17:24, Philippe Mathieu-Daudé wrote:
> Hi Zheng,
>
> On 28/2/25 06:03, Zheng Huang wrote:
>> This commit adds failback routine for `virtio_pci_realize` to
>> fix the memory leak of an address space and the virtio-net device object.
>> If the realization of the device failed, the address space should be
>> destroyed too.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2845
>>
>> Signed-off-by: Zheng Huang <hz1624917200@outlook.com>
>>
>> ---
>> hw/virtio/virtio-pci.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index c773a9130c..4b0d8cd90a 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2266,6 +2266,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>> virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
>> if (k->realize) {
>> k->realize(proxy, errp);
>> + if (*errp) {
>> + address_space_destroy(&proxy->modern_cfg_mem_as);
>> + }
>> }
>> }
>>
>
> I think instead we want to add an instance_init in virtio_pci_class_init
> and move the address_space_init call from virtio_pci_realize there.
>
> Regards,
>
> Phil.
I have reviewed the relevant code again and found that if address_space_init
is moved into instance_init, it will not be able to take follow-up actions
such as free the AS if device realization failed, thus failing to address the
issue. Additionally, I referred to the code for AS initialization and
destruction in other devices and found that they are managed in device
realize and unrealize handlers. Therefore, I still believe the previous
approach is a better choice.
If there are other potential solutions or considerations that I might have
missed, please let me know. I'm looking forward to hearing your thoughts!
Best regards,
Zheng.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] virtio-pci: fix memory leak from device realization failure
2025-02-28 9:24 ` Philippe Mathieu-Daudé
2025-03-03 7:41 ` Zheng Huang
@ 2025-03-10 9:04 ` Zheng Huang
1 sibling, 0 replies; 4+ messages in thread
From: Zheng Huang @ 2025-03-10 9:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel
Hi Philippe,
On 2025/2/28 17:24, Philippe Mathieu-Daudé wrote:
> Hi Zheng,
>
> On 28/2/25 06:03, Zheng Huang wrote:
>> This commit adds failback routine for `virtio_pci_realize` to
>> fix the memory leak of an address space and the virtio-net device object.
>> If the realization of the device failed, the address space should be
>> destroyed too.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2845
>>
>> Signed-off-by: Zheng Huang <hz1624917200@outlook.com>
>>
>> ---
>> hw/virtio/virtio-pci.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index c773a9130c..4b0d8cd90a 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2266,6 +2266,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>> virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
>> if (k->realize) {
>> k->realize(proxy, errp);
>> + if (*errp) {
>> + address_space_destroy(&proxy->modern_cfg_mem_as);
>> + }
>> }
>> }
>>
>
> I think instead we want to add an instance_init in virtio_pci_class_init
> and move the address_space_init call from virtio_pci_realize there.
>
> Regards,
>
> Phil.
I have reviewed the relevant code again and found that if address_space_init
is moved into instance_init, it will not be able to take follow-up actions
such as free the AS if device realization failed, thus failing to address the
issue. Additionally, I referred to the code for AS initialization and
destruction in other devices and found that they are managed in device
realize and unrealize handlers. Therefore, I still believe the previous
approach is a better choice.
If there are other potential solutions or considerations that I might have
missed, please let me know. I'm looking forward to hearing your thoughts!
Sorry to bother you again, but I wanted to follow up on my previous email. Apologize
if this is inconvenient
Best regards,
Zheng.
^ permalink raw reply [flat|nested] 4+ messages in thread