Re: [QEMU PATCH v4 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/08/31 18:32, Huang Rui wrote:
> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> 
> When the memory region has a different life-cycle from that of her parent,
> could be automatically released, once has been unparent and once all of her
> references have gone away, via the object's free callback.
> 
> However, currently, references to the memory region are held by its owner
> without first incrementing the memory region object's reference count.
> As a result, the automatic deallocation of the object, not taking into
> account those references, results in use-after-free memory corruption.
> 
> This patch increases the reference count of the memory region object on
> each memory_region_ref() and decreases it on each memory_region_unref().
> 
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> New patch
> 
>   softmmu/memory.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7d9494ce70..0fdd5eebf9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1797,6 +1797,15 @@ Object *memory_region_owner(MemoryRegion *mr)
>   
>   void memory_region_ref(MemoryRegion *mr)
>   {
> +    if (!mr) {
> +        return;
> +    }
> +
> +    /* Obtain a reference to prevent the memory region object
> +     * from being released under our feet.
> +     */
> +    object_ref(OBJECT(mr));
> +
>       /* MMIO callbacks most likely will access data that belongs
>        * to the owner, hence the need to ref/unref the owner whenever
>        * the memory region is in use.
> @@ -1807,16 +1816,22 @@ void memory_region_ref(MemoryRegion *mr)
>        * Memory regions without an owner are supposed to never go away;
>        * we do not ref/unref them because it slows down DMA sensibly.
>        */

The collapsed comment says:
 > The memory region is a child of its owner.  As long as the
 > owner doesn't call unparent itself on the memory region,
 > ref-ing the owner will also keep the memory region alive.
 > Memory regions without an owner are supposed to never go away;
 > we do not ref/unref them because it slows down DMA sensibly.

It contradicts with this patch.

> -    if (mr && mr->owner) {
> +    if (mr->owner) {
>           object_ref(mr->owner);
>       }
>   }
>   
>   void memory_region_unref(MemoryRegion *mr)
>   {
> -    if (mr && mr->owner) {
> +    if (!mr) {
> +        return;
> +    }
> +
> +    if (mr->owner) {
>           object_unref(mr->owner);
>       }
> +
> +    object_unref(OBJECT(mr));
>   }
>   
>   uint64_t memory_region_size(MemoryRegion *mr)