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

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Mar 05, 2015 at 05:19:30PM -0500, Vivek Goyal wrote:
> On Wed, Mar 04, 2015 at 05:56:48PM +0900, HATAYAMA Daisuke wrote:
> > The commit f06e5153f4ae2e2f3b0300f0e260e40cb7fefd45 introduced
> > "crash_kexec_post_notifiers" kernel boot option, which toggles
> > wheather panic() calls crash_kexec() before or after panic_notifiers
> > and dump kmsg.
> > 
> > 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().
> > 
> > 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>
> > ---
> >  include/linux/kernel.h | 3 +++
> >  kernel/kexec.c         | 2 ++
> >  kernel/panic.c         | 2 +-
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 64ce58b..f47379f 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -426,6 +426,9 @@ extern int panic_on_unrecovered_nmi;
> >  extern int panic_on_io_nmi;
> >  extern int panic_on_warn;
> >  extern int sysctl_panic_on_stackoverflow;
> > +
> > +extern bool crash_kexec_post_notifiers;
> > +
> >  /*
> >   * Only to be used by arch init code. If the user over-wrote the default
> >   * CONFIG_PANIC_TIMEOUT, honor it.
> > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > index 9a8a01a..0ecf252 100644
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -84,6 +84,8 @@ struct resource crashk_low_res = {
> >  
> >  int kexec_should_crash(struct task_struct *p)
> >  {
> > +	if (crash_kexec_post_notifiers)
> > +		return 0;
> 
> This is little confusing. So if crash_kexec_post_notifiers is set but
> panic_on_oops is not set, still we will return?
> 
> Should we do this only if panic_on_oops is set? IOW, how about following
> 
> 	if (panic_on_oops && crash_kexec_post_notifiers)
> 		return 0;
> 
> And then also put a comment explaining the rationale.

Ok, I went through the previous version of patch and discussion there
which says that all the 4 conditions lead to panic. So putting above
code should be fine.

Can you please atleast put a comment here to explain it as it was not
obvious. Just mention that all the checks below lead to panic hence
if user wants to run panic notifiers then don't run crash_kexec() yet.
It will be run after panic notifiers.

Thanks
Vivek

> 
> Thanks
> Vivek
> 
> >  	if (in_interrupt() || !p->pid || is_global_init(p) || panic_on_oops)
> >  		return 1;
> >  	return 0;
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 4d8d6f9..6582546 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -32,7 +32,7 @@ static unsigned long tainted_mask;
> >  static int pause_on_oops;
> >  static int pause_on_oops_flag;
> >  static DEFINE_SPINLOCK(pause_on_oops_lock);
> > -static bool crash_kexec_post_notifiers;
> > +bool crash_kexec_post_notifiers;
> >  int panic_on_warn __read_mostly;
> >  
> >  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> > -- 
> > 1.9.3
> >