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
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(-)
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index 238625a36f..d89c659662 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -26,6 +26,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/rcu.h"
#include "cpu.h"
#include "exec/ioport.h"
#include "exec/memory.h"
@@ -152,8 +153,7 @@ void portio_list_destroy(PortioList *piolist)
for (i = 0; i < piolist->nr; ++i) {
mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
object_unparent(OBJECT(&mrpio->mr));
- g_free(mrpio->ports);
- g_free(mrpio);
+ object_unref(mrpio);
}
g_free(piolist->regions);
}
@@ -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));
+ 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);
+
if (piolist->flush_coalesced_mmio) {
memory_region_set_flush_coalesced(&mrpio->mr);
}
@@ -305,10 +324,19 @@ void portio_list_del(PortioList *piolist)
}
}
+static void memory_region_portio_list_finalize(Object *obj)
+{
+ MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
+
+ object_unref(&mrpio->mr);
+ g_free(mrpio->ports);
+}
+
static const TypeInfo memory_region_portio_list_info = {
.parent = TYPE_OBJECT,
.name = TYPE_MEMORY_REGION_PORTIO_LIST,
.instance_size = sizeof(MemoryRegionPortioList),
+ .instance_finalize = memory_region_portio_list_finalize,
};
static void ioport_register_types(void)
--
2.30.2
