Use nmi_panic() in panic on NMI case
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
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.
--- kernel/panic.c | 2 ++ 1 file changed, 2 insertions(+)
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);/** * panic - halt the system
Re: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop
Quoted 20 lines · Hidehiro Kawai
Export
panic_cpuandnmi_panic_self_stopsymbols for modules which usenmi_panic()macro.Signed-off-by Hidehiro Kawai <[email protected]>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
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.
--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_cpuandnmi_panic_self_stopsymbols for modules which usenmi_panic()macro.Signed-off-by Hidehiro Kawai <[email protected]>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@@ -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
-0atomic_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
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_cpuandnmi_panic_self_stopsymbols for modules which usenmi_panic()macro.Signed-off-by Hidehiro Kawai <[email protected]>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@@ -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
-0atomic_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 topanic(). Fortunately, since all invocations ofnmi_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
PATCH 2/3 ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro Kawaicommit 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
@@ -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 ;
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") introducednmi_panic()which prevents concurrent/recursive execution ofpanic(). It also saves registers for the crash dump on x86.
ipmi_watchdogdriver can callpanic()from NMI handler, so replace it withnmi_panic().Signed-off-by Hidehiro Kawai <[email protected]>Cc Corey Minyard <[email protected]>
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@@ -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
Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro KawaiSure, 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") introducednmi_panic()which prevents concurrent/recursive execution ofpanic(). It also saves registers for the crash dump on x86.
ipmi_watchdogdriver can callpanic()from NMI handler, so replace it withnmi_panic().Signed-off-by Hidehiro Kawai <[email protected]>Cc Corey Minyard <[email protected]>--- drivers/char/ipmi/ipmi_watchdog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/char/ipmi/ipmi_watchdog.c +1
-1@@ -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 ;
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.
Acked-by Corey Minyard <[email protected]>Note that
nmi_panic()came in commit1717f2096b5(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
Guenter Roeck
maintainerRe: [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.
Acked-by Corey Minyard <[email protected]>Note that
nmi_panic()came in commit1717f2096b5(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
Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro KawaiQuoted 11 lines · Hidehiro Kawai
commit
58c5661f2144("panic, x86: Allow CPUs to save registers even if looping in NMI context") introducednmi_panic()which prevents concurrent/recursive execution ofpanic(). It also saves registers for the crash dump on x86.
ipmi_watchdogdriver can callpanic()from NMI handler, so replace it withnmi_panic().Signed-off-by Hidehiro Kawai <[email protected]>Cc Corey Minyard <[email protected]>
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@@ -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
PATCH 3/3 hpwdt: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro Kawaicommit 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().
--- drivers/watchdog/hpwdt.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/watchdog/hpwdt.c +10 -2
@@ -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 ; }
Guenter Roeck
maintainerRe: [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") introducednmi_panic()which prevents concurrent/recursive execution ofpanic(). It also saves registers for the crash dump on x86.hpwdt driver can call
panic()from NMI handler, so replace it withnmi_panic().Signed-off-by Hidehiro Kawai <[email protected]>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
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
-0return NMI_DONE ; }
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") introducednmi_panic()which prevents concurrent/recursive execution ofpanic(). It also saves registers for the crash dump on x86.hpwdt driver can call
panic()from NMI handler, so replace it withnmi_panic().Signed-off-by Hidehiro Kawai <[email protected]>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@@ -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
-1if (!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