Re: [Qemu-devel] [PATCH] memory: move MemoryRegion::size cleanup to memory_region_finalize()

[Laszlo Ersek]

On 10/09/18 17:47, Paolo Bonzini wrote:
> On 09/10/2018 15:50, Igor Mammedov wrote:
>> It's not necessary to clean up MemoryRegion::size manually in multiple places
>> like it's been implemented in
>>  (1cd3d49262 "memory: cleanup side effects of  memory_region_init_foo() on 
>> failure")
>> since each memory_region_init_foo() now calls object_unparent(mr) on failure,
>> memory region destructor memory_region_finalize() will be called upon its
>> completion for each memory_region_init_foo().
>> It's sufficient to clean MemoryRegion::size only from 
>> memory_region_finalize(),
>> so do it.
> 
> Stupid question, why is it necessary to clear mr->size at all?...

IIRC it is explained in the above-mentioned, earlier commit 1cd3d49262:

> if MemoryRegion intialization fails it's left in semi-initialized state,
> where it's size is not 0 and attached as child to owner object.
> And this leds to crash in following use-case:
>     (monitor) object_add 
> memory-backend-file,id=mem1,size=99999G,mem-path=/tmp/foo,discard-data=yes
>     memory.c:2083: memory_region_get_ram_ptr: Assertion `mr->ram_block' failed
>     Aborted (core dumped)
> it happens due to assumption that memory region is intialized when
>    memory_region_size() != 0
> and therefore it's ok to access it in
>    file_backend_unparent()
>       if (memory_region_size() != 0)
>           memory_region_get_ram_ptr()
> 
> which happens when object_add fails and unparents failed backend making
> file_backend_unparent() access invalid memory region.

I think it makes sense to zero out the size even if unparenting would, in 
itself, prevent the above crash. Because, in host_memory_backend_mr_inited(), 
we have:

    /*
     * NOTE: We forbid zero-length memory backend, so here zero means
     * "we haven't inited the backend memory region yet".
     */

I'm unsure how general that invariant is, but it can't hurt to honor it 
everywhere. (Especially if we can do the zeroing in one common place.)

Anyway, I should defer to Igor on this! :)

Thanks,
Laszlo

>>  memory.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index d852f11..ad24b57 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1491,7 +1491,6 @@ void 
>> memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>>      mr->ram_block = qemu_ram_alloc(size, share, mr, &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>> -        mr->size = int128_zero();
>>          object_unparent(OBJECT(mr));
>>          error_propagate(errp, err);
>>      }
>> @@ -1516,7 +1515,6 @@ void memory_region_init_resizeable_ram(MemoryRegion 
>> *mr,
>>                                                mr, &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>> -        mr->size = int128_zero();
>>          object_unparent(OBJECT(mr));
>>          error_propagate(errp, err);
>>      }
>> @@ -1541,7 +1539,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>>      mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, 
>> &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>> -        mr->size = int128_zero();
>>          object_unparent(OBJECT(mr));
>>          error_propagate(errp, err);
>>      }
>> @@ -1565,7 +1562,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>>                                             fd, &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>> -        mr->size = int128_zero();
>>          object_unparent(OBJECT(mr));
>>          error_propagate(errp, err);
>>      }
>> @@ -1628,7 +1624,6 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>      mr->ram_block = qemu_ram_alloc(size, false, mr, &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>> -        mr->size = int128_zero();
>>          object_unparent(OBJECT(mr));
>>          error_propagate(errp, err);
>>      }
>> @@ -1652,7 +1647,6 @@ void 
>> memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>>      mr->destructor = memory_region_destructor_ram;
>>      mr->ram_block = qemu_ram_alloc(size, false,  mr, &err);
>>      if (err) {
>> -        mr->size = int128_zero();
>>          object_unparent(OBJECT(mr));
>>          error_propagate(errp, err);
>>      }
>> @@ -1701,6 +1695,7 @@ static void memory_region_finalize(Object *obj)
>>      memory_region_clear_coalescing(mr);
>>      g_free((char *)mr->name);
>>      g_free(mr->ioeventfds);
>> +    mr->size = int128_zero();
>>  }
>>  
>>  Object *memory_region_owner(MemoryRegion *mr)
>>
>