Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1273947 > unrolled thread

[V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on  NMI Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com> - 2015-11-20 11:30 +0100
    Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI Borislav Petkov <bp@alien8.de> - 2015-11-23 20:00 +0100
      RE: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI 河合英宏 / KAWAI，HIDEHIRO   <hidehiro.kawai.ez@hitachi.com> - 2015-11-24 05:10 +0100
    Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI Michal Hocko <mhocko@kernel.org> - 2015-11-24 13:50 +0100
    Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI Steven Rostedt <rostedt@goodmis.org> - 2015-11-24 16:20 +0100
      Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI Steven Rostedt <rostedt@goodmis.org> - 2015-11-24 16:20 +0100
      Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI Michal Hocko <mhocko@kernel.org> - 2015-11-24 21:30 +0100
        Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic  on NMI Steven Rostedt <rostedt@goodmis.org> - 2015-11-24 21:50 +0100

#1273947 — [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

If panic on NMI happens just after panic() on the same CPU, panic()
is recursively called.  As the result, it stalls after failing to
acquire panic_lock.

To avoid this problem, don't call panic() in NMI context if
we've already entered panic().

V4:
- Improve comments in io_check_error() and panic()

V3:
- Introduce nmi_panic() macro to reduce code duplication
- In the case of panic on NMI, don't return from NMI handlers
  if another cpu already panicked

V2:
- Use atomic_cmpxchg() instead of current spin_trylock() to
  exclude concurrent accesses to the panic routines
- Don't introduce no-lock version of panic()

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Michal Hocko <mhocko@kernel.org>
---
 arch/x86/kernel/nmi.c  |   16 ++++++++++++----
 include/linux/kernel.h |   13 +++++++++++++
 kernel/panic.c         |   15 ++++++++++++---
 kernel/watchdog.c      |    2 +-
 4 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 697f90d..5131714 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -231,7 +231,7 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
 #endif
 
 	if (panic_on_unrecovered_nmi)
-		panic("NMI: Not continuing");
+		nmi_panic("NMI: Not continuing");
 
 	pr_emerg("Dazed and confused, but trying to continue\n");
 
@@ -255,8 +255,16 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
 		 reason, smp_processor_id());
 	show_regs(regs);
 
-	if (panic_on_io_nmi)
-		panic("NMI IOCK error: Not continuing");
+	if (panic_on_io_nmi) {
+		nmi_panic("NMI IOCK error: Not continuing");
+
+		/*
+		 * If we return from nmi_panic(), it means we have received
+		 * NMI while processing panic().  So, simply return without
+		 * a delay and re-enabling NMI.
+		 */
+		return;
+	}
 
 	/* Re-enable the IOCK line, wait for a few seconds */
 	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
@@ -297,7 +305,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 
 	pr_emerg("Do you have a strange power saving mode enabled?\n");
 	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
-		panic("NMI: Not continuing");
+		nmi_panic("NMI: Not continuing");
 
 	pr_emerg("Dazed and confused, but trying to continue\n");
 }
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 350dfb0..480a4fd 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -445,6 +445,19 @@ extern int sysctl_panic_on_stackoverflow;
 
 extern bool crash_kexec_post_notifiers;
 
