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]

Date:   Wed, 27 Jun 2018 14:14:12 -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
Subject: Re: [PATCH] mm: drop VM_BUG_ON from __get_free_pages

On Wed, 27 Jun 2018 09:50:01 +0200 Vlastimil Babka <vbabka@...e.cz> wrote:

> On 06/27/2018 09:34 AM, Michal Hocko wrote:
> > On Tue 26-06-18 10:04:16, Andrew Morton wrote:
> > 
> > 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.
> 
> Maybe put the fixup into a "#ifdef CONFIG_HIGHMEM" block and then modern
> systems won't care? In that case it could even be if (WARN_ON_ONCE(...))
> so future cases with wrong expectations would become known.
> 

The more I think about it, the more I like the VM_BUG_ON.

Look, if I was reviewing code which did

	page = alloc_page(__GFP_HIGHMEM);
	addr = page_to_virt(page);

I would say "that's a bug, you forgot to kmap the page".

And any code which does __get_free_pages(__GFP_HIGHMEM) is just as
buggy: it's requesting the virtual address of a high page without
having kmapped it.  Core MM shouldn't be silently kludging around the
bug by restricting the caller to using lowmem pages.

Maybe the caller really does want to use highmem, in which case the caller
should be using alloc_page(__GFP_HIGHMEM) and kmap().  Because core MM
detects and reports this bug, the developer will fix it.