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

[Peter Xu]

On Tue, Aug 27, 2024 at 01:28:02PM -0400, Stefan Hajnoczi wrote:
> On Tue, 27 Aug 2024 at 13:24, David Hildenbrand <david@redhat.com> wrote:
> >
> > 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 ?
> 
> Sounds good to me. In practice scalability bottlenecks are likely with
> those memory sizes and it will be necessary to change how guest memory
> is organized anyway. But it doesn't hurt to make this counter
> future-proof.

IMHO it'll be nice to only use ram_addr_t when a variable is describing the
ramblock address space (with an offset, or a length there).  In this case
it is a pure counter for how many bitmap chunks we allocated, so maybe
"unsigned long" or "uint64_t" would suite more?

Though I'd think "unsigned int" is good enough per the calculation Stefan
provided.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu