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

[Uladzislau Rezki <urezki@gmail.com>]

On Wed, Nov 05, 2025 at 03:58:22PM -0800, Vishal Moola (Oracle) wrote:
> 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.
> 
As far as i got, we fix the mask if it contains buggy flags. In that
sense it is absolutely OK to warn and print the stack of caller that
violates the rules.

> > > +
> > > +	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.
> 
Well, i will not insist on. You decide :)

>
> 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.
>
Sounds good. One thing i have noticed, it is below peace of code:


/* __GFP_NOFAIL and "noblock" flags are mutually exclusive. */
if (!gfpflags_allow_blocking(gfp_mask))
	nofail = false;

i forgot to drop the __GFP_NOFAIL for non-blocking flags. But this
is not a problem of this series. I will fix it by sending the patch.

--
Uladzislau Rezki