Re: [RFC PATCH v2 1/4] mm/vmalloc: warn on invalid vmalloc gfp flags

["Vishal Moola (Oracle)" <vishal.moola@gmail.com>]

On Tue, Nov 04, 2025 at 05:28:16PM +0100, Uladzislau Rezki wrote:
> On Mon, Nov 03, 2025 at 11:04:26AM -0800, Vishal Moola (Oracle) wrote:
> > Vmalloc explicitly supports a list of flags, but we never enforce them.
> > vmalloc has been trying to handle unsupported flags by clearing and
> > setting flags wherever necessary. This is messy and makes the code
> > harder to understand, when we could simply check for a supported input
> > immediately instead.
> > 
> > Define a helper mask and function telling callers they have passed in
> > invalid flags, and clear those unsupported vmalloc flags.
> > 
> > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> > ---
> >  mm/vmalloc.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 0832f944544c..290016c7fb58 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -3911,6 +3911,26 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> >  	return NULL;
> >  }
> >  
> > +/*
> > + * See __vmalloc_node_range() for a clear list of supported vmalloc flags.
> > + * This gfp lists all flags currently passed through vmalloc. Currently,
> > + * __GFP_ZERO is used by BFP and __GFP_NORETRY is used by percpu.
> > + */
> > +#define GFP_VMALLOC_SUPPORTED (GFP_KERNEL | GFP_ATOMIC | GFP_NOWAIT |\
> > +				__GFP_NOFAIL |  __GFP_ZERO | __GFP_NORETRY)
> > +
> > +static gfp_t vmalloc_fix_flags(gfp_t flags)
> > +{
> > +	gfp_t invalid_mask = flags & ~GFP_VMALLOC_SUPPORTED;
> > +
> > +	flags &= GFP_VMALLOC_SUPPORTED;
> > +	pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> > +			invalid_mask, &invalid_mask, flags, &flags);
> > +	dump_stack();
> >
> WARN_ON() or friends? It prints the stack.

My understanding of WARN_ON() variants is they're used for internal
kernel concerns, while the pr_warn() variants are for situations like
this where proprietary drivers can cause unexpected behavior.

> > +
> > +	return flags;
> > +}
> > +
> >  /**
> >   * __vmalloc_node_range - allocate virtually contiguous memory
> >   * @size:		  allocation size
> > @@ -4092,6 +4112,8 @@ EXPORT_SYMBOL_GPL(__vmalloc_node_noprof);
> >  
> >  void *__vmalloc_noprof(unsigned long size, gfp_t gfp_mask)
> >  {
> > +	if (gfp_mask & ~GFP_VMALLOC_SUPPORTED)
> > +		gfp_mask = vmalloc_fix_flags(gfp_mask);
> >  	return __vmalloc_node_noprof(size, 1, gfp_mask, NUMA_NO_NODE,
> >  				__builtin_return_address(0));
> >  }
> > @@ -4131,6 +4153,8 @@ EXPORT_SYMBOL(vmalloc_noprof);
> >   */
> >  void *vmalloc_huge_node_noprof(unsigned long size, gfp_t gfp_mask, int node)
> >  {
> > +	if (gfp_mask & ~GFP_VMALLOC_SUPPORTED)
> > +		gfp_mask = vmalloc_fix_flags(gfp_mask);
> >
> gfp_mask = check_and_fix_flags()? 

I just mirrored how its done in mm/slab.h. IMO its cleaner to keep
the check out here and have vmalloc_fix_flags() stick to one thing.

If you really want it as check_and_fix_flags(), let me know and I'm open
to changing it in the next version.

On another note, I'm now realizing I forgot to mark the check as
unlikey(). I'll include that in a final version once the other 2
patches have been looked at.