[PATCH 0/3] softmmu/ioport.c: fix use-after-free when calling portio_list_destroy()

[PATCH 0/3] softmmu/ioport.c: fix use-after-free when calling portio_list_destroy()

[Mark Cave-Ayland]

When attempting to use portio_list_destroy() to remove a portio_list then the
QEMU process segfaults with the backtrace 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 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 which causes a use-after-free segfault when the RCU thread next updates
the flatview.

This series resolves the issue by QOMifying the MemoryRegionPortioList, and setting
that as the MemoryRegion owner instead of the portio_list owner. This allows the
MemoryRegionPortioList to hold the refcount for its MemoryRegion and so manually
finalize it when flatview_destroy() removes its final refcount.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (3):
  softmmu/ioport.c: allocate MemoryRegionPortioList ports on the heap
  softmmu/ioport.c: QOMify MemoryRegionPortioList
  softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list
    MemoryRegions

 softmmu/ioport.c | 62 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 7 deletions(-)

-- 
2.30.2

[PATCH 1/3] softmmu/ioport.c: allocate MemoryRegionPortioList ports on the heap

[Mark Cave-Ayland]

In order to facilitate a conversion of MemoryRegionPortioList to a QOM object
move the allocation of MemoryRegionPortioList ports to the heap instead of
using a variable-length member at the end of the MemoryRegionPortioList
structure.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 softmmu/ioport.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index cb8adb0b93..d0d5b0bcaa 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -35,7 +35,7 @@
 typedef struct MemoryRegionPortioList {
     MemoryRegion mr;
     void *portio_opaque;
-    MemoryRegionPortio ports[];
+    MemoryRegionPortio *ports;
 } MemoryRegionPortioList;
 
 static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
@@ -147,6 +147,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);
     }
     g_free(piolist->regions);
@@ -227,9 +228,9 @@ static void portio_list_add_1(PortioList *piolist,
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
-    mrpio = g_malloc0(sizeof(MemoryRegionPortioList) +
-                      sizeof(MemoryRegionPortio) * (count + 1));
+    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
     mrpio->portio_opaque = piolist->opaque;
+    mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
     memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
     memset(mrpio->ports + count, 0, sizeof(MemoryRegionPortio));
 
-- 
2.30.2

[PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Mark Cave-Ayland]

The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
structure can be managed using QOM's in-built refcounting instead of having to
handle this manually.

Due to the use of an opaque pointer it isn't possible to model the new
TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
use of the new object is restricted to the portio API we can simply set the
opaque pointer (and the heap-allocated port list) internally.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 softmmu/ioport.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index d0d5b0bcaa..238625a36f 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -32,11 +32,16 @@
 #include "exec/address-spaces.h"
 #include "trace.h"
 
-typedef struct MemoryRegionPortioList {
+struct MemoryRegionPortioList {
+    Object obj;
+
     MemoryRegion mr;
     void *portio_opaque;
     MemoryRegionPortio *ports;
-} MemoryRegionPortioList;
+};
+
+#define TYPE_MEMORY_REGION_PORTIO_LIST "memory-region-portio-list"
+OBJECT_DECLARE_SIMPLE_TYPE(MemoryRegionPortioList, MEMORY_REGION_PORTIO_LIST)
 
 static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
 {
@@ -228,7 +233,8 @@ static void portio_list_add_1(PortioList *piolist,
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
-    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
+    mrpio = MEMORY_REGION_PORTIO_LIST(
+                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));
     mrpio->portio_opaque = piolist->opaque;
     mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
     memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
@@ -298,3 +304,16 @@ void portio_list_del(PortioList *piolist)
         memory_region_del_subregion(piolist->address_space, &mrpio->mr);
     }
 }
