Re: [PATCH] memblocks: Move late alloc warning down to phys alloc

[Mike Rapoport <rppt@kernel.org>]

On Wed, Jun 19, 2024 at 07:53:14AM +0000, Gowans, James wrote:
> On Tue, 2024-06-18 at 14:02 +0300, Mike Rapoport wrote:
> > On Fri, Jun 14, 2024 at 03:30:16PM +0200, James Gowans wrote:
> > > Subject: [PATCH] memblocks: Move late alloc warning down to phys alloc
> > 
> > Nit: memblock
> 
> Ack!
> 
> > 
> > > If a driver/subsystem tries to do an allocation after memblocks have

And another one here :)                                     ^

> > > been freed and the memory handed to the buddy allocator, it will not
> > > actually be legal to use that allocation - the buddy allocator owns the
> > > memory. This is handled by the memblocks function which does allocations
> > > and returns virtual addresses by printing a warning and doing a kmalloc
> > > instead. However, the physical allocation function does not to do this
> > > check - callers of the physical alloc function are unprotected against
> > > mis-use.
> > 
> > Did you see such misuse or this is a theoretical issue?
> 
> Yeah, I was driving the memblock allocator badly when prototyping
> something. Allocating a large contiguous block of memory for an in-
> memory filesystem and I was doing the allocation in an initcall, but by
> that point it was too late. The memblock allocator happily gave me a
> large chunk of memory, but it was already in use by the buddy allocator,
> so ended up with memory corruption. Oops! Getting this warning would
> have made the problem immediately obvious.
> 
> > 
> > > Improve the error catching here by moving the check into the physical
> > > allocation function which is used by the virtual addr allocation
> > > function.
> > > 
> > > Signed-off-by: James Gowans <jgowans@amazon.com>
> > > Cc: Mike Rapoport <rppt@kernel.org>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Alex Graf <graf@amazon.de>
> > > ---
> > >  mm/memblock.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/mm/memblock.c b/mm/memblock.c
> > > index d09136e040d3..dd4f237dc1fc 100644
> > > --- a/mm/memblock.c
> > > +++ b/mm/memblock.c
> > > @@ -1457,6 +1457,17 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
> > >               align = SMP_CACHE_BYTES;
> > >       }
> > > 
> > > +     /*
> > > +      * Detect any accidental use of these APIs after slab is ready, as at
> > > +      * this moment memblock may be deinitialized already and its
> > > +      * internal data may be destroyed (after execution of memblock_free_all)
> > > +      */
> > > +     if (WARN_ON_ONCE(slab_is_available())) {
> > > +             void *vaddr = kzalloc_node(size, GFP_NOWAIT, nid);
> > > +
> > > +             return vaddr ? virt_to_phys(vaddr) : 0;
> > > +     }
> > 
> > I'd move this before alignment check.
> 
> Ack, will do in V2.
> 
> Anything else or should I make the tweaks and post V2?
 
Looks ok to me.

> JG

-- 
Sincerely yours,
Mike.