Re: [PATCH] mm: drop VM_BUG_ON from __get_free_pages

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 26 Jun 2018 15:57:39 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

> On 06/22/2018 06:28 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > 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!

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.


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.

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

Or just leave the ode as it is now.