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

[Xenia Ragiadakou]

On 15/9/23 18:11, Akihiko Odaki wrote:
> On 2023/09/15 20:11, 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 an owned 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>
> > ---
> >
> > V4 -> V5:
> >      - ref/unref only owned memory regions (Akihiko)
> >
> >   softmmu/memory.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 7d9494ce70..15e1699750 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *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.
> > +     * Likewise, the owner keeps references to the memory region,
> > +     * hence the need to ref/unref the memory region object to prevent
> > +     * its automatic deallocation while still referenced by its owner.
>
> This comment does not make sense. Traditionally no such automatic 
> deallocation happens so the owner has been always required to free the 
> memory region when it gets finalized.
>
> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
> introduces a different kind of memory region, which can be freed 
> anytime before the device gets finalized. Even in this case, the owner 
> removes the reference to the memory owner by doing res->region = NULL;

Hi Akihiko,

You are right, the word "owner" is not correct. The issue observed was 
due to the references kept in flatview ranges and the fact that 
flatview_destroy() is asynchronous and was called after memory region's 
destruction.

If I replace the word "owner" with "memory subsystem" in the commit 
message and drop the comment, would that be ok with you? or do want to 
suggest something else?

Xenia

> Regards,
> Akihiko Odaki