(updated cc list with Akihiko's new email address)
On Thu, 21 Aug 2025 at 13:40, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Fri, 10 Jan 2025 at 09:20, Akihiko Odaki <akihiko.od...@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.od...@daynix.com>
> > Reviewed-by: Peter Xu <pet...@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
