Re: [PATCH] mm: drop VM_BUG_ON from __get_free_pages

[Michal Hocko]

On Tue 26-06-18 10:04:16, Andrew Morton wrote:
> On Tue, 26 Jun 2018 15:57:39 +0200 Vlastimil Babka <vbabka@xxxxxxx> wrote:
> 
> > On 06/22/2018 06:28 PM, Michal Hocko wrote:
> > > From: Michal Hocko <mhocko@xxxxxxxx>
> > > 
> > > There is no real reason to blow up just because the caller doesn't know
> > > that __get_free_pages cannot return highmem pages. Simply fix that up
> > > silently. Even if we have some confused users such a fixup will not be
> > > harmful.
> > > 
> >
> > ...
> >
> > >  /*
> > > - * Common helper functions.
> > > + * Common helper functions. Never use with __GFP_HIGHMEM because the returned
> > > + * address cannot represent highmem pages. Use alloc_pages and then kmap if
> > > + * you need to access high mem.
> > >   */
> > >  unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
> > >  {
> > >  	struct page *page;
> > >  
> > > -	/*
> > > -	 * __get_free_pages() returns a virtual address, which cannot represent
> > > -	 * a highmem page
> > > -	 */
> > > -	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
> > > -
> > >  	page = alloc_pages(gfp_mask, order);
> > 
> > The previous version had also replaced the line above with:
> > 
> > +	page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order);
> > 
> > This one doesn't, yet you say "fix that up silently". Bug?
> > 
> 
> This reminds me what is irritating about the patch.  We're adding
> additional code to a somewhat fast path to handle something which we
> know never happens, thanks to the now-removed check.
> 
> This newly-added code might become functional in the future, if people
> add incorrect callers.  Callers whose incorrectness would have been
> revealed by the now-removed check!

That check depends on a debugging config option which is not enabled all
the time so how does this help in most production systems? More over it
is "blow up on incorrect use" kind of check. I am pretty sure Linus
would have some word about such error handling...

> So.. argh.
> 
> Really, the changelog isn't right.  There *is* a real reason to blow
> up.  Effectively the caller is attempting to obtain the virtual address
> of a highmem page without having kmapped it first.  That's an outright
> bug.

And as I've argued before the code would be wrong regardless. We would
leak the memory or worse touch somebody's else kmap without knowing
that.  So we have a choice between a mem leak, data corruption k or a
silent fixup. I would prefer the last option. And blowing up on a BUG
is not much better on something that is easily fixable. I am not really
convinced that & ~__GFP_HIGHMEM is something to lose sleep over.
 
> An alternative might be to just accept the bogus __GFP_HIGHMEM, let
> page_to_virt() return a crap address and wait for the user bug reports
> to come in when someone tries to run the offending code on a highmem
> machine.  That shouldn't take too long - the page allocator will prefer
> to return a highmem page in this case.

Well, I would be concerned about page_address returning an active kmap
much more.

> And adding a rule to the various static checkers should catch most
> offenders.

This all sounds like a heavy lifting for something that can be easily
fixed up. If we ever see the additional and with a mask as a problem we
can think about a more optimal solution.

-- 
Michal Hocko
SUSE Labs