On 12/5/23 08:56, Mark Cave-Ayland wrote:
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.
Right.
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.
Sure, I'm not asking for change on this series.
Possibly the maintainer taking this can fix the typo.
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.