+extern atomic_t panic_cpu;
+
+/*
+ * A variant of panic() called from NMI context.
+ * If we've already panicked on this cpu, return from here.
+ */
+#define nmi_panic(fmt, ...)						\
+	do {								\
+		int this_cpu = raw_smp_processor_id();			\
+		if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \
+			panic(fmt, ##__VA_ARGS__);			\
+	} while (0)
+
 /*
  * Only to be used by arch init code. If the user over-wrote the default
  * CONFIG_PANIC_TIMEOUT, honor it.
diff --git a/kernel/panic.c b/kernel/panic.c
index 4579dbb..24ee2ea 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -61,6 +61,8 @@ void __weak panic_smp_self_stop(void)
 		cpu_relax();
 }
 
+atomic_t panic_cpu = ATOMIC_INIT(-1);
+
 /**
  *	panic - halt the system
  *	@fmt: The text string to print
@@ -71,17 +73,17 @@ void __weak panic_smp_self_stop(void)
  */
 void panic(const char *fmt, ...)
 {
-	static DEFINE_SPINLOCK(panic_lock);
 	static char buf[1024];
 	va_list args;
 	long i, i_next = 0;
 	int state = 0;
+	int old_cpu, this_cpu;
 
 	/*
 	 * Disable local interrupts. This will prevent panic_smp_self_stop
 	 * from deadlocking the first cpu that invokes the panic, since
 	 * there is nothing to prevent an interrupt handler (that runs
-	 * after the panic_lock is acquired) from invoking panic again.
+	 * after setting panic_cpu) from invoking panic again.
 	 */
 	local_irq_disable();
 
@@ -94,8 +96,15 @@ void panic(const char *fmt, ...)
 	 * multiple parallel invocations of panic, all other CPUs either
 	 * stop themself or will wait until they are stopped by the 1st CPU
 	 * with smp_send_stop().
+	 *
+	 * `old_cpu == -1' means this is the 1st CPU which comes here, so
+	 * go ahead.
+	 * `old_cpu == this_cpu' means we came from nmi_panic() which sets
+	 * panic_cpu to this cpu.  In this case, this is also the 1st CPU.
 	 */
-	if (!spin_trylock(&panic_lock))
+	this_cpu = raw_smp_processor_id();
+	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
+	if (old_cpu != -1 && old_cpu != this_cpu)
 		panic_smp_self_stop();
 
 	console_verbose();
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 18f34cf..b9be18f 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -351,7 +351,7 @@ static void watchdog_overflow_callback(struct perf_event *event,
 			trigger_allbutself_cpu_backtrace();
 
 		if (hardlockup_panic)
-			panic("Hard LOCKUP");
+			nmi_panic("Hard LOCKUP");
 
 		__this_cpu_write(hard_watchdog_warn, true);
 		return;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1275753 — Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

On Fri, Nov 20, 2015 at 06:36:44PM +0900, Hidehiro Kawai wrote:
> If panic on NMI happens just after panic() on the same CPU, panic()
> is recursively called.  As the result, it stalls after failing to
> acquire panic_lock.
> 
> To avoid this problem, don't call panic() in NMI context if
> we've already entered panic().
> 
> V4:
> - Improve comments in io_check_error() and panic()
> 
> V3:
> - Introduce nmi_panic() macro to reduce code duplication
> - In the case of panic on NMI, don't return from NMI handlers
>   if another cpu already panicked
> 
> V2:
> - Use atomic_cmpxchg() instead of current spin_trylock() to
>   exclude concurrent accesses to the panic routines
> - Don't introduce no-lock version of panic()
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> ---
>  arch/x86/kernel/nmi.c  |   16 ++++++++++++----
>  include/linux/kernel.h |   13 +++++++++++++
>  kernel/panic.c         |   15 ++++++++++++---
>  kernel/watchdog.c      |    2 +-
>  4 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 697f90d..5131714 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -231,7 +231,7 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
>  #endif
>  
>  	if (panic_on_unrecovered_nmi)
> -		panic("NMI: Not continuing");
> +		nmi_panic("NMI: Not continuing");
>  
>  	pr_emerg("Dazed and confused, but trying to continue\n");
>  
> @@ -255,8 +255,16 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
>  		 reason, smp_processor_id());
>  	show_regs(regs);
>  
> -	if (panic_on_io_nmi)
> -		panic("NMI IOCK error: Not continuing");
> +	if (panic_on_io_nmi) {
> +		nmi_panic("NMI IOCK error: Not continuing");

Btw, that panic_on_io_nmi seems undocumented in
Documentation/sysctl/kernel.txt. Care to document it, please, as a
separate patch?

> +
> +		/*
> +		 * If we return from nmi_panic(), it means we have received
> +		 * NMI while processing panic().  So, simply return without
> +		 * a delay and re-enabling NMI.
> +		 */
> +		return;
> +	}
>  
>  	/* Re-enable the IOCK line, wait for a few seconds */
>  	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
> @@ -297,7 +305,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
>  
>  	pr_emerg("Do you have a strange power saving mode enabled?\n");
>  	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
> -		panic("NMI: Not continuing");
> +		nmi_panic("NMI: Not continuing");
>  
>  	pr_emerg("Dazed and confused, but trying to continue\n");
>  }
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 350dfb0..480a4fd 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -445,6 +445,19 @@ extern int sysctl_panic_on_stackoverflow;
>  
>  extern bool crash_kexec_post_notifiers;
>  
> +extern atomic_t panic_cpu;

This needs a comment explaining what this variable is and what it
denotes.

> +
> +/*
> + * A variant of panic() called from NMI context.
> + * If we've already panicked on this cpu, return from here.
> + */
> +#define nmi_panic(fmt, ...)						\
> +	do {								\
> +		int this_cpu = raw_smp_processor_id();			\
> +		if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \
> +			panic(fmt, ##__VA_ARGS__);			\
> +	} while (0)
> +
>  /*
>   * Only to be used by arch init code. If the user over-wrote the default
>   * CONFIG_PANIC_TIMEOUT, honor it.
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 4579dbb..24ee2ea 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -61,6 +61,8 @@ void __weak panic_smp_self_stop(void)
>  		cpu_relax();
>  }
>  
> +atomic_t panic_cpu = ATOMIC_INIT(-1);
> +
>  /**
>   *	panic - halt the system
>   *	@fmt: The text string to print
> @@ -71,17 +73,17 @@ void __weak panic_smp_self_stop(void)
>   */
>  void panic(const char *fmt, ...)
>  {
> -	static DEFINE_SPINLOCK(panic_lock);
>  	static char buf[1024];
>  	va_list args;
>  	long i, i_next = 0;
>  	int state = 0;
> +	int old_cpu, this_cpu;
>  
>  	/*
>  	 * Disable local interrupts. This will prevent panic_smp_self_stop
>  	 * from deadlocking the first cpu that invokes the panic, since
>  	 * there is nothing to prevent an interrupt handler (that runs
> -	 * after the panic_lock is acquired) from invoking panic again.
> +	 * after setting panic_cpu) from invoking panic again.
>  	 */
>  	local_irq_disable();
>  
> @@ -94,8 +96,15 @@ void panic(const char *fmt, ...)
>  	 * multiple parallel invocations of panic, all other CPUs either
>  	 * stop themself or will wait until they are stopped by the 1st CPU
>  	 * with smp_send_stop().
> +	 *
> +	 * `old_cpu == -1' means this is the 1st CPU which comes here, so
> +	 * go ahead.
> +	 * `old_cpu == this_cpu' means we came from nmi_panic() which sets
> +	 * panic_cpu to this cpu.  In this case, this is also the 1st CPU.

I'd prefer that -1 to be

#define INVALID_CPU_NUM		-1

or

#define UNDEFINED_CPU_NUM	-1

or so instead of a naked number.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276047 — RE: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

SGksDQoNCj4gT24gRnJpLCBOb3YgMjAsIDIwMTUgYXQgMDY6MzY6NDRQTSArMDkwMCwgSGlkZWhp
cm8gS2F3YWkgd3JvdGU6DQo+ID4gSWYgcGFuaWMgb24gTk1JIGhhcHBlbnMganVzdCBhZnRlciBw
YW5pYygpIG9uIHRoZSBzYW1lIENQVSwgcGFuaWMoKQ0KPiA+IGlzIHJlY3Vyc2l2ZWx5IGNhbGxl
ZC4gIEFzIHRoZSByZXN1bHQsIGl0IHN0YWxscyBhZnRlciBmYWlsaW5nIHRvDQo+ID4gYWNxdWly
ZSBwYW5pY19sb2NrLg0KPiA+DQo+ID4gVG8gYXZvaWQgdGhpcyBwcm9ibGVtLCBkb24ndCBjYWxs
IHBhbmljKCkgaW4gTk1JIGNvbnRleHQgaWYNCj4gPiB3ZSd2ZSBhbHJlYWR5IGVudGVyZWQgcGFu
aWMoKS4NCj4gPg0KPiA+IFY0Og0KPiA+IC0gSW1wcm92ZSBjb21tZW50cyBpbiBpb19jaGVja19l
cnJvcigpIGFuZCBwYW5pYygpDQo+ID4NCj4gPiBWMzoNCj4gPiAtIEludHJvZHVjZSBubWlfcGFu
aWMoKSBtYWNybyB0byByZWR1Y2UgY29kZSBkdXBsaWNhdGlvbg0KPiA+IC0gSW4gdGhlIGNhc2Ug
b2YgcGFuaWMgb24gTk1JLCBkb24ndCByZXR1cm4gZnJvbSBOTUkgaGFuZGxlcnMNCj4gPiAgIGlm
IGFub3RoZXIgY3B1IGFscmVhZHkgcGFuaWNrZWQNCj4gPg0KPiA+IFYyOg0KPiA+IC0gVXNlIGF0
b21pY19jbXB4Y2hnKCkgaW5zdGVhZCBvZiBjdXJyZW50IHNwaW5fdHJ5bG9jaygpIHRvDQo+ID4g
ICBleGNsdWRlIGNvbmN1cnJlbnQgYWNjZXNzZXMgdG8gdGhlIHBhbmljIHJvdXRpbmVzDQo+ID4g
LSBEb24ndCBpbnRyb2R1Y2Ugbm8tbG9jayB2ZXJzaW9uIG9mIHBhbmljKCkNCj4gPg0KPiA+IFNp
Z25lZC1vZmYtYnk6IEhpZGVoaXJvIEthd2FpIDxoaWRlaGlyby5rYXdhaS5lekBoaXRhY2hpLmNv
bT4NCj4gPiBDYzogQW5kcmV3IE1vcnRvbiA8YWtwbUBsaW51eC1mb3VuZGF0aW9uLm9yZz4NCj4g
PiBDYzogVGhvbWFzIEdsZWl4bmVyIDx0Z2x4QGxpbnV0cm9uaXguZGU+DQo+ID4gQ2M6IEluZ28g
TW9sbmFyIDxtaW5nb0ByZWRoYXQuY29tPg0KPiA+IENjOiAiSC4gUGV0ZXIgQW52aW4iIDxocGFA
enl0b3IuY29tPg0KPiA+IENjOiBQZXRlciBaaWpsc3RyYSA8cGV0ZXJ6QGluZnJhZGVhZC5vcmc+
DQo+ID4gQ2M6IE1pY2hhbCBIb2NrbyA8bWhvY2tvQGtlcm5lbC5vcmc+DQo+ID4gLS0tDQo+ID4g
IGFyY2gveDg2L2tlcm5lbC9ubWkuYyAgfCAgIDE2ICsrKysrKysrKysrKy0tLS0NCj4gPiAgaW5j
bHVkZS9saW51eC9rZXJuZWwuaCB8ICAgMTMgKysrKysrKysrKysrKw0KPiA+ICBrZXJuZWwvcGFu
aWMuYyAgICAgICAgIHwgICAxNSArKysrKysrKysrKystLS0NCj4gPiAga2VybmVsL3dhdGNoZG9n
LmMgICAgICB8ICAgIDIgKy0NCj4gPiAgNCBmaWxlcyBjaGFuZ2VkLCAzOCBpbnNlcnRpb25zKCsp
LCA4IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2FyY2gveDg2L2tlcm5lbC9u
bWkuYyBiL2FyY2gveDg2L2tlcm5lbC9ubWkuYw0KPiA+IGluZGV4IDY5N2Y5MGQuLjUxMzE3MTQg
MTAwNjQ0DQo+ID4gLS0tIGEvYXJjaC94ODYva2VybmVsL25taS5jDQo+ID4gKysrIGIvYXJjaC94
ODYva2VybmVsL25taS5jDQo+ID4gQEAgLTIzMSw3ICsyMzEsNyBAQCBwY2lfc2Vycl9lcnJvcih1
bnNpZ25lZCBjaGFyIHJlYXNvbiwgc3RydWN0IHB0X3JlZ3MgKnJlZ3MpDQo+ID4gICNlbmRpZg0K
PiA+DQo+ID4gIAlpZiAocGFuaWNfb25fdW5yZWNvdmVyZWRfbm1pKQ0KPiA+IC0JCXBhbmljKCJO
TUk6IE5vdCBjb250aW51aW5nIik7DQo+ID4gKwkJbm1pX3BhbmljKCJOTUk6IE5vdCBjb250aW51
aW5nIik7DQo+ID4NCj4gPiAgCXByX2VtZXJnKCJEYXplZCBhbmQgY29uZnVzZWQsIGJ1dCB0cnlp
bmcgdG8gY29udGludWVcbiIpOw0KPiA+DQo+ID4gQEAgLTI1NSw4ICsyNTUsMTYgQEAgaW9fY2hl
Y2tfZXJyb3IodW5zaWduZWQgY2hhciByZWFzb24sIHN0cnVjdCBwdF9yZWdzICpyZWdzKQ0KPiA+
ICAJCSByZWFzb24sIHNtcF9wcm9jZXNzb3JfaWQoKSk7DQo+ID4gIAlzaG93X3JlZ3MocmVncyk7
DQo+ID4NCj4gPiAtCWlmIChwYW5pY19vbl9pb19ubWkpDQo+ID4gLQkJcGFuaWMoIk5NSSBJT0NL
IGVycm9yOiBOb3QgY29udGludWluZyIpOw0KPiA+ICsJaWYgKHBhbmljX29uX2lvX25taSkgew0K
PiA+ICsJCW5taV9wYW5pYygiTk1JIElPQ0sgZXJyb3I6IE5vdCBjb250aW51aW5nIik7DQo+IA0K
PiBCdHcsIHRoYXQgcGFuaWNfb25faW9fbm1pIHNlZW1zIHVuZG9jdW1lbnRlZCBpbg0KPiBEb2N1
bWVudGF0aW9uL3N5c2N0bC9rZXJuZWwudHh0LiBDYXJlIHRvIGRvY3VtZW50IGl0LCBwbGVhc2Us
IGFzIGENCj4gc2VwYXJhdGUgcGF0Y2g/DQoNClN1cmUuIEknbGwgcG9zdCBhIGRvY3VtZW50YXRp
b24gcGF0Y2ggZm9yIGl0IGluIGEgc2VwYXJhdGUgcGF0Y2guDQpCZWNhdXNlIHBhbmljX29uX2lv
X25taSBoYXMgYmVlbiBhdmFpbGFibGUgc2luY2UgcmVsYXRpdmVseSBvbGQgZGF5cywNCkkgZGlk
bid0IGNoZWNrIGl0LiANCg0KPiA+ICsNCj4gPiArCQkvKg0KPiA+ICsJCSAqIElmIHdlIHJldHVy
biBmcm9tIG5taV9wYW5pYygpLCBpdCBtZWFucyB3ZSBoYXZlIHJlY2VpdmVkDQo+ID4gKwkJICog
Tk1JIHdoaWxlIHByb2Nlc3NpbmcgcGFuaWMoKS4gIFNvLCBzaW1wbHkgcmV0dXJuIHdpdGhvdXQN
Cj4gPiArCQkgKiBhIGRlbGF5IGFuZCByZS1lbmFibGluZyBOTUkuDQo+ID4gKwkJICovDQo+ID4g
KwkJcmV0dXJuOw0KPiA+ICsJfQ0KPiA+DQo+ID4gIAkvKiBSZS1lbmFibGUgdGhlIElPQ0sgbGlu
ZSwgd2FpdCBmb3IgYSBmZXcgc2Vjb25kcyAqLw0KPiA+ICAJcmVhc29uID0gKHJlYXNvbiAmIE5N
SV9SRUFTT05fQ0xFQVJfTUFTSykgfCBOTUlfUkVBU09OX0NMRUFSX0lPQ0hLOw0KPiA+IEBAIC0y
OTcsNyArMzA1LDcgQEAgdW5rbm93bl9ubWlfZXJyb3IodW5zaWduZWQgY2hhciByZWFzb24sIHN0
cnVjdCBwdF9yZWdzICpyZWdzKQ0KPiA+DQo+ID4gIAlwcl9lbWVyZygiRG8geW91IGhhdmUgYSBz
dHJhbmdlIHBvd2VyIHNhdmluZyBtb2RlIGVuYWJsZWQ/XG4iKTsNCj4gPiAgCWlmICh1bmtub3du
X25taV9wYW5pYyB8fCBwYW5pY19vbl91bnJlY292ZXJlZF9ubWkpDQo+ID4gLQkJcGFuaWMoIk5N
STogTm90IGNvbnRpbnVpbmciKTsNCj4gPiArCQlubWlfcGFuaWMoIk5NSTogTm90IGNvbnRpbnVp
bmciKTsNCj4gPg0KPiA+ICAJcHJfZW1lcmcoIkRhemVkIGFuZCBjb25mdXNlZCwgYnV0IHRyeWlu
ZyB0byBjb250aW51ZVxuIik7DQo+ID4gIH0NCj4gPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51
eC9rZXJuZWwuaCBiL2luY2x1ZGUvbGludXgva2VybmVsLmgNCj4gPiBpbmRleCAzNTBkZmIwLi40
ODBhNGZkIDEwMDY0NA0KPiA+IC0tLSBhL2luY2x1ZGUvbGludXgva2VybmVsLmgNCj4gPiArKysg
Yi9pbmNsdWRlL2xpbnV4L2tlcm5lbC5oDQo+ID4gQEAgLTQ0NSw2ICs0NDUsMTkgQEAgZXh0ZXJu
IGludCBzeXNjdGxfcGFuaWNfb25fc3RhY2tvdmVyZmxvdzsNCj4gPg0KPiA+ICBleHRlcm4gYm9v
bCBjcmFzaF9rZXhlY19wb3N0X25vdGlmaWVyczsNCj4gPg0KPiA+ICtleHRlcm4gYXRvbWljX3Qg
cGFuaWNfY3B1Ow0KPiANCj4gVGhpcyBuZWVkcyBhIGNvbW1lbnQgZXhwbGFpbmluZyB3aGF0IHRo
aXMgdmFyaWFibGUgaXMgYW5kIHdoYXQgaXQNCj4gZGVub3Rlcy4NCg0KT0ssIEknbGwgZG8gdGhh
dCBpbiB0aGUgbmV4dCB2ZXJzaW9uLg0KDQo+IA0KPiA+ICsNCj4gPiArLyoNCj4gPiArICogQSB2
YXJpYW50IG9mIHBhbmljKCkgY2FsbGVkIGZyb20gTk1JIGNvbnRleHQuDQo+ID4gKyAqIElmIHdl
J3ZlIGFscmVhZHkgcGFuaWNrZWQgb24gdGhpcyBjcHUsIHJldHVybiBmcm9tIGhlcmUuDQo+ID4g
KyAqLw0KPiA+ICsjZGVmaW5lIG5taV9wYW5pYyhmbXQsIC4uLikJCQkJCQlcDQo+ID4gKwlkbyB7
CQkJCQkJCQlcDQo+ID4gKwkJaW50IHRoaXNfY3B1ID0gcmF3X3NtcF9wcm9jZXNzb3JfaWQoKTsJ
CQlcDQo+ID4gKwkJaWYgKGF0b21pY19jbXB4Y2hnKCZwYW5pY19jcHUsIC0xLCB0aGlzX2NwdSkg
IT0gdGhpc19jcHUpIFwNCj4gPiArCQkJcGFuaWMoZm10LCAjI19fVkFfQVJHU19fKTsJCQlcDQo+
ID4gKwl9IHdoaWxlICgwKQ0KPiA+ICsNCj4gPiAgLyoNCj4gPiAgICogT25seSB0byBiZSB1c2Vk
IGJ5IGFyY2ggaW5pdCBjb2RlLiBJZiB0aGUgdXNlciBvdmVyLXdyb3RlIHRoZSBkZWZhdWx0DQo+
ID4gICAqIENPTkZJR19QQU5JQ19USU1FT1VULCBob25vciBpdC4NCj4gPiBkaWZmIC0tZ2l0IGEv
a2VybmVsL3BhbmljLmMgYi9rZXJuZWwvcGFuaWMuYw0KPiA+IGluZGV4IDQ1NzlkYmIuLjI0ZWUy
ZWEgMTAwNjQ0DQo+ID4gLS0tIGEva2VybmVsL3BhbmljLmMNCj4gPiArKysgYi9rZXJuZWwvcGFu
aWMuYw0KPiA+IEBAIC02MSw2ICs2MSw4IEBAIHZvaWQgX193ZWFrIHBhbmljX3NtcF9zZWxmX3N0
b3Aodm9pZCkNCj4gPiAgCQljcHVfcmVsYXgoKTsNCj4gPiAgfQ0KPiA+DQo+ID4gK2F0b21pY190
IHBhbmljX2NwdSA9IEFUT01JQ19JTklUKC0xKTsNCj4gPiArDQo+ID4gIC8qKg0KPiA+ICAgKglw
YW5pYyAtIGhhbHQgdGhlIHN5c3RlbQ0KPiA+ICAgKglAZm10OiBUaGUgdGV4dCBzdHJpbmcgdG8g
cHJpbnQNCj4gPiBAQCAtNzEsMTcgKzczLDE3IEBAIHZvaWQgX193ZWFrIHBhbmljX3NtcF9zZWxm
X3N0b3Aodm9pZCkNCj4gPiAgICovDQo+ID4gIHZvaWQgcGFuaWMoY29uc3QgY2hhciAqZm10LCAu
Li4pDQo+ID4gIHsNCj4gPiAtCXN0YXRpYyBERUZJTkVfU1BJTkxPQ0socGFuaWNfbG9jayk7DQo+
ID4gIAlzdGF0aWMgY2hhciBidWZbMTAyNF07DQo+ID4gIAl2YV9saXN0IGFyZ3M7DQo+ID4gIAls
b25nIGksIGlfbmV4dCA9IDA7DQo+ID4gIAlpbnQgc3RhdGUgPSAwOw0KPiA+ICsJaW50IG9sZF9j
cHUsIHRoaXNfY3B1Ow0KPiA+DQo+ID4gIAkvKg0KPiA+ICAJICogRGlzYWJsZSBsb2NhbCBpbnRl
cnJ1cHRzLiBUaGlzIHdpbGwgcHJldmVudCBwYW5pY19zbXBfc2VsZl9zdG9wDQo+ID4gIAkgKiBm
cm9tIGRlYWRsb2NraW5nIHRoZSBmaXJzdCBjcHUgdGhhdCBpbnZva2VzIHRoZSBwYW5pYywgc2lu
Y2UNCj4gPiAgCSAqIHRoZXJlIGlzIG5vdGhpbmcgdG8gcHJldmVudCBhbiBpbnRlcnJ1cHQgaGFu
ZGxlciAodGhhdCBydW5zDQo+ID4gLQkgKiBhZnRlciB0aGUgcGFuaWNfbG9jayBpcyBhY3F1aXJl
ZCkgZnJvbSBpbnZva2luZyBwYW5pYyBhZ2Fpbi4NCj4gPiArCSAqIGFmdGVyIHNldHRpbmcgcGFu
aWNfY3B1KSBmcm9tIGludm9raW5nIHBhbmljIGFnYWluLg0KPiA+ICAJICovDQo+ID4gIAlsb2Nh
bF9pcnFfZGlzYWJsZSgpOw0KPiA+DQo+ID4gQEAgLTk0LDggKzk2LDE1IEBAIHZvaWQgcGFuaWMo
Y29uc3QgY2hhciAqZm10LCAuLi4pDQo+ID4gIAkgKiBtdWx0aXBsZSBwYXJhbGxlbCBpbnZvY2F0
aW9ucyBvZiBwYW5pYywgYWxsIG90aGVyIENQVXMgZWl0aGVyDQo+ID4gIAkgKiBzdG9wIHRoZW1z
ZWxmIG9yIHdpbGwgd2FpdCB1bnRpbCB0aGV5IGFyZSBzdG9wcGVkIGJ5IHRoZSAxc3QgQ1BVDQo+
ID4gIAkgKiB3aXRoIHNtcF9zZW5kX3N0b3AoKS4NCj4gPiArCSAqDQo+ID4gKwkgKiBgb2xkX2Nw
dSA9PSAtMScgbWVhbnMgdGhpcyBpcyB0aGUgMXN0IENQVSB3aGljaCBjb21lcyBoZXJlLCBzbw0K
PiA+ICsJICogZ28gYWhlYWQuDQo+ID4gKwkgKiBgb2xkX2NwdSA9PSB0aGlzX2NwdScgbWVhbnMg
d2UgY2FtZSBmcm9tIG5taV9wYW5pYygpIHdoaWNoIHNldHMNCj4gPiArCSAqIHBhbmljX2NwdSB0
byB0aGlzIGNwdS4gIEluIHRoaXMgY2FzZSwgdGhpcyBpcyBhbHNvIHRoZSAxc3QgQ1BVLg0KPiAN
Cj4gSSdkIHByZWZlciB0aGF0IC0xIHRvIGJlDQo+IA0KPiAjZGVmaW5lIElOVkFMSURfQ1BVX05V
TQkJLTENCj4gDQo+IG9yDQo+IA0KPiAjZGVmaW5lIFVOREVGSU5FRF9DUFVfTlVNCS0xDQo+IA0K
PiBvciBzbyBpbnN0ZWFkIG9mIGEgbmFrZWQgbnVtYmVyLg0KDQpPSywgSSdsbCB1c2UgYSBtYWNy
by4NCg0KUmVnYXJkcywNCg0KLS0NCkhpZGVoaXJvIEthd2FpDQpIaXRhY2hpLCBMdGQuIFJlc2Vh
cmNoICYgRGV2ZWxvcG1lbnQgR3JvdXANCg0KDQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276403 — Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

On Fri 20-11-15 18:36:44, Hidehiro Kawai wrote:
> If panic on NMI happens just after panic() on the same CPU, panic()
> is recursively called.  As the result, it stalls after failing to
> acquire panic_lock.
> 
> To avoid this problem, don't call panic() in NMI context if
> we've already entered panic().
> 
> V4:
> - Improve comments in io_check_error() and panic()
> 
> V3:
> - Introduce nmi_panic() macro to reduce code duplication
> - In the case of panic on NMI, don't return from NMI handlers
>   if another cpu already panicked
> 
> V2:
> - Use atomic_cmpxchg() instead of current spin_trylock() to
>   exclude concurrent accesses to the panic routines
> - Don't introduce no-lock version of panic()
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Michal Hocko <mhocko@kernel.org>

I've finally seen testing results for these patches and managed to look
at them again.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  arch/x86/kernel/nmi.c  |   16 ++++++++++++----
>  include/linux/kernel.h |   13 +++++++++++++
>  kernel/panic.c         |   15 ++++++++++++---
>  kernel/watchdog.c      |    2 +-
>  4 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 697f90d..5131714 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -231,7 +231,7 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
>  #endif
>  
>  	if (panic_on_unrecovered_nmi)
> -		panic("NMI: Not continuing");
> +		nmi_panic("NMI: Not continuing");
>  
>  	pr_emerg("Dazed and confused, but trying to continue\n");
>  
> @@ -255,8 +255,16 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
>  		 reason, smp_processor_id());
>  	show_regs(regs);
>  
> -	if (panic_on_io_nmi)
> -		panic("NMI IOCK error: Not continuing");
> +	if (panic_on_io_nmi) {
> +		nmi_panic("NMI IOCK error: Not continuing");
> +
> +		/*
> +		 * If we return from nmi_panic(), it means we have received
> +		 * NMI while processing panic().  So, simply return without
> +		 * a delay and re-enabling NMI.
> +		 */
> +		return;
> +	}
>  
>  	/* Re-enable the IOCK line, wait for a few seconds */
>  	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
> @@ -297,7 +305,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
>  
>  	pr_emerg("Do you have a strange power saving mode enabled?\n");
>  	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
> -		panic("NMI: Not continuing");
> +		nmi_panic("NMI: Not continuing");
>  
>  	pr_emerg("Dazed and confused, but trying to continue\n");
>  }
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 350dfb0..480a4fd 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -445,6 +445,19 @@ extern int sysctl_panic_on_stackoverflow;
>  
>  extern bool crash_kexec_post_notifiers;
>  
> +extern atomic_t panic_cpu;
> +
> +/*
> + * A variant of panic() called from NMI context.
> + * If we've already panicked on this cpu, return from here.
> + */
> +#define nmi_panic(fmt, ...)						\
> +	do {								\
> +		int this_cpu = raw_smp_processor_id();			\
> +		if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \
> +			panic(fmt, ##__VA_ARGS__);			\
> +	} while (0)
> +
>  /*
>   * Only to be used by arch init code. If the user over-wrote the default
>   * CONFIG_PANIC_TIMEOUT, honor it.
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 4579dbb..24ee2ea 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -61,6 +61,8 @@ void __weak panic_smp_self_stop(void)
>  		cpu_relax();
>  }
>  
> +atomic_t panic_cpu = ATOMIC_INIT(-1);
> +
>  /**
>   *	panic - halt the system
>   *	@fmt: The text string to print
> @@ -71,17 +73,17 @@ void __weak panic_smp_self_stop(void)
>   */
>  void panic(const char *fmt, ...)
>  {
> -	static DEFINE_SPINLOCK(panic_lock);
>  	static char buf[1024];
>  	va_list args;
>  	long i, i_next = 0;
>  	int state = 0;
> +	int old_cpu, this_cpu;
>  
>  	/*
>  	 * Disable local interrupts. This will prevent panic_smp_self_stop
>  	 * from deadlocking the first cpu that invokes the panic, since
>  	 * there is nothing to prevent an interrupt handler (that runs
> -	 * after the panic_lock is acquired) from invoking panic again.
> +	 * after setting panic_cpu) from invoking panic again.
>  	 */
>  	local_irq_disable();
>  
> @@ -94,8 +96,15 @@ void panic(const char *fmt, ...)
>  	 * multiple parallel invocations of panic, all other CPUs either
>  	 * stop themself or will wait until they are stopped by the 1st CPU
>  	 * with smp_send_stop().
> +	 *
> +	 * `old_cpu == -1' means this is the 1st CPU which comes here, so
> +	 * go ahead.
> +	 * `old_cpu == this_cpu' means we came from nmi_panic() which sets
> +	 * panic_cpu to this cpu.  In this case, this is also the 1st CPU.
>  	 */
> -	if (!spin_trylock(&panic_lock))
> +	this_cpu = raw_smp_processor_id();
> +	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
> +	if (old_cpu != -1 && old_cpu != this_cpu)
>  		panic_smp_self_stop();
>  
>  	console_verbose();
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 18f34cf..b9be18f 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -351,7 +351,7 @@ static void watchdog_overflow_callback(struct perf_event *event,
>  			trigger_allbutself_cpu_backtrace();
>  
>  		if (hardlockup_panic)
> -			panic("Hard LOCKUP");
> +			nmi_panic("Hard LOCKUP");
>  
>  		__this_cpu_write(hard_watchdog_warn, true);
>  		return;
> 

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276563 — Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

On Fri, Nov 20, 2015 at 06:36:44PM +0900, Hidehiro Kawai wrote:
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 350dfb0..480a4fd 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -445,6 +445,19 @@ extern int sysctl_panic_on_stackoverflow;
>  
>  extern bool crash_kexec_post_notifiers;
>  
> +extern atomic_t panic_cpu;
> +
> +/*
> + * A variant of panic() called from NMI context.
> + * If we've already panicked on this cpu, return from here.
> + */
> +#define nmi_panic(fmt, ...)						\
> +	do {								\
> +		int this_cpu = raw_smp_processor_id();			\
> +		if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \
> +			panic(fmt, ##__VA_ARGS__);			\

Hmm,

What happens if:

	CPU 0:				CPU 1:
	------				------
	nmi_panic();

					nmi_panic();
					<external nmi>
					nmi_panic();

?

cmpxchg(&panic_cpu, -1, 0) != 0

returns -1 for cpu 0, thus 0 != 0, and sets panic_cpu to 0


cmpxchg(&panic_cpu, -1, 1) != 1

returns 0, and then it too panics, but does not set panic_cpu to 1

Now you have your external NMI triggering on CPU 1

cmpxchg(&panic_cpu, -1, 1) != 1

returns 0 again, and you call panic again within the panic of CPU 1.

Is this OK?

Perhaps you want a per cpu bitmask, and do a test_and_set() on the CPU. That
would prevent any CPU from rerunning a panic() twice on any CPU.

-- Steve

> +	} while (0)
> +
>  /*
>   * Only to be used by arch init code. If the user over-wrote the default
>   * CONFIG_PANIC_TIMEOUT, honor it.
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 4579dbb..24ee2ea 100644
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276566 — Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

On Tue, 24 Nov 2015 10:05:10 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> cmpxchg(&panic_cpu, -1, 0) != 0
> 
> returns -1 for cpu 0, thus 0 != 0, and sets panic_cpu to 0

That was suppose to be "thus -1 != 0".

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276745 — Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

On Tue 24-11-15 10:05:10, Steven Rostedt wrote:
> On Fri, Nov 20, 2015 at 06:36:44PM +0900, Hidehiro Kawai wrote:
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 350dfb0..480a4fd 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -445,6 +445,19 @@ extern int sysctl_panic_on_stackoverflow;
> >  
> >  extern bool crash_kexec_post_notifiers;
> >  
> > +extern atomic_t panic_cpu;
> > +
> > +/*
> > + * A variant of panic() called from NMI context.
> > + * If we've already panicked on this cpu, return from here.
> > + */
> > +#define nmi_panic(fmt, ...)						\
> > +	do {								\
> > +		int this_cpu = raw_smp_processor_id();			\
> > +		if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \
> > +			panic(fmt, ##__VA_ARGS__);			\
> 
> Hmm,
> 
> What happens if:
> 
> 	CPU 0:				CPU 1:
> 	------				------
> 	nmi_panic();
> 
> 					nmi_panic();
> 					<external nmi>
> 					nmi_panic();

I thought that nmi_panic is called only from the nmi context. If so how
can we get a nested NMI like that?
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276756 — Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

On Tue, 24 Nov 2015 21:27:13 +0100
Michal Hocko <mhocko@kernel.org> wrote:


> > What happens if:
> > 
> > 	CPU 0:				CPU 1:
> > 	------				------
> > 	nmi_panic();
> > 
> > 					nmi_panic();
> > 					<external nmi>
> > 					nmi_panic();  
> 
> I thought that nmi_panic is called only from the nmi context. If so how
> can we get a nested NMI like that?

Nevermind. I was thinking the external NMI could nest, but I'm guessing
it cant. Anyway, the patches later on modify this code which checks for
something other than != this_cpu, which makes this issue mute even if
it could nest.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web