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

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

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

Regards,
Akihiko Odaki