linux-kernel - Re: [patch] mm, page_alloc: make __GFP

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: <20131210153909.8b4bfa1d643e5f8582eff7c9@linux-foundation.org>
Date:	Tue, 10 Dec 2013 15:39:09 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Mel Gorman <mgorman@...e.de>, Michal Hocko <mhocko@...e.cz>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch] mm, page_alloc: make __GFP_NOFAIL really not fail

On Tue, 10 Dec 2013 15:20:17 -0800 (PST) David Rientjes <rientjes@...gle.com> wrote:

> On Mon, 9 Dec 2013, Andrew Morton wrote:
> 
> > > __GFP_NOFAIL specifies that the page allocator cannot fail to return
> > > memory.  Allocators that call it may not even check for NULL upon
> > > returning.
> > > 
> > > It turns out GFP_NOWAIT | __GFP_NOFAIL or GFP_ATOMIC | __GFP_NOFAIL can
> > > actually return NULL.  More interestingly, processes that are doing
> > > direct reclaim and have PF_MEMALLOC set may also return NULL for any
> > > __GFP_NOFAIL allocation.
> > 
> > __GFP_NOFAIL is a nasty thing and making it pretend to work even better
> > is heading in the wrong direction, surely?  It would be saner to just
> > disallow these even-sillier combinations.  Can we fix up the current
> > callers then stick a WARN_ON() in there?
> > 
> 
> Heh, it's difficult to remove __GFP_NOFAIL when new users get added: 
> 84235de394d9 ("fs: buffer: move allocation failure loop into the 
> allocator") added a new user

That wasn't reeeeealy a new user - it was "convert an existing
open-coded retry-for-ever loop".  Which is what __GFP_NOFAIL is for.

I don't think I've ever seen anyone actually fix one of these things
(by teaching the caller to handle ENOMEM), so it obviously isn't
working...

> and a bypass of memcg limits in oom 
> conditions so __GFP_NOFAIL just essentially became 
> __GFP_BYPASS_MEMCG_LIMIT_ON_OOM.
> 
> We can probably ignore the PF_MEMALLOC behavior since it allows full 
> access to memory reserves and the only time we would see a __GFP_NOFAIL 
> allocation fail in such a context is if every zone's free memory was 0.  
> We have bigger problems if memory reserves are completely depleted like 
> that, so it's probably sufficient not to address it.
> 
> I'd be concerned about new users of __GFP_NOFAIL that are added for 
> GFP_NOWAIT or GFP_ATOMIC and never actually trigger such a warning because 
> in testing they never trigger the slowpath, but the conditional is 
> probably better placed outside of the fastpath:
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2536,8 +2536,15 @@ rebalance:
>  	}
>  
>  	/* Atomic allocations - we can't balance anything */
> -	if (!wait)
> +	if (!wait) {
> +		/*
> +		 * All existing users of the deprecated __GFP_NOFAIL are
> +		 * blockable, so warn of any new users that actually allow this
> +		 * type of allocation to fail.
> +		 */
> +		WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL);
>  		goto nopage;
> +	}

Seems sensible.

> But perhaps the best way to do this in a preventative way is to add a 
> warning to checkpatch.pl that actually warns about adding new users.

yup.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/