Re: [PATCH] mm: drop VM_BUG_ON from __get_free_pages

[Michal Hocko]

On Wed 27-06-18 14:14:12, Andrew Morton wrote:
> On Wed, 27 Jun 2018 09:50:01 +0200 Vlastimil Babka <vbabka@xxxxxxx> 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.

I would argue that internal kernel APIs should trust their callers.
Panicing with an unknown context is about the worst way to teach
developers how to use the API properly. Because it will be end users
seeing an outage. So I would simply not care beyond documenting the
expectation. If we want to be more careful then fix it up. If you
disagree then just drop the patch. I do not insist so much to spend much
more time on something I thought was quite obvious. BUG_ON for an inpropoer
API usage is considered harmful for quite a long time by now. I do not
see why this would be any different.

-- 
Michal Hocko
SUSE Labs