Skip to contentSkip to conversation

Use nmi_panic() in panic on NMI case

PATCH Superseded Hidehiro Kawai 5 participants 14 messages Mar 1, 2016

Subsystems (3)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover Use nmi_panic() in panic on NMI case

commit 1717f2096b54 ("panic, x86: Fix re-entrance problem due to panic on NMI") and commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

However, there are some cases where NMI handlers still use panic(). This patch set partially replaces them with nmi_panic() in those cases.

---
Even if applying this patch set, some NMI or similar handlers (e.g. MCE handler) remains to use panic(). This is because I can't test them well and actual problems won't happen. For example, the possibility that normal panic and panic on MCE happen simultaneously is very low.

Hidehiro Kawai (3):
panic: Export panic_cpu and nmi_panic_self_stop ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler
hpwdt: Use nmi_panic() when kernel panics in NMI handler

 drivers/char/ipmi/ipmi_watchdog.c |    2 +-
 drivers/watchdog/hpwdt.c          |   12 ++++++++++--
 kernel/panic.c                    |    2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)
Signature · 2 lines
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
#1 lore raw

PATCH 1/3 panic: Export panic_cpu and nmi_panic_self_stop

Export panic_cpu and nmi_panic_self_stop symbols for modules which use nmi_panic() macro.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc HATAYAMA Daisuke <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc Tejun Heo <[email protected]>
---
 kernel/panic.c |    2 ++
 1 file changed, 2 insertions(+)
kernel/panic.c +2 -0
index d96469d..f4e8035 100644
@@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 {
 	panic_smp_self_stop();
 }
+EXPORT_SYMBOL(nmi_panic_self_stop);
 
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
+EXPORT_SYMBOL(panic_cpu);
 
 /**
  *	panic - halt the system
#2 lore raw

Re: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

Quoted 20 lines · Hidehiro Kawai

Export panic_cpu and nmi_panic_self_stop symbols for modules which use nmi_panic() macro.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc HATAYAMA Daisuke <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc Tejun Heo <[email protected]>
---
 kernel/panic.c |    2 ++
 1 file changed, 2 insertions(+)
kernel/panic.c +0 -0
index d96469d..f4e8035 100644

kernel/panic.c void __weak nmi_panic_self_stop(struct pt_regs *regs) · line 75

Quoted 7 lines
kernel/panic.c +2 -0
@@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 {
 	panic_smp_self_stop();
 }
+EXPORT_SYMBOL(nmi_panic_self_stop);
 
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
+EXPORT_SYMBOL(panic_cpu);

Can we make nmi_panic() at least a proper function and export that instead of exporting all those implementation details...?

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#3 lore raw

RE: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

Hi Borislav,

Quoted 27 lines

From: Borislav Petkov [mailto:[email protected]]

Quoted 25 lines · Hidehiro Kawai

Export panic_cpu and nmi_panic_self_stop symbols for modules which use nmi_panic() macro.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc HATAYAMA Daisuke <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc Tejun Heo <[email protected]>
---
 kernel/panic.c |    2 ++
 1 file changed, 2 insertions(+)
kernel/panic.c +1 -0
index d96469d..f4e8035 100644
@@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 {
 	panic_smp_self_stop();
 }
+EXPORT_SYMBOL(nmi_panic_self_stop);
 
Quoted 5 lines

kernel/panic.c void __weak nmi_panic_self_stop(struct pt_regs *regs) · line 75

Quoted 2 lines
kernel/panic.c +1 -0
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
+EXPORT_SYMBOL(panic_cpu);

Can we make nmi_panic() at least a proper function and export that instead of exporting all those implementation details...?

The reason I implemented nmi_panic() as a macro is to pass variable arguments directly to panic(). Fortunately, since all invocations of nmi_panic() just pass a fixed string, I can change it to a normal function like:

int nmi_panic(struct pt_regs *regs, const char *msg) {
...

	panic("%s", msg);

If people don't mind if that, I'll change it.

Regards,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

#4 lore raw

Re: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

Quoted 40 lines · 河合英宏 / KAWAI,HIDEHIRO

Hi Borislav,

Quoted 27 lines

From: Borislav Petkov [mailto:[email protected]]

Quoted 25 lines · Hidehiro Kawai

Export panic_cpu and nmi_panic_self_stop symbols for modules which use nmi_panic() macro.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc HATAYAMA Daisuke <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc Tejun Heo <[email protected]>
---
 kernel/panic.c |    2 ++
 1 file changed, 2 insertions(+)
kernel/panic.c +1 -0
index d96469d..f4e8035 100644
@@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 {
 	panic_smp_self_stop();
 }
+EXPORT_SYMBOL(nmi_panic_self_stop);
 
Quoted 5 lines

kernel/panic.c void __weak nmi_panic_self_stop(struct pt_regs *regs) · line 75

Quoted 2 lines
kernel/panic.c +1 -0
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
+EXPORT_SYMBOL(panic_cpu);

Can we make nmi_panic() at least a proper function and export that instead of exporting all those implementation details...?

The reason I implemented nmi_panic() as a macro is to pass variable arguments directly to panic(). Fortunately, since all invocations of nmi_panic() just pass a fixed string, I can change it to a normal function like:

Quoted 6 lines

int nmi_panic(struct pt_regs *regs, const char *msg) {
...

	panic("%s", msg);

If people don't mind if that, I'll change it.

Yeah, I think this is better. If we ever need a variable strings we can reconsider.

Signature · 2 lines
Michal Hocko
SUSE Labs
#5 lore raw

PATCH 2/3 ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

In reply to Hidehiro Kawai

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

ipmi_watchdog driver can call panic() from NMI handler, so replace it with nmi_panic().

---
 drivers/char/ipmi/ipmi_watchdog.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/ipmi/ipmi_watchdog.c +1 -1
index 096f0ce..4facc75 100644
@@ -1140,7 +1140,7 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)
 		   the timer.   So do so. */
 		pretimeout_since_last_heartbeat = 1;
 		if (atomic_inc_and_test(&preop_panic_excl))
