linux-kernel - Re: [PATCH] mm: drop VM_BUG_ON from __get_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180626100416.a3ff53f5c4aac9fae954e3f6@linux-foundation.org>
Date:   Tue, 26 Jun 2018 10:04:16 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Michal Hocko <mhocko@...nel.org>,
        JianKang Chen <chenjiankang1@...wei.com>,
        Mel Gorman <mgorman@...e.de>,
        Johannes Weiner <hannes@...xchg.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, xieyisheng1@...wei.com,
        guohanjun@...wei.com, wangkefeng.wang@...wei.com,
        Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH] mm: drop VM_BUG_ON from __get_free_pages

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

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