linux-kernel - Re: [PATCH 0/3 -v3] 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: <ad1fdd02-04c4-d7e1-776b-1a49302303d9@suse.cz>
Date:   Tue, 3 Jan 2017 17:25:02 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        mhocko@...nel.org
Cc:     akpm@...ux-foundation.org, hannes@...xchg.org, rientjes@...gle.com,
        mgorman@...e.de, hillf.zj@...baba-inc.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups

On 01/03/2017 03:38 PM, Tetsuo Handa wrote:
> Michal Hocko wrote:
>> On Tue 03-01-17 10:36:31, Tetsuo Handa wrote:
>> [...]
>> > I'm OK with "[PATCH 1/3] mm: consolidate GFP_NOFAIL checks in the allocator
>> > slowpath" given that we describe that we make __GFP_NOFAIL stronger than
>> > __GFP_NORETRY with this patch in the changelog.
>>
>> Again. __GFP_NORETRY | __GFP_NOFAIL is nonsense! I do not really see any
>> reason to describe all the nonsense combinations of gfp flags.
>
> Before [PATCH 1/3]:
>
>   __GFP_NORETRY is used as "Do not invoke the OOM killer. Fail allocation
>   request even if __GFP_NOFAIL is specified if direct reclaim/compaction
>   did not help."
>
>   __GFP_NOFAIL is used as "Never fail allocation request unless __GFP_NORETRY
>   is specified even if direct reclaim/compaction did not help."
>
> After [PATCH 1/3]:
>
>   __GFP_NORETRY is used as "Do not invoke the OOM killer. Fail allocation
>   request unless __GFP_NOFAIL is specified."
>
>   __GFP_NOFAIL is used as "Never fail allocation request even if direct
>   reclaim/compaction did not help. Invoke the OOM killer unless __GFP_NORETRY is
>   specified."
>
> Thus, __GFP_NORETRY | __GFP_NOFAIL perfectly makes sense as
> "Never fail allocation request if direct reclaim/compaction did not help.
> But do not invoke the OOM killer even if direct reclaim/compaction did not help."

It may technically do that, but how exactly is that useful, i.e. "make sense"? 
Patch 2/3 here makes sure that OOM killer is not invoked when the allocation 
context is "limited" and thus OOM might be premature (despite __GFP_NOFAIL).
What's the use case for __GFP_NORETRY | __GFP_NOFAIL ?

>
>>
>> > But I don't think "[PATCH 2/3] mm, oom: do not enfore OOM killer for __GFP_NOFAIL
>> > automatically" is correct. Firstly, we need to confirm
>> >
>> >   "The pre-mature OOM killer is a real issue as reported by Nils Holland"
>> >
>> > in the changelog is still true because we haven't tested with "[PATCH] mm, memcg:
>> > fix the active list aging for lowmem requests when memcg is enabled" applied and
>> > without "[PATCH 2/3] mm, oom: do not enfore OOM killer for __GFP_NOFAIL
>> > automatically" and "[PATCH 3/3] mm: help __GFP_NOFAIL allocations which do not
>> > trigger OOM killer" applied.
>>
>> Yes I have dropped the reference to this report already in my local
>> patch because in this particular case the issue was somewhere else
>> indeed!
>
> OK.
>
>>
>> > Secondly, as you are using __GFP_NORETRY in "[PATCH] mm: introduce kv[mz]alloc
>> > helpers" as a mean to enforce not to invoke the OOM killer
>> >
>> > 	/*
>> > 	 * Make sure that larger requests are not too disruptive - no OOM
>> > 	 * killer and no allocation failure warnings as we have a fallback
>> > 	 */
>> > 	if (size > PAGE_SIZE)
>> > 		kmalloc_flags |= __GFP_NORETRY | __GFP_NOWARN;
>> >
>> > , we can use __GFP_NORETRY as a mean to enforce not to invoke the OOM killer
>> > rather than applying "[PATCH 2/3] mm, oom: do not enfore OOM killer for
>> > __GFP_NOFAIL automatically".
>> >
>
> As I wrote above, __GFP_NORETRY | __GFP_NOFAIL perfectly makes sense.
>
>> > Additionally, although currently there seems to be no
>> > kv[mz]alloc(GFP_KERNEL | __GFP_NOFAIL) users, kvmalloc_node() in
>> > "[PATCH] mm: introduce kv[mz]alloc helpers" will be confused when a
>> > kv[mz]alloc(GFP_KERNEL | __GFP_NOFAIL) user comes in in the future because
>> > "[PATCH 1/3] mm: consolidate GFP_NOFAIL checks in the allocator slowpath" makes
>> > __GFP_NOFAIL stronger than __GFP_NORETRY.
>>
>> Using NOFAIL in kv[mz]alloc simply makes no sense at all. The vmalloc
>> fallback would be simply unreachable!
>
> My intention is shown below.
>
>  void *kvmalloc_node(size_t size, gfp_t flags, int node)
>  {
>  	gfp_t kmalloc_flags = flags;
>  	void *ret;
>
>  	/*
>  	 * vmalloc uses GFP_KERNEL for some internal allocations (e.g page tables)
>  	 * so the given set of flags has to be compatible.
>  	 */
>  	WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
>
>  	/*
>  	 * Make sure that larger requests are not too disruptive - no OOM
>  	 * killer and no allocation failure warnings as we have a fallback
>  	 */
> -	if (size > PAGE_SIZE)
> +	if (size > PAGE_SIZE) {
>  		kmalloc_flags |= __GFP_NORETRY | __GFP_NOWARN;
> +		kmalloc_flags &= ~__GFP_NOFAIL;

This does make kvmalloc_node more robust against callers that would try to use 
it with __GFP_NOFAIL, but is it a good idea to allow that right now? If there 
are none yet (AFAIK?), we should rather let the existing WARN_ON kick in (which 
won't happen if we strip __GFP_NOFAIL) and discuss a better solution for such 
new future caller.

Also this means the kmalloc() cannot do "__GFP_NORETRY | __GFP_NOFAIL" so I'm 
not sure how it's related with your points above - it's not an example of the 
combination that would show that "it makes perfect sense".

Thanks,
Vlastimil