-			panic(PFX "pre-timeout");
+			nmi_panic(regs, PFX "pre-timeout");
 	}
 
 	return NMI_HANDLED;
maintainer Acked-by on v1 2/3
#6 lore raw

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

Quoted 11 lines · Hidehiro Kawai

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

ipmi_watchdog driver can call panic() from NMI handler, so replace it with nmi_panic().

Quoted 24 lines
---
  drivers/char/ipmi/ipmi_watchdog.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/ipmi/ipmi_watchdog.c +1 -2
index 096f0ce..4facc75 100644
@@ -1140,7 +1140,7 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)
  		   the timer.   So do so. */
  		pretimeout_since_last_heartbeat = 1;
  		if (atomic_inc_and_test(&preop_panic_excl))
-			panic(PFX "pre-timeout");
+			nmi_panic(regs, PFX "pre-timeout");
  	}
 
  	return NMI_HANDLED;
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by on v1 2/3
#7 lore raw

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

In reply to Hidehiro Kawai

Sure, this is a good idea.

Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix re-entrance problem due to panic on NMI) and then the regs field was added in the commit you reference.

Do you want me to add this to the IPMI queue or do you have another way to get this patch into the kernel?

-corey

Quoted 30 lines · Hidehiro Kawai

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

ipmi_watchdog driver can call panic() from NMI handler, so replace it with nmi_panic().

---
  drivers/char/ipmi/ipmi_watchdog.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/ipmi/ipmi_watchdog.c +1 -1
index 096f0ce..4facc75 100644
@@ -1140,7 +1140,7 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)
  		   the timer.   So do so. */
  		pretimeout_since_last_heartbeat = 1;
  		if (atomic_inc_and_test(&preop_panic_excl))
-			panic(PFX "pre-timeout");
+			nmi_panic(regs, PFX "pre-timeout");
  	}
 
  	return NMI_HANDLED;
#8 lore raw

