On 11/05/2023 20:22, Philippe Mathieu-Daudé wrote:
On 19/4/23 17:16, Mark Cave-Ayland wrote:
Currently when portio_list MemoryRegions are freed using portio_list_destroy()
the RCU
thread segfaults generating a backtrace similar to that below:
#0 0x5555599a34b6 in phys_section_destroy ../softmmu/physmem.c:996
#1 0x5555599a37a3 in phys_sections_free ../softmmu/physmem.c:1011
#2 0x5555599b24aa in address_space_dispatch_free ../softmmu/physmem.c:2430
#3 0x55555996a283 in flatview_destroy ../softmmu/memory.c:292
#4 0x55555a2cb9fb in call_rcu_thread ../util/rcu.c:284
#5 0x55555a29b71d in qemu_thread_start ../util/qemu-thread-posix.c:541
#6 0x7ffff4a0cea6 in start_thread nptl/pthread_create.c:477
#7 0x7ffff492ca2e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfca2e)
The problem here is that portio_list_destroy() unparents the portio_list
MemoryRegions
causing them to be freed immediately, however the flatview still has a reference to
the
MemoryRegion and so causes a use-after-free segfault when the RCU thread next
updates
the flatview.
Solve the lifetime issue by making MemoryRegionPortioList the owner of the
portio_list
MemoryRegions, and then reparenting them to the portio_list owner. This ensures
that they
can be accessed as QOM childen via the portio_list owner, yet the
MemoryRegionPortioList
"children"
Ooops, thanks - I'll correct that on the next respin.
owns the refcount.
Update portio_list_destroy() to unparent the MemoryRegion from the portio_list
owner and
then add a finalize() method to MemoryRegionPortioList, so that the portio_list
MemoryRegions remain allocated until flatview_destroy() removes the final refcount
upon
the next flatview update.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
---
softmmu/ioport.c | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
@@ -230,6 +230,8 @@ static void portio_list_add_1(PortioList *piolist,
unsigned off_low, unsigned off_high)
{
MemoryRegionPortioList *mrpio;
+ Object *owner;
+ char *name;
unsigned i;
/* Copy the sub-list and null-terminate it. */
@@ -246,8 +248,25 @@ static void portio_list_add_1(PortioList *piolist,
mrpio->ports[i].base = start + off_low;
}
- memory_region_init_io(&mrpio->mr, piolist->owner, &portio_ops, mrpio,
+ /*
+ * The MemoryRegion owner is the MemoryRegionPortioList since that manages
+ * the lifecycle via the refcount
+ */
+ memory_region_init_io(&mrpio->mr, OBJECT(mrpio), &portio_ops, mrpio,
piolist->name, off_high - off_low);
+
+ /* Reparent the MemoryRegion to the piolist owner */
+ object_ref(&mrpio->mr);
+ object_unparent(OBJECT(&mrpio->mr));
Out of this patch scope, but could this part <...
+ if (!piolist->owner) {
+ owner = container_get(qdev_get_machine(), "/unattached");
+ } else {
+ owner = piolist->owner;
+ }
+ name = g_strdup_printf("%s[*]", piolist->name);
+ object_property_add_child(owner, name, OBJECT(&mrpio->mr));
+ g_free(name);
...> be extracted as qdev_add_child()? It seems to duplicate
code from device_set_realized().
I would say no for 2 reasons: firstly qdev itself only has the concept of devices and
buses: the fact that devices appear as children of their bus is an artifact of how
they are modelled in QOM, rather than being part of the qdev design philosophy.
Secondly there is actually only one user of portio_list which doesn't have an owner,
and that is our old friend the PCI IDE controller. That's because everything else is
done through isa_register_ioport(), and in fact we have both attempted to solve this
previously (see my comments at
https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-9-phi...@linaro.org/#ca177083-b24d-90cd-9f3c-c99653bc9...@ilande.co.uk
and
https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/#6bc0dc92-3787-5379-b139-a8b5973d8...@ilande.co.uk).
My preference would be to merge this in its current form and then remove the part
handling NULL piolist->owner and replace it with assert(piolist->owner) once one of
the PCI IDE controller/ISA ioport patches have been merged.
if (piolist->flush_coalesced_mmio) {
memory_region_set_flush_coalesced(&mrpio->mr);
}
@@ -305,10 +324,19 @@ void portio_list_del(PortioList *piolist)
}
}
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
ATB,
Mark.
