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
| ||
|
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
Powered by blists - more mailing lists