+
+static const TypeInfo memory_region_portio_list_info = {
+    .parent             = TYPE_OBJECT,
+    .name               = TYPE_MEMORY_REGION_PORTIO_LIST,
+    .instance_size      = sizeof(MemoryRegionPortioList),
+};
+
+static void ioport_register_types(void)
+{
+    type_register_static(&memory_region_portio_list_info);
+}
+
+type_init(ioport_register_types)
-- 
2.30.2

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

[Mark Cave-Ayland]

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-ayland@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

Re: [PATCH 1/3] softmmu/ioport.c: allocate MemoryRegionPortioList ports on the heap

[Philippe Mathieu-Daudé]

On 19/4/23 17:16, Mark Cave-Ayland wrote:
> In order to facilitate a conversion of MemoryRegionPortioList to a QOM object
> move the allocation of MemoryRegionPortioList ports to the heap instead of
> using a variable-length member at the end of the MemoryRegionPortioList
> structure.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   softmmu/ioport.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Philippe Mathieu-Daudé]

On 19/4/23 17:16, Mark Cave-Ayland wrote:
> The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
> structure can be managed using QOM's in-built refcounting instead of having to
> handle this manually.
> 
> Due to the use of an opaque pointer it isn't possible to model the new
> TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
> use of the new object is restricted to the portio API we can simply set the
> opaque pointer (and the heap-allocated port list) internally.

In all uses this opaque pointer is a Object*. Almost all cases are
a DeviceState* and one is a BusState* (IDEBus).

Could this opaque become 'Object *owner' (simplifying the next patch)?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   softmmu/ioport.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index d0d5b0bcaa..238625a36f 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -32,11 +32,16 @@
>   #include "exec/address-spaces.h"
>   #include "trace.h"
>   
> -typedef struct MemoryRegionPortioList {
> +struct MemoryRegionPortioList {
> +    Object obj;
> +
>       MemoryRegion mr;
>       void *portio_opaque;
>       MemoryRegionPortio *ports;
> -} MemoryRegionPortioList;
> +};

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Mark Cave-Ayland]

On 20/04/2023 09:41, Philippe Mathieu-Daudé wrote:

> On 19/4/23 17:16, Mark Cave-Ayland wrote:
>> The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
>> structure can be managed using QOM's in-built refcounting instead of having to
>> handle this manually.
>>
>> Due to the use of an opaque pointer it isn't possible to model the new
>> TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
>> use of the new object is restricted to the portio API we can simply set the
>> opaque pointer (and the heap-allocated port list) internally.
> 
> In all uses this opaque pointer is a Object*. Almost all cases are
> a DeviceState* and one is a BusState* (IDEBus).
> 
> Could this opaque become 'Object *owner' (simplifying the next patch)?

I'm not sure that it does, since the opaque is part of the portio API and not related 
to this series which is focused on MemoryRegionPortioList i.e. the glue interface 
between the portio and memory APIs. Otherwise you end up changing the portio API and 
the associated callbacks which is orthogonal to the bug this series is trying to fix.

>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   softmmu/ioport.c | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
>> index d0d5b0bcaa..238625a36f 100644
>> --- a/softmmu/ioport.c
>> +++ b/softmmu/ioport.c
>> @@ -32,11 +32,16 @@
>>   #include "exec/address-spaces.h"
>>   #include "trace.h"
>> -typedef struct MemoryRegionPortioList {
>> +struct MemoryRegionPortioList {
>> +    Object obj;
>> +
>>       MemoryRegion mr;
>>       void *portio_opaque;
>>       MemoryRegionPortio *ports;
>> -} MemoryRegionPortioList;
>> +};


ATB,

Mark.

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Philippe Mathieu-Daudé]

