Re: QEMU device refcounting when device creates a container MR

[Igor Mammedov]

On Thu, 10 Mar 2022 17:11:14 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 10 Mar 2022 at 16:30, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > Do On Thu, 10 Mar 2022 16:05:24 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >  
> > > On Thu, 10 Mar 2022 at 15:36, Igor Mammedov <imammedo@redhat.com> wrote:  
> > > >
> > > > On Wed, 9 Mar 2022 16:56:21 +0000
> > > > Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > > > ...also, in the device-introspect-test where I see this problem,
> > > > > unrealize is never going to be called anyway, because the device
> > > > > is only put through "instance_init" and then dereffed (which
> > > > > does not result in instance_finalize being called, because the
> > > > > refcount is still non-zero).  
> > > >
> > > > question is why introspected device is deferred instead of being
> > > > destroyed if it's no longer needed?  
> > >
> > > ...because the reference count is not zero.
> > >
> > > What is supposed to happen is:
> > >  * device is created (inited), and has refcount of 1
> > >  * introspection code does its thing
> > >  * introspection code derefs the device, and it gets deinited
> > >
> > > This bug means that when the device is inited it has a refcount
> > > that is too high, and so despite the code that creates it
> > > correctly dereffing it, it's still lying around.  
> >
> > looks like ref count leak somewhere, instance_finalize() take care
> > of cleaning up instance_init() actions.  
> 
> If you read the rest of the thread, we know why the refcount
> is too high. And instance_finalize *is never called*, so it
> cannot clean up what instance_init has done.
> 
> > Do you have an example/reproducer?  
> 
> Yes, see the thread -- device-introspect-test shows it.
> (You can put printfs in ehci_sysbus_init and ehci_sysbus_finalize
> and see that for some devices we don't ever call finalize.)

something like following might work.

basic idea is avoid cyclic references when subregion and container
have the same owner.
And properly handle references of subregion itsef when it's added to container,
this is necessary to prevent subregion being freed (when it's removed as a 
child property)
since container might still exist and 'referencing' subregion.
So that later when container is finalized it would call del_region()
on still alive subregion.


diff --git a/softmmu/memory.c b/softmmu/memory.c
index 8060c6de78..499c20fcef 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2527,8 +2527,11 @@ static void 
memory_region_update_container_subregions(MemoryRegion *subregion)
     MemoryRegion *other;
 
     memory_region_transaction_begin();
+    object_ref(subregion);
 
-    memory_region_ref(subregion);
+    if (subregion->container->owner != subregion->owner) {
+        memory_region_ref(subregion);
+    }
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2580,14 +2583,17 @@ void memory_region_del_subregion(MemoryRegion *mr,
 
     memory_region_transaction_begin();
     assert(subregion->container == mr);
-    subregion->container = NULL;
     for (alias = subregion->alias; alias; alias = alias->alias) {
         alias->mapped_via_alias--;
         assert(alias->mapped_via_alias >= 0);
     }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-    memory_region_unref(subregion);
+    if (subregion->container->owner != subregion->owner) {
+        memory_region_unref(subregion);
+    }
+    subregion->container = NULL;
     memory_region_update_pending |= mr->enabled && subregion->enabled;
+    object_unref(subregion);
     memory_region_transaction_commit();
 }

> 
> -- PMM
>