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

[Huang Rui <ray.huang@xxxxxxx>]

On Tue, Sep 05, 2023 at 05:17:32PM +0800, Akihiko Odaki wrote:
> On 2023/09/05 18:07, Huang Rui wrote:
> > On Thu, Aug 31, 2023 at 06:10:08PM +0800, Akihiko Odaki wrote:
> >> On 2023/08/31 18:32, Huang Rui wrote:
> >>> From: Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>
> >>>
> >>> 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@xxxxxxx>
> >>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> >>> ---
> >>>
> >>> 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.
> > 
> > The reason that we modify it is because we would like to address the memory
> > leak issue in the original codes. Please see below, we find the memory
> > region will be crashed once we free(unref) the simple resource, because the
> > region will be freed in object_finalize() after unparent and the ref count
> > is to 0. Then the VM will be crashed with this.
> > 
> > In virgl_cmd_resource_map_blob():
> >      memory_region_init_ram_device_ptr(res->region, OBJECT(g), NULL, size, 
> > data);
> >      OBJECT(res->region)->free = g_free;
> >      memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
> >      memory_region_set_enabled(res->region, true);
> > 
> > In virtio_gpu_virgl_resource_unmap():
> >      memory_region_set_enabled(res->region, false);
> >      memory_region_del_subregion(&b->hostmem, res->region);
> >      object_unparent(OBJECT(res->region));
> >      res->region = NULL;
> > 
> > I spent a bit more time to understand your point, do you want me to update
> > corresponding comments or you have some concern about this change?
> 
> As the comment says ref-ing memory regions without an owner will slow 
> down DMA, you should avoid that. More concretely, you should check 
> mr->owner before doing object_ref(OBJECT(mr)).
> 

I get it, thanks to point this exactly. Will update it in V5.

Thanks,
Ray