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!
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.
