Re: [PATCH v1] softmmu/physmem: fix memory leak in dirty_memory_extend()

[David Hildenbrand <david@redhat.com>]

On 27.08.24 18:52, Stefan Hajnoczi wrote:
> On Tue, 27 Aug 2024 at 04:38, David Hildenbrand <david@redhat.com> wrote:
>>
>> As reported by Peter, we might be leaking memory when removing the
>> highest RAMBlock (in the weird ram_addr_t space), and adding a new one.
>>
>> We will fail to realize that we already allocated bitmaps for more
>> dirty memory blocks, and effectively discard the pointers to them.
>>
>> Fix it by getting rid of last_ram_page() and simply storing the number
>> of dirty memory blocks that have been allocated. We'll store the number
>> of blocks along with the actual pointer to keep it simple.
>>
>> Looks like this leak was introduced as we switched from using a single
>> bitmap_zero_extend() to allocating multiple bitmaps:
>> bitmap_zero_extend() relies on g_renew() which should have taken care of
>> this.
>>
>> Resolves: https://lkml.kernel.org/r/CAFEAcA-k7a+VObGAfCFNygQNfCKL=AfX6A4kScq=VSSK0peqPg@mail.gmail.com
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Fixes: 5b82b703b69a ("memory: RCU ram_list.dirty_memory[] for safe RAM hotplug")
>> Cc: qemu-stable@nongnu.org
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/exec/ramlist.h |  1 +
>>   system/physmem.c       | 44 ++++++++++++++----------------------------
>>   2 files changed, 16 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
>> index 2ad2a81acc..f2a965f293 100644
>> --- a/include/exec/ramlist.h
>> +++ b/include/exec/ramlist.h
>> @@ -41,6 +41,7 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
>>   #define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
>>   typedef struct {
>>       struct rcu_head rcu;
>> +    unsigned int num_blocks;
> 
> The maximum amount of memory supported by unsigned int is:
> (2 ^ 32 - 1) * 4KB * DIRTY_MEMORY_BLOCK_SIZE
> = ~32 exabytes
> 

True, should we simply use ram_addr_t ?

> This should be fine. The maximum guest RAM sizes are in the TBs range
> (source: https://access.redhat.com/articles/rhel-kvm-limits).
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks

-- 
Cheers,

David / dhildenb