From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752394AbbCXDaU (ORCPT ); Mon, 23 Mar 2015 23:30:20 -0400 Received: from mail7.hitachi.co.jp ([133.145.228.42]:35797 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752114AbbCXDaR (ORCPT ); Mon, 23 Mar 2015 23:30:17 -0400 Message-ID: <5510DA42.6040708@hitachi.com> Date: Tue, 24 Mar 2015 12:30:10 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: Ingo Molnar CC: Baoquan He , =?UTF-8?B?IkhhdGF5YW1hLCBEYWlzdWtlL+eVkeWxsQ==?= =?UTF-8?B?IOWkp+i8lCI=?= , ebiederm@xmission.com, Vivek Goyal , hidehiro.kawai.ez@hitachi.com, linux-kernel@vger.kernel.org, kexec@lists.infradead.org, akpm@linux-foundation.org, mingo@redhat.com, bp@suse.de Subject: Re: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path References: <54F9D645.2050008@jp.fujitsu.com> <20150323034752.GD2068@dhcp-16-105.nay.redhat.com> <20150323071943.GA22765@gmail.com> In-Reply-To: <20150323071943.GA22765@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (2015/03/23 16:19), Ingo Molnar wrote: > > * Baoquan He wrote: > >> CC more people ... >> >> On 03/07/15 at 01:31am, "Hatayama, Daisuke/畑山 大輔" wrote: >>> The commit f06e5153f4ae2e2f3b0300f0e260e40cb7fefd45 introduced >>> "crash_kexec_post_notifiers" kernel boot option, which toggles >>> wheather panic() calls crash_kexec() before panic_notifiers and dump >>> kmsg or after. >>> >>> The problem is that the commit overlooks panic_on_oops kernel boot >>> option. If it is enabled, crash_kexec() is called directly without >>> going through panic() in oops path. >>> >>> To fix this issue, this patch adds a check to >>> "crash_kexec_post_notifiers" in the condition of kexec_should_crash(). >>> >>> Also, put a comment in kexec_should_crash() to explain not obvious >>> things on this patch. >>> >>> Signed-off-by: HATAYAMA Daisuke >>> Acked-by: Baoquan He >>> Tested-by: Hidehiro Kawai >>> Reviewed-by: Masami Hiramatsu >>> --- >>> include/linux/kernel.h | 3 +++ >>> kernel/kexec.c | 11 +++++++++++ >>> kernel/panic.c | 2 +- >>> 3 files changed, 15 insertions(+), 1 deletion(-) > > This is hack upon hack, but why was this crap merged in the first > place? > > I see two problems just by cursory review: > > 1) > > Firstly, the real bug in: > > f06e5153f4ae ("kernel/panic.c: add "crash_kexec_post_notifiers" option for kdump after panic_notifers") > > Was that crash_kexec() was called unconditionally after notifiers were > called, which should be fixed via the simple patch below (untested). > Looks much simpler than your fix. No, Daisuke's patch is not for that case. Since the kdump has a special hook in kernel oops, when both of panic_on_oops and crash_kernel are set, panic() is never called. Please see oops_end@arch/x86/kernel/dumpstack.c ---- void oops_end(unsigned long flags, struct pt_regs *regs, int signr) { if (regs && kexec_should_crash(current)) crash_kexec(regs); ---- Of course crash_kexec() never return except failing kexec unexpectedly. Thus, kexec_should_crash should returns 0 if crash_kexec_post_notifiers is set. (Semantically, it is a bit strange that panic_on_oops doesn't call panic(), but that is another topic.) However, your patch is also needed since the first crash_kexec() can fail in panic() when crash_kexec_post_notifiers is not set. In that case, kernel tries to call notifiers and call the 2nd crash_kexec() again. Actually the 2nd one is useless. So, here is my reviewed-by. Reviewed-by: Masami Hiramatsu I'll be reply the latter part in other mail. Thank you, > > 2) > > Secondly, and more importantly, the whole premise of commit > f06e5153f4ae is broken IMHO: > > "This can help rare situations where kdump fails because of unstable > crashed kernel or hardware failure (memory corruption on critical > data/code)" > > wtf? > > If the kernel crashed due to a kernel crash, then the kernel booting > up in whatever hardware state should be able to do a clean bootup. The > fix for those 'rare situations' should be to fix the real bug (for > example by making hardware driver init (or deinit) sequences more > robust), not to paper it over by ordering around crash-time sequences > ... > > If it crashed due to some hardware failure, there's literally an > infinite amount of failure modes that may or may not be impacted by > kexec crash-time handling ordering. We don't want to put a zillion > such flags into the kernel proper just to allow the perturbation of > the kernel. > > Thanks, > > Ingo > > diff --git a/kernel/panic.c b/kernel/panic.c > index 8136ad76e5fd..774614f72cbd 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -142,7 +142,8 @@ void panic(const char *fmt, ...) > * Note: since some panic_notifiers can make crashed kernel > * more unstable, it can increase risks of the kdump failure too. > */ > - crash_kexec(NULL); > + if (crash_kexec_post_notifiers) > + crash_kexec(NULL); > > bust_spinlocks(0); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com