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@xxxxxxxxxx>
> > > Cc: Mike Rapoport <rppt@xxxxxxxxxx>
> > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Alex Graf <graf@xxxxxxxxx>
> > > ---
> > > 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.
