[v1] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

[PATCH] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

Posted by Thomas Huth 3 weeks, 2 days ago

From: Thomas Huth <thuth@redhat.com>

The elements that get removed with QTAILQ_REMOVE are never referenced
afterwards anymore, so the corresponding memory should get freed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/s390-pci-bus.c  | 1 +
 hw/s390x/s390-pci-vfio.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 4de7b587e8a..d45e08a69fd 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -911,6 +911,7 @@ static void s390_pcihost_unrealize(DeviceState *dev)
     while (!QTAILQ_EMPTY(&s->zpci_groups)) {
         group = QTAILQ_FIRST(&s->zpci_groups);
         QTAILQ_REMOVE(&s->zpci_groups, group, link);
+        g_free(group);
     }
 }
 
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index 7c754b656da..db6de00bd28 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -101,6 +101,7 @@ void s390_pci_end_dma_count(S390pciState *s, S390PCIDMACount *cnt)
     cnt->users--;
     if (cnt->users == 0) {
         QTAILQ_REMOVE(&s->zpci_dma_limit, cnt, link);
+        g_free(cnt);
     }
 }
 
-- 
2.54.0

Re: [PATCH] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

Posted by Farhan Ali 3 weeks, 2 days ago

On 5/6/2026 10:41 AM, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
>
> The elements that get removed with QTAILQ_REMOVE are never referenced
> afterwards anymore, so the corresponding memory should get freed.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c  | 1 +
>   hw/s390x/s390-pci-vfio.c | 1 +
>   2 files changed, 2 insertions(+)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 4de7b587e8a..d45e08a69fd 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -911,6 +911,7 @@ static void s390_pcihost_unrealize(DeviceState *dev)
>       while (!QTAILQ_EMPTY(&s->zpci_groups)) {
>           group = QTAILQ_FIRST(&s->zpci_groups);
>           QTAILQ_REMOVE(&s->zpci_groups, group, link);
> +        g_free(group);
>       }
>   }
>   
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index 7c754b656da..db6de00bd28 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -101,6 +101,7 @@ void s390_pci_end_dma_count(S390pciState *s, S390PCIDMACount *cnt)
>       cnt->users--;
>       if (cnt->users == 0) {
>           QTAILQ_REMOVE(&s->zpci_dma_limit, cnt, link);
> +        g_free(cnt);
>       }
>   }
>   

Reviewed-by: Farhan Ali<alifm@linux.ibm.com>

Re: [PATCH] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

Posted by Matthew Rosato 3 weeks, 2 days ago

On 5/6/26 1:41 PM, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The elements that get removed with QTAILQ_REMOVE are never referenced
> afterwards anymore, so the corresponding memory should get freed.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c  | 1 +
>  hw/s390x/s390-pci-vfio.c | 1 +
>  2 files changed, 2 insertions(+)
> 

Thanks Thomas!  Code looks good and I did some regression testing with
this patch applied:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>

Should we also consider adding:

Fixes: b354d5d804 ("s390x/pci: clean up s390 PCI groups")
Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")

?   Neither are commonly-driven paths, but s390_pci_end_dma_count can be
driven N times via unplug of N vfio-pci devices.

Also out of curiosity, did you just stumble on this or was there some
tooling or testing that was being employed here that found this?

Thanks,
Matt

Re: [PATCH] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

Posted by Thomas Huth 3 weeks, 2 days ago

On 06/05/2026 21.45, Matthew Rosato wrote:
> On 5/6/26 1:41 PM, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> The elements that get removed with QTAILQ_REMOVE are never referenced
>> afterwards anymore, so the corresponding memory should get freed.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/s390x/s390-pci-bus.c  | 1 +
>>   hw/s390x/s390-pci-vfio.c | 1 +
>>   2 files changed, 2 insertions(+)
>>
> 
> Thanks Thomas!  Code looks good and I did some regression testing with
> this patch applied:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> Should we also consider adding:
> 
> Fixes: b354d5d804 ("s390x/pci: clean up s390 PCI groups")
> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")

Yes, that makes sense.

Actually, looking at the commit description of b354d5d804, I think this code 
has been added with a wrong assumption: The unrealize method is not called 
during system reset.
So to free the memory, this should likely be added to a reset handler 
instead? Could you maybe have a look?

> Also out of curiosity, did you just stumble on this or was there some
> tooling or testing that was being employed here that found this?

I asked Claude Code to find bugs in the QEMU sources, and this was something 
that looked reasonable to me. (I only used Claude to find bugs, the patch 
has been 100% written by myself of course)

  Thomas

Re: [PATCH] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

Posted by Matthew Rosato 3 weeks ago

On 5/7/26 3:31 AM, Thomas Huth wrote:
> On 06/05/2026 21.45, Matthew Rosato wrote:
>> On 5/6/26 1:41 PM, Thomas Huth wrote:
>>> From: Thomas Huth <thuth@redhat.com>
>>>
>>> The elements that get removed with QTAILQ_REMOVE are never referenced
>>> afterwards anymore, so the corresponding memory should get freed.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/s390x/s390-pci-bus.c  | 1 +
>>>   hw/s390x/s390-pci-vfio.c | 1 +
>>>   2 files changed, 2 insertions(+)
>>>
>>
>> Thanks Thomas!  Code looks good and I did some regression testing with
>> this patch applied:
>>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>
>> Should we also consider adding:
>>
>> Fixes: b354d5d804 ("s390x/pci: clean up s390 PCI groups")
>> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")
> 
> Yes, that makes sense.
> 
> Actually, looking at the commit description of b354d5d804, I think this
> code has been added with a wrong assumption: The unrealize method is not
> called during system reset.

So, yes, you're right that this isn't doing what was intended per the
commit message...

> So to free the memory, this should likely be added to a reset handler
> instead? Could you maybe have a look?

But I also don't think that we really want to arbitrarily clear this
list on a machine reset.

As written, it's only OK to free this list arbitrarily if we are 1)
ending the process anyway or 2) can expect another call to
s390_pcihost_realize() + device plug (s390_pcihost_plug) events for all
devices in order to re-establish new group pointers.

When we do, say, a reboot or kexec in the guest, because the associated
devices did not go through an unplug they continue to use the reference
to their pci_group which should be OK -- the underlying vfio device will
still give the same group data post-reset.  (Or, if it's an emulated
device, it will today always use the same QEMU-defined group)

I think the only issue is that when we do actually unplug vfio-pci
devices, we can potentially orphan groups in the list that are no longer
used.  Fortunately there is an upper-limit of 256 entries; while the
S390PCIGroup supports a full int, s390-pci-vfio.c will base all group
IDs from a uint8_t.
But it does leave a theoretical issue of what if the device is also
unplugged from the host, and a new device later comes along that re-uses
the host group ID with new group info, and then you pass that device
through to this guest.

So: I wonder if the proper thing to do is to refcount the groups during
plug and remove them from the list during unplug when refcount hits 0
(and of course, add the missing g_free here) -- with special handling
for the default group that should never go away.

Which is probably more than you signed on for with this patch ;) If
you'd like, I can instead sort this out and give you a reported-by?

Meanwhile you could still split out the fix to s390_pci_end_dma_count()
and send that one again by itself (w/ Fixes: 37fa32de70)

Thanks,
Matt

Re: [PATCH] hw/s390x: Fix memory leaks after removing elements with QTAILQ_REMOVE

Posted by Thomas Huth 3 weeks ago

On 08/05/2026 19.25, Matthew Rosato wrote:
> On 5/7/26 3:31 AM, Thomas Huth wrote:
>> On 06/05/2026 21.45, Matthew Rosato wrote:
>>> On 5/6/26 1:41 PM, Thomas Huth wrote:
>>>> From: Thomas Huth <thuth@redhat.com>
>>>>
>>>> The elements that get removed with QTAILQ_REMOVE are never referenced
>>>> afterwards anymore, so the corresponding memory should get freed.
...
>>> Thanks Thomas!  Code looks good and I did some regression testing with
>>> this patch applied:
>>>
>>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>
>>> Should we also consider adding:
>>>
>>> Fixes: b354d5d804 ("s390x/pci: clean up s390 PCI groups")
>>> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")
>>
>> Yes, that makes sense.
>>
>> Actually, looking at the commit description of b354d5d804, I think this
>> code has been added with a wrong assumption: The unrealize method is not
>> called during system reset.
...
> So: I wonder if the proper thing to do is to refcount the groups during
> plug and remove them from the list during unplug when refcount hits 0
> (and of course, add the missing g_free here) -- with special handling
> for the default group that should never go away.
> 
> Which is probably more than you signed on for with this patch ;) If
> you'd like, I can instead sort this out and give you a reported-by?

Yes, sounds good!

> Meanwhile you could still split out the fix to s390_pci_end_dma_count()
> and send that one again by itself (w/ Fixes: 37fa32de70)

Sure, I'll send a v2 that contains only the other change.

  Thomas