virtio-pci: fix memory leak from device realization failure

[PATCH] virtio-pci: fix memory leak from device realization failure

Posted by Zheng Huang 11 months, 2 weeks ago

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);
+        }
     }
 }
 
-- 
2.34.1

Re: [PATCH] virtio-pci: fix memory leak from device realization failure

Posted by Philippe Mathieu-Daudé 11 months, 2 weeks ago

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.

Re: [PATCH] virtio-pci: fix memory leak from device realization failure

Posted by Zheng Huang 11 months ago

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.

Re: [PATCH] virtio-pci: fix memory leak from device realization failure

Posted by Zheng Huang 11 months, 1 week ago

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.