Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path

[Ingo Molnar <mingo@kernel.org>]

* Baoquan He <bhe@redhat.com> 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 <d.hatayama@jp.fujitsu.com>
> > Acked-by: Baoquan He <bhe@redhat.com>
> > Tested-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > ---
> >  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.

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);