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

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

[HATAYAMA Daisuke]

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

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

[Masami Hiramatsu]

(2015/03/04 17:56), 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().
> 

Thank you for fixing this issue :)

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

I think this is fundamentally needed, because crash_kexec (a.k.a.
kdump) is mainly for shooting bugs which cause kernel panic.
Thus, to avoid the scenario that something went wrong on kernel
panic and failed to get kdump, we have to call panic_notifiers
before that.

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


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

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

[Vivek Goyal]

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.

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
> 

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

[Vivek Goyal]

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

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

[HATAYAMA Daisuke]

From: Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [RESEND PATCH] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path
Date: Thu, 5 Mar 2015 17:22:04 -0500

> 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 for your reviewing.

Yes, I'll put such new comment in the patch of next version.

--
Thanks.
HATAYAMA, Daisuke