RE: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

Hi Corey,

Thanks for the review.

Quoted 7 lines

Sure, this is a good idea.

Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix re-entrance problem due to panic on NMI) and then the regs field was added in the commit you reference.

Yes. So, I'll change the description to more proper one.

Quoted 2 lines

Do you want me to add this to the IPMI queue or do you have another way to get this patch into the kernel?

I don't have another way, and I don't know how cross-subsystem patch set should be handled.

I think it would be better this patch set is managed by one person because both PATCH 2/3 and 3/3 depend on 1/3.

Thanks,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

maintainer
#9 lore raw

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

Quoted 14 lines · 河合英宏 / KAWAI,HIDEHIRO

Hi Corey,

Thanks for the review.

Quoted 7 lines

Sure, this is a good idea.

Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix re-entrance problem due to panic on NMI) and then the regs field was added in the commit you reference.

Yes. So, I'll change the description to more proper one.

Quoted 9 lines
Quoted 2 lines

Do you want me to add this to the IPMI queue or do you have another way to get this patch into the kernel?

I don't have another way, and I don't know how cross-subsystem patch set should be handled.

I think it would be better this patch set is managed by one person because both PATCH 2/3 and 3/3 depend on 1/3.

We'll need an Ack from someone with authority over kernel/panic.c. If someone sends an Ack, the series can go through IPMI (or watchdog). It might be easier though if Andrew would take the entire series.

Guenter

Reviewed-by on v1 2/3
#10 lore raw

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

In reply to Hidehiro Kawai
Quoted 11 lines · Hidehiro Kawai

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

ipmi_watchdog driver can call panic() from NMI handler, so replace it with nmi_panic().

Quoted 18 lines
---
 drivers/char/ipmi/ipmi_watchdog.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/ipmi/ipmi_watchdog.c +1 -1
index 096f0ce..4facc75 100644
@@ -1140,7 +1140,7 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)
 		   the timer.   So do so. */
 		pretimeout_since_last_heartbeat = 1;
 		if (atomic_inc_and_test(&preop_panic_excl))
-			panic(PFX "pre-timeout");
+			nmi_panic(regs, PFX "pre-timeout");
 	}
 
 	return NMI_HANDLED;
Signature · 2 lines
Michal Hocko
SUSE Labs
#11 lore raw

PATCH 3/3 hpwdt: Use nmi_panic() when kernel panics in NMI handler

In reply to Hidehiro Kawai

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

hpwdt driver can call panic() from NMI handler, so replace it with nmi_panic().

Cc Thomas Mingarelli <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>
---
 drivers/watchdog/hpwdt.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/watchdog/hpwdt.c +10 -2
index 92443c3..fd0486f 100644
@@ -496,11 +496,12 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 
 	if (!is_icru && !is_uefi) {
 		if (cmn_regs.u1.ral == 0) {
-			panic("An NMI occurred, "
+			nmi_panic(regs, "An NMI occurred, "
 				"but unable to determine source.\n");
+			goto handled;
 		}
 	}
-	panic("An NMI occurred. Depending on your system the reason "
+	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
 		"for the NMI is logged in any one of the following "
 		"resources:\n"
 		"1. Integrated Management Log (IML)\n"
@@ -508,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 		"3. OA Forward Progress Log\n"
 		"4. iLO Event Log");
 
+handled:
+	/*
+	 * Returning from nmi_panic() means this CPU was processing panic()
+	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
+	 */
+	return NMI_HANDLED;
+
 out:
 	return NMI_DONE;
 }
maintainer
#12 lore raw

Re: [PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler

Quoted 22 lines · Hidehiro Kawai

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

hpwdt driver can call panic() from NMI handler, so replace it with nmi_panic().

Cc Thomas Mingarelli <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>
---
  drivers/watchdog/hpwdt.c |   12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)
drivers/watchdog/hpwdt.c +0 -0
index 92443c3..fd0486f 100644

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 500

