linux-kernel - Re: [PATCH] oom: always panic on OOM when panic_on

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: <alpine.DEB.2.10.1506091516000.30516@chino.kir.corp.google.com>
Date:	Tue, 9 Jun 2015 15:28:40 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Michal Hocko <mhocko@...e.cz>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	linux-mm@...ck.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] oom: always panic on OOM when panic_on_oom is
 configured

On Tue, 9 Jun 2015, Michal Hocko wrote:

> > > On Mon 08-06-15 12:51:53, David Rientjes wrote:
> > > Do you actually have panic_on_oops enabled?
> > > 
> > 
> > CONFIG_PANIC_ON_OOPS_VALUE should be 0, I'm not sure why that's relevant.
> 
> No I meant panic_on_oops > 0.
> 

CONFIG_PANIC_ON_OOPS_VALUE sets panic_on_oops, so it's 0.

> > The functionality I'm referring to is that your patch now panics the 
> > machine for configs where /proc/sys/vm/panic_on_oom is set and the same 
> > scenario occurs as described above.  You're introducing userspace breakage 
> > because you are using panic_on_oom in a way that it hasn't been used in 
> > the past and isn't described as working in the documentation.
> 
> I am sorry, but I do not follow. The knob has been always used to
> _panic_ the OOM system. Nothing more and nothing less. Now you
> are arguing about the change being buggy because a task might be
> killed but that argument doesn't make much sense to me because
> basically _any_ other allocation which allows OOM to trigger might hit
> check_panic_on_oom() and panic the system well before your killed task
> gets a chance to terminate.
> 

Not necessarily.  We pin a lot of memory with get_user_pages() and 
short-circuit it by checking for fatal_signal_pending() specifically for 
oom conditions.  This was done over six years ago by commit 4779280d1ea4 
("mm: make get_user_pages() interruptible").  When such a process is 
faulting in memory, and it is killed by userspace as a result of an oom 
condition, it needs to be able to allocate (TIF_MEMDIE set by the oom 
killer due to SIGKILL), return to __get_user_pages(), abort, handle the 
signal, and exit.

I can't possibly make that any more clear.

Your patch causes that to instead panic the system if panic_on_oom is set.  
It's inappropriate and userspace breakage.  The fact that I don't 
personally use panic_on_oom is completely and utterly irrelevant.

There is absolutely nothing wrong with a process that has been killed 
either directly by userspace or as part of a group exit, or a process that 
is already in the exit path and needs to allocate memory to be able to 
free its memory, to get access to memory reserves.  That's not an oom 
condition, that's memory reserves.  Panic_on_oom has nothing to do with 
this scenario whatsoever.

Panic_on_oom is not panic_when_reclaim_fails.  It's to suppress a kernel 
oom kill.  That's why it's checked where it is checked and always has 
been.  This patch cannot possibly be merged.

> I would understand your complain if we waited for oom victim(s) before
> check_panic_on_oom but we have not been doing that.
> 

I don't think anybody is changing panic_on_oom after boot, so we wouldn't 
have any oom victims if the oom killer never did anything.
--
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/