Re: [PATCH 3/3] softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions

[Philippe Mathieu-Daudé]

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"

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().

      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>