On 19/4/23 17:16, Mark Cave-Ayland wrote:
> The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
> structure can be managed using QOM's in-built refcounting instead of having to
> handle this manually.
> 
> Due to the use of an opaque pointer it isn't possible to model the new
> TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
> use of the new object is restricted to the portio API we can simply set the
> opaque pointer (and the heap-allocated port list) internally.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   softmmu/ioport.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index d0d5b0bcaa..238625a36f 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -32,11 +32,16 @@
>   #include "exec/address-spaces.h"
>   #include "trace.h"
>   
> -typedef struct MemoryRegionPortioList {
> +struct MemoryRegionPortioList {
> +    Object obj;
> +
>       MemoryRegion mr;
>       void *portio_opaque;
>       MemoryRegionPortio *ports;
> -} MemoryRegionPortioList;
> +};
> +
> +#define TYPE_MEMORY_REGION_PORTIO_LIST "memory-region-portio-list"

Possibly simpler as: TYPE_MEMORY_REGION_PORTIO "memory-region-portio"

Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Philippe Mathieu-Daudé]

On 19/4/23 17:16, Mark Cave-Ayland wrote:
> The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
> structure can be managed using QOM's in-built refcounting instead of having to
> handle this manually.
> 
> Due to the use of an opaque pointer it isn't possible to model the new
> TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
> use of the new object is restricted to the portio API we can simply set the
> opaque pointer (and the heap-allocated port list) internally.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   softmmu/ioport.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)


>   static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
>   {
> @@ -228,7 +233,8 @@ static void portio_list_add_1(PortioList *piolist,
>       unsigned i;
>   
>       /* Copy the sub-list and null-terminate it.  */
> -    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
> +    mrpio = MEMORY_REGION_PORTIO_LIST(
> +                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));

Shouldn't we need to replace the g_free() call by object_unref()
in portio_list_destroy()?

>       mrpio->portio_opaque = piolist->opaque;
>       mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
>       memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
> @@ -298,3 +304,16 @@ void portio_list_del(PortioList *piolist)
>           memory_region_del_subregion(piolist->address_space, &mrpio->mr);
>       }
>   }

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Mark Cave-Ayland]

On 11/05/2023 14:46, Philippe Mathieu-Daudé wrote:

> On 19/4/23 17:16, Mark Cave-Ayland wrote:
>> The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
>> structure can be managed using QOM's in-built refcounting instead of having to
>> handle this manually.
>>
>> Due to the use of an opaque pointer it isn't possible to model the new
>> TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
>> use of the new object is restricted to the portio API we can simply set the
>> opaque pointer (and the heap-allocated port list) internally.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   softmmu/ioport.c | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
>> index d0d5b0bcaa..238625a36f 100644
>> --- a/softmmu/ioport.c
>> +++ b/softmmu/ioport.c
>> @@ -32,11 +32,16 @@
>>   #include "exec/address-spaces.h"
>>   #include "trace.h"
>> -typedef struct MemoryRegionPortioList {
>> +struct MemoryRegionPortioList {
>> +    Object obj;
>> +
>>       MemoryRegion mr;
>>       void *portio_opaque;
>>       MemoryRegionPortio *ports;
>> -} MemoryRegionPortioList;
>> +};
>> +
>> +#define TYPE_MEMORY_REGION_PORTIO_LIST "memory-region-portio-list"
> 
> Possibly simpler as: TYPE_MEMORY_REGION_PORTIO "memory-region-portio"

I'm a little undecided about this one: the ports field contains an array of 
MemoryRegionPortio entries e.g. 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/ide/ioport.c#L31 so I 
considered that the QOM object contains a list of MemoryRegionPortios. 
TYPE_MEMORY_REGION_PORTIO_LIST feels a better fit here since it reflects this whilst 
also matching the existing MemoryRegionPortioList struct name.

> Otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


ATB,

Mark.

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Mark Cave-Ayland]

On 11/05/2023 14:50, Philippe Mathieu-Daudé wrote:

> On 19/4/23 17:16, Mark Cave-Ayland wrote:
>> The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
>> structure can be managed using QOM's in-built refcounting instead of having to
>> handle this manually.
>>
>> Due to the use of an opaque pointer it isn't possible to model the new
>> TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
>> use of the new object is restricted to the portio API we can simply set the
>> opaque pointer (and the heap-allocated port list) internally.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   softmmu/ioport.c | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> 
>>   static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
>>   {
>> @@ -228,7 +233,8 @@ static void portio_list_add_1(PortioList *piolist,
>>       unsigned i;
>>       /* Copy the sub-list and null-terminate it.  */
>> -    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
>> +    mrpio = MEMORY_REGION_PORTIO_LIST(
>> +                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));
> 
> Shouldn't we need to replace the g_free() call by object_unref()
> in portio_list_destroy()?

Both the existing g_free() call and replacing it with object_unref() cause QEMU to 
segfault if you call portio_list_destroy() before the final patch in this series. So 
in effect we'd end up causing more code churn just to replace one crash with another ;)

The real fix is in patch 3 which alters the refcounting/ownership in order to solve 
the underlying issue, which I hope I have described in enough detail in that commit 
message.

>>       mrpio->portio_opaque = piolist->opaque;
>>       mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
>>       memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
>> @@ -298,3 +304,16 @@ void portio_list_del(PortioList *piolist)
>>           memory_region_del_subregion(piolist->address_space, &mrpio->mr);
>>       }
>>   }


ATB,

Mark.

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-ayland@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é <philmd@linaro.org>

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

[Mark Cave-Ayland]

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-ayland@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-philmd@linaro.org/20230302224058.43315-9-philmd@linaro.org/#ca177083-b24d-90cd-9f3c-c99653bc9a08@ilande.co.uk 
and 
https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/#6bc0dc92-3787-5379-b139-a8b5973d87fc@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é <philmd@linaro.org>


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

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-ayland@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-philmd@linaro.org/20230302224058.43315-9-philmd@linaro.org/#ca177083-b24d-90cd-9f3c-c99653bc9a08@ilande.co.uk and https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/#6bc0dc92-3787-5379-b139-a8b5973d87fc@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é <philmd@linaro.org>
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Paolo Bonzini]

On 5/11/23 16:43, Mark Cave-Ayland wrote:
>>>
>>> +#define TYPE_MEMORY_REGION_PORTIO_LIST "memory-region-portio-list"
>>
>> Possibly simpler as: TYPE_MEMORY_REGION_PORTIO "memory-region-portio"
> 
> I'm a little undecided about this one: the ports field contains an array 
> of MemoryRegionPortio entries e.g. 
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/ide/ioport.c#L31 
> so I considered that the QOM object contains a list of 
> MemoryRegionPortios. TYPE_MEMORY_REGION_PORTIO_LIST feels a better fit 
> here since it reflects this whilst also matching the existing 
> MemoryRegionPortioList struct name.

I agree, using memory-region-portio would be confusing given the name of 
the struct.

Paolo

Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Paolo Bonzini]

On 5/11/23 16:52, Mark Cave-Ayland wrote:
>>>
>>>       /* Copy the sub-list and null-terminate it.  */
>>> -    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
>>> +    mrpio = MEMORY_REGION_PORTIO_LIST(
>>> +                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));
>>
>> Shouldn't we need to replace the g_free() call by object_unref()
>> in portio_list_destroy()?
> 
> Both the existing g_free() call and replacing it with object_unref() 
> cause QEMU to segfault if you call portio_list_destroy() before the 
> final patch in this series. So in effect we'd end up causing more code 
> churn just to replace one crash with another 😉
> 
> The real fix is in patch 3 which alters the refcounting/ownership in 
> order to solve the underlying issue, which I hope I have described in 
> enough detail in that commit message.

Here I agree with Philippe though, there is a mismatch between new and 
unref that would (for example) cause the finalize method not to be called.

I think this patch should already introduce the instance_finalize 
function that only does "g_free(mrpio->ports);", and include the full 
portio_list_destroy() hunk from patch 3.

Then patch 3 basically focuses on the reparenting trick (including 
adding the object_unref() call in memory_region_portio_list_finalize).

I can do the change myself when applying though.

Paolo