Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Xu <peterx@redhat.com>]

On Thu, Aug 21, 2025 at 01:45:20PM +0100, Peter Maydell wrote:
> (updated cc list with Akihiko's new email address)
> 
> On Thu, 21 Aug 2025 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Fri, 10 Jan 2025 at 09:20, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > >
> > > memory_region_update_container_subregions() used to call
> > > memory_region_ref(), which creates a reference to the owner of the
> > > subregion, on behalf of the owner of the container. This results in a
> > > circular reference if the subregion and container have the same owner.
> > >
> > > memory_region_ref() creates a reference to the owner instead of the
> > > memory region to match the lifetime of the owner and memory region. We
> > > do not need such a hack if the subregion and container have the same
> > > owner because the owner will be alive as long as the container is.
> > > Therefore, create a reference to the subregion itself instead ot its
> > > owner in such a case; the reference to the subregion is still necessary
> > > to ensure that the subregion gets finalized after the container.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > Hi; I came back to this patchset because it was never applied,
> > and so we still have various leaks of MemoryRegion objects
> > in devices which use the "device owns a container MemoryRegion C
> > and puts another MemoryRegion X which it owns into that container"
> > pattern.
> >
> > Unfortunately, with this patch applied, I see a segfault when running
> > the device-introspect-test for Arm:
> >
> > $ (cd build/x86 && QTEST_QEMU_BINARY=./qemu-system-arm
> > ./tests/qtest/device-introspect-test -p
> > /arm/device/introspect/concrete/defaults/none)
> > [...]
> > # Testing device 'imx7.analog'
> > Broken pipe
> > ../../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> > signal 11 (Segmentation fault) (core dumped)
> >
> > Here's a backtrace collected with gdb:
> >
> > $ (cd build/x86 && QTEST_QEMU_BINARY="xterm -e gdb --tty $(tty) --args
> > ./qemu-system-arm" ./tests/qtest/device-introspect-test -p
> > /arm/device/introspect/concrete/defaults/none)
> > [hit 'r' in the gdb prompt in the new window]
> >
> > Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
> > memory_region_unref_subregion (subregion=0x555557d58370) at
> > ../../system/memory.c:1862
> > 1862        if (subregion->container->owner == subregion->owner) {
> > (gdb) bt
> > #0  memory_region_unref_subregion (subregion=0x555557d58370) at
> > ../../system/memory.c:1862
> > #1  0x0000555555d5f97b in memory_region_del_subregion
> > (mr=0x555557d58150, subregion=0x555557d58370)
> >     at ../../system/memory.c:2705
> > #2  0x0000555555d5cc1c in memory_region_finalize (obj=0x555557d58150)
> > at ../../system/memory.c:1811
> > #3  0x0000555556197595 in object_deinit (obj=0x555557d58150,
> > type=0x5555579d2ea0) at ../../qom/object.c:715
> > #4  0x0000555556197613 in object_finalize (data=0x555557d58150) at
> > ../../qom/object.c:729
> > #5  0x00005555561988f9 in object_unref (objptr=0x555557d58150) at
> > ../../qom/object.c:1232
> > #6  0x000055555619a3d6 in object_finalize_child_property
> >     (obj=0x555557d57e20, name=0x555557c59810 "imx7.analog[0]",
> > opaque=0x555557d58150) at ../../qom/object.c:1818
> > #7  0x0000555556197344 in object_property_del_all (obj=0x555557d57e20)
> > at ../../qom/object.c:667
> > #8  0x0000555556197600 in object_finalize (data=0x555557d57e20) at
> > ../../qom/object.c:728
> > #9  0x00005555561988f9 in object_unref (objptr=0x555557d57e20) at
> > ../../qom/object.c:1232
> > #10 0x00005555562f4600 in qmp_device_list_properties
> > (typename=0x555557d3ee70 "imx7.analog", errp=0x7fffffffdc00)
> >     at ../../qom/qom-qmp-cmds.c:237
> > #11 0x00005555563981ba in qmp_marshal_device_list_properties
> > (args=0x7fffe00031f0, ret=0x7fffef6afda8, errp=0x7fffef6afda0)
> >     at qapi/qapi-commands-qdev.c:65
> > #12 0x00005555563bcb53 in do_qmp_dispatch_bh (opaque=0x7fffef6afe40)
> > at ../../qapi/qmp-dispatch.c:128
> > #13 0x00005555563ed87f in aio_bh_call (bh=0x555557d3f540) at
> > ../../util/async.c:172
> > #14 0x00005555563ed9cb in aio_bh_poll (ctx=0x555557a00770) at
> > ../../util/async.c:219
> > #15 0x00005555563cd517 in aio_dispatch (ctx=0x555557a00770) at
> > ../../util/aio-posix.c:436
> > #16 0x00005555563edebe in aio_ctx_dispatch (source=0x555557a00770,
> > callback=0x0, user_data=0x0) at ../../util/async.c:361
> > #17 0x00007ffff6e965c5 in ??? () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #18 0x00007ffff6e96710 in g_main_context_dispatch () at
> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #19 0x00005555563ef89e in glib_pollfds_poll () at ../../util/main-loop.c:287
> > #20 0x00005555563ef930 in os_host_main_loop_wait (timeout=0) at
> > ../../util/main-loop.c:310
> > #21 0x00005555563efa6b in main_loop_wait (nonblocking=0) at
> > ../../util/main-loop.c:589
> > #22 0x0000555555d7d181 in qemu_main_loop () at ../../system/runstate.c:905
> > #23 0x00005555562f94c7 in qemu_default_main (opaque=0x0) at
> > ../../system/main.c:50
> > #24 0x00005555562f9585 in main (argc=18, argv=0x7fffffffe078) at
> > ../../system/main.c:93
> >
> >
> > In memory_region_unref_subregion(), subregion->container is NULL.
> >
> > This is because in memory_region_del_subregion() we do:
> >
> >     subregion->container = NULL;
> >
> > and then after that we call
> >     memory_region_unref_subregion(subregion);
> > which dereferences subregion->container.
> >
> > Won't this always SEGV ?
> >
> > thanks
> > -- PMM
> 

Peter, could you try the v3 version patch 8/9 instead?

https://lore.kernel.org/all/20240708-san-v3-8-b03f671c40c6@daynix.com/

I still prefer that one, and I hope that one doesn't have this issue.

If no one objects, IMHO we could merge v3's patch 8/9, and v8's patch 1 (I
can touch up some commit messages and details, e.g. v3's 8/9 commit
message is a bit too simple to me).

Thanks,

-- 
Peter Xu