Quoted 6 lines
drivers/watchdog/hpwdt.c +1 -1
@@ -496,11 +496,12 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 
  	if (!is_icru && !is_uefi) {
  		if (cmn_regs.u1.ral == 0) {
-			panic("An NMI occurred, "
+			nmi_panic(regs, "An NMI occurred, "
  				"but unable to determine source.\n");

This message should really be in a single line.
Sure, strictly speaking that should be done in a separate patch, but since you touch the line you might as well fix it.

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 501

Quoted 1 line
drivers/watchdog/hpwdt.c +1 -0
+			goto handled;

The goto is unnecessary. Just return NMI_HANDLED.

Quoted 8 lines
drivers/watchdog/hpwdt.c +1 -1
  		}
  	}
-	panic("An NMI occurred. Depending on your system the reason "
+	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
  		"for the NMI is logged in any one of the following "
  		"resources:\n"
  		"1. Integrated Management Log (IML)\n"

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 516

Quoted 8 lines
drivers/watchdog/hpwdt.c +5 -0
@@ -508,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
  		"3. OA Forward Progress Log\n"
  		"4. iLO Event Log");
 
+handled:
+	/*
+	 * Returning from nmi_panic() means this CPU was processing panic()
+	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
+	 */

I don't think this comment adds much if any value.

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 519

Quoted 3 lines
drivers/watchdog/hpwdt.c +2 -0
+	return NMI_HANDLED;
+
  out:

Any goto to this label does no longer serve a useful purpose (if it ever served one), and should be replaced with "return NMI_DONE;"

Quoted 5 lines
drivers/watchdog/hpwdt.c +0 -0
  	return NMI_DONE;
  }
#13 lore raw

RE: [PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler

Hi Guenter,

Thanks for the review.

Quoted 24 lines · Hidehiro Kawai
Quoted 23 lines

commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.

hpwdt driver can call panic() from NMI handler, so replace it with nmi_panic().

Cc Thomas Mingarelli <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>
---
  drivers/watchdog/hpwdt.c |   12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)
drivers/watchdog/hpwdt.c +0 -0
index 92443c3..fd0486f 100644
@@ -496,11 +496,12 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 
Quoted 9 lines

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 500

Quoted 5 lines
drivers/watchdog/hpwdt.c +1 -1
  	if (!is_icru && !is_uefi) {
  		if (cmn_regs.u1.ral == 0) {
-			panic("An NMI occurred, "
+			nmi_panic(regs, "An NMI occurred, "
  				"but unable to determine source.\n");

This message should really be in a single line.
Sure, strictly speaking that should be done in a separate patch, but since you touch the line you might as well fix it.

OK, I'll combine them.

Quoted 3 lines

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 501

Quoted 1 line
drivers/watchdog/hpwdt.c +1 -0
+			goto handled;

The goto is unnecessary. Just return NMI_HANDLED.

Sure, I'll do that.

Quoted 12 lines
Quoted 12 lines
drivers/watchdog/hpwdt.c +1 -1
  		}
  	}
-	panic("An NMI occurred. Depending on your system the reason "
+	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
  		"for the NMI is logged in any one of the following "
  		"resources:\n"
  		"1. Integrated Management Log (IML)\n"
@@ -508,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
  		"3. OA Forward Progress Log\n"
  		"4. iLO Event Log");
 
Quoted 7 lines

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 516

Quoted 5 lines
drivers/watchdog/hpwdt.c +5 -0
+handled:
+	/*
+	 * Returning from nmi_panic() means this CPU was processing panic()
+	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
+	 */

I don't think this comment adds much if any value.

So, I remove this comment. NMI_HANDLED would be self-explanatory.

Quoted 6 lines

drivers/watchdog/hpwdt.c static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) · line 519

Quoted 3 lines
drivers/watchdog/hpwdt.c +2 -0
+	return NMI_HANDLED;
+
  out:

Any goto to this label does no longer serve a useful purpose (if it ever served one), and should be replaced with "return NMI_DONE;"

I'll fix it. It will become a bit simpler.

Thanks,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group