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
