linux-kernel - Re: [PATCH] oom: split out forced OOM killer

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: <20150608210621.GA18360@dhcp22.suse.cz>
Date:	Mon, 8 Jun 2015 23:06:21 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Austin S Hemmelgarn <ahferroin7@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] oom: split out forced OOM killer

On Mon 08-06-15 12:41:48, David Rientjes wrote:
> On Mon, 8 Jun 2015, Austin S Hemmelgarn wrote:
> 
> > I believe so (haven't actually read the patch itself, just the changelog),
> > although it is only a change for certain configurations to a very specific and
> > (I hope infrequently) used piece of functionality. Like I said above, if I
> > wanted to crash my system, I'd be using sysrq-c; and if I'm using sysrq-f, I
> > want _some_ task to die _now_.
> > 
> 
> This patch is not a functional change, so I don't interpret your feedback 
> as any support of it being merged.

David, have you actually read the patch? The changelog is mentioning this:
"
    check_panic_on_oom on the other hand will work and that is kind of
    unexpected because sysrq+f should be usable to kill a mem hog whether
    the global OOM policy is to panic or not.
    It also doesn't make much sense to panic the system when no task cannot
    be killed because admin has a separate sysrq for that purpose.
"
and the patch exludes panic_on_oom from the sysrq path.

> That said, you raise an interesting point of whether sysrq+f should ever 
> trigger a panic due to panic_on_oom.  The case can be made that it should 
> ignore panic_on_oom and require the use of another sysrq to panic the 
> machine instead.  Sysrq+f could then be used to oom kill a process, 
> regardless of panic_on_oom, and the panic only occurs if userspace did not 
> trigger the kill or the kill itself will fail.

Why would it panic the system if there is no killable task? Shoudln't
be admin able to do additional steps after the explicit oom killer failed
and only then panic by sysrq?

> I think we should pursue that direction.
> 
> This patch also changes the text which is output to the kernel log on 
> panic, which we use to parse for machines that have crashed due to no 
> killable memcg processes, so NACK on this patch. 

Could you point to the code snippet which does that? Because the only
change to the output is for the forced oom killer.

> There's also no reason 
> to add more source code to try to make things cleaner when it just 
> obfuscates the oom killer code more than it needs to (we don't need to 
> optimize or have multiple entry points).

I am not sure I understand your objection here. The forced oom killer
path is now clear (__oom_kill_process does the dirty job while
oom_kill_process can do heuristics to prevent from pointless killing),
easier to follow and thus more maintainable from my POV.
I could understand your objection if this has added a lot of code but
 4 files changed, 58 insertions(+), 23 deletions(-)

which seems appropriate to me.

-- 
Michal Hocko
SUSE Labs
--
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/