Fix race issues among panic, NMI and crash_kexec

From: Peter Zijlstra [mailto:[email protected]]

Quoted 11 lines · Hidehiro Kawai

arch/x86/kernel/nmi.c +1 -1

index d05bd2e..dcd4038 100644
@@ -231,7 +231,7 @@ void unregister_nmi_handler(unsigned int type, const char *name)
 #endif
 
 	if (panic_on_unrecovered_nmi)
-		panic("NMI: Not continuing");
+		nmi_panic("NMI: Not continuing");
 

arch/x86/kernel/nmi.c void unregister_nmi_handler(unsigned int type, const char *name) · line 237

Quoted 2 lines

arch/x86/kernel/nmi.c +0 -0

 	pr_emerg("Dazed and confused, but trying to continue\n");
 

What tree is this again.. my tree (-tip) doesn't have panic_on_unrecovered_nmi nonsense.

When cpu-A panics on NMI just after cpu-B has panicked, cpu-A loops infinitely in NMI context. Especially for x86, cpu-B issues NMI IPI to other cpus to save their register states and do some cleanups if kdump is enabled, but cpu-A can't handle the NMI and fails to save register states.

To solve thie issue, we wait for the timing of the NMI IPI, then call the NMI handler which saves register states.

From: Peter Zijlstra [mailto:[email protected]]

Quoted 6 lines · Hidehiro Kawai

When cpu-A panics on NMI just after cpu-B has panicked, cpu-A loops infinitely in NMI context. Especially for x86, cpu-B issues NMI IPI to other cpus to save their register states and do some cleanups if kdump is enabled, but cpu-A can't handle the NMI and fails to save register states.

Quoted 2 lines

To solve thie issue, we wait for the timing of the NMI IPI, then call the NMI handler which saves register states.

Sorry, I don't follow, what?

From: Peter Zijlstra [mailto:[email protected]]

kernel/kexec.c void __weak crash_unmap_reserved_pages(void) · line 1485

Quoted 13 lines · Hidehiro Kawai

void crash_kexec(struct pt_regs *regs)
{

kernel/kexec.c +11 -0

+	int old_cpu, this_cpu;
+
+	/*
+	 * `old_cpu == -1' means we are the first comer and crash_kexec()
+	 * was called without entering panic().
+	 * `old_cpu == this_cpu' means crash_kexec() was called from panic().
+	 */
+	this_cpu = raw_smp_processor_id();
+	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
+	if (old_cpu != -1 && old_cpu != this_cpu)
+		return;

This allows recursive calling of crash_kexec(), the Changelog did not mention that. Is this really required?

Quoted 19 lines

From: Peter Zijlstra [mailto:[email protected]]

kernel/kexec.c void __weak crash_unmap_reserved_pages(void) · line 1485

Quoted 13 lines · Hidehiro Kawai

void crash_kexec(struct pt_regs *regs)
{

kernel/kexec.c +11 -0

+	int old_cpu, this_cpu;
+
+	/*
+	 * `old_cpu == -1' means we are the first comer and crash_kexec()
+	 * was called without entering panic().
+	 * `old_cpu == this_cpu' means crash_kexec() was called from panic().
+	 */
+	this_cpu = raw_smp_processor_id();
+	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
+	if (old_cpu != -1 && old_cpu != this_cpu)
+		return;

This allows recursive calling of crash_kexec(), the Changelog did not mention that. Is this really required?

What part are you arguing? Recursive call of crash_kexec() doesn't happen. In the first place, one of the purpose of this patch is to prevent a recursive call of crash_kexec() in the following case as I stated in the description:

CPU 0:
oops_end()
crash_kexec()
mutex_trylock() // acquired
<NMI>
io_check_error()
panic()
crash_kexec()
mutex_trylock() // failed to acquire
infinite loop

Also, the logic doesn't change form V1 (although the implementation changed), so I didn't add changelogs any more.

From: Peter Zijlstra [mailto:[email protected]]

Quoted 25 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 19 lines

From: Peter Zijlstra [mailto:[email protected]]

kernel/kexec.c void __weak crash_unmap_reserved_pages(void) · line 1485

Quoted 13 lines · Hidehiro Kawai

void crash_kexec(struct pt_regs *regs)
{

kernel/kexec.c +11 -0

+	int old_cpu, this_cpu;
+
+	/*
+	 * `old_cpu == -1' means we are the first comer and crash_kexec()
+	 * was called without entering panic().
+	 * `old_cpu == this_cpu' means crash_kexec() was called from panic().
+	 */
+	this_cpu = raw_smp_processor_id();
+	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
+	if (old_cpu != -1 && old_cpu != this_cpu)
+		return;

This allows recursive calling of crash_kexec(), the Changelog did not mention that. Is this really required?

What part are you arguing? Recursive call of crash_kexec() doesn't happen. In the first place, one of the purpose of this patch is to prevent a recursive call of crash_kexec() in the following case as I stated in the description:

Quoted 11 lines

CPU 0:
oops_end()
crash_kexec()
mutex_trylock() // acquired
<NMI>
io_check_error()
panic()
crash_kexec()
mutex_trylock() // failed to acquire
infinite loop

Yes, but what to we want to do there? It seems to me that is wrong, we do not want to let a recursive crash_kexec() proceed.

Whereas the condition you created explicitly allows this recursion by virtue of the 'old_cpu != this_cpu' check.

From: [email protected] [mailto:[email protected]] On Behalf Of 河合英宏 / KAWAI，

Hi,

Quoted 40 lines

From: Peter Zijlstra [mailto:[email protected]]

Quoted 25 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 19 lines

From: Peter Zijlstra [mailto:[email protected]]

kernel/kexec.c void __weak crash_unmap_reserved_pages(void) · line 1485

Quoted 13 lines · Hidehiro Kawai

void crash_kexec(struct pt_regs *regs)
{

kernel/kexec.c +11 -0

+	int old_cpu, this_cpu;
+
+	/*
+	 * `old_cpu == -1' means we are the first comer and crash_kexec()
+	 * was called without entering panic().
+	 * `old_cpu == this_cpu' means crash_kexec() was called from panic().
+	 */
+	this_cpu = raw_smp_processor_id();
+	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
+	if (old_cpu != -1 && old_cpu != this_cpu)
+		return;

This allows recursive calling of crash_kexec(), the Changelog did not mention that. Is this really required?

What part are you arguing? Recursive call of crash_kexec() doesn't happen. In the first place, one of the purpose of this patch is to prevent a recursive call of crash_kexec() in the following case as I stated in the description:

Quoted 11 lines

CPU 0:
oops_end()
crash_kexec()
mutex_trylock() // acquired
<NMI>
io_check_error()
panic()
crash_kexec()
mutex_trylock() // failed to acquire
infinite loop

Quoted 5 lines

Yes, but what to we want to do there? It seems to me that is wrong, we do not want to let a recursive crash_kexec() proceed.

Whereas the condition you created explicitly allows this recursion by virtue of the 'old_cpu != this_cpu' check.

I understand your question. I don't intend to permit the recursive call of crash_kexec() as for 'old_cpu != this_cpu' check. That is needed for the case of panic() --> crash_kexec(). Since panic_cpu has already been set to this_cpu in panic() (please see PATCH 1/4), no one can run crash_kexec() without 'old_cpu != this_cpu' check.

If you don't like this check, I would also be able to handle this case like below:

crash_kexec()

{
	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
	if (old_cpu != -1)
		return;

	__crash_kexec();
}

panic()

{
	atomic_cmpxchg(&panic_cpu, -1, this_cpu);
	__crash_kexec();

...

Quoted 18 lines

I understand your question. I don't intend to permit the recursive call of crash_kexec() as for 'old_cpu != this_cpu' check. That is needed for the case of panic() --> crash_kexec(). Since panic_cpu has already been set to this_cpu in panic() (please see PATCH 1/4), no one can run crash_kexec() without 'old_cpu != this_cpu' check.

If you don't like this check, I would also be able to handle this case like below:

crash_kexec()

{
	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
	if (old_cpu != -1)
		return;

	__crash_kexec();
}

Quoted 6 lines

panic()

{
	atomic_cmpxchg(&panic_cpu, -1, this_cpu);
	__crash_kexec();

...

Is that OK?

From: Peter Zijlstra [mailto:[email protected]]

Quoted 25 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 24 lines

I understand your question. I don't intend to permit the recursive call of crash_kexec() as for 'old_cpu != this_cpu' check. That is needed for the case of panic() --> crash_kexec(). Since panic_cpu has already been set to this_cpu in panic() (please see PATCH 1/4), no one can run crash_kexec() without 'old_cpu != this_cpu' check.

If you don't like this check, I would also be able to handle this case like below:

crash_kexec()

{
	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
	if (old_cpu != -1)
		return;

	__crash_kexec();
}

panic()

{
	atomic_cmpxchg(&panic_cpu, -1, this_cpu);
	__crash_kexec();

...

Quoted 1 line

Is that OK?

I suppose so, but I think me getting confused means comments can be added/improved.

When an HA cluster software or administrator detects non-response of a host, they issue an NMI to the host to completely stop current works and take a crash dump. If the kernel has already panicked or is capturing a crash dump at that time, further NMI can cause a crash dump failure.

Also, crash_kexec() called from oops context and panic() can cause race conditions.

To solve these issue, this patch set does following things:

- Don't panic on NMI if the kernel has already panicked
- Extend exclusion control currently done by panic_lock to crash_kexec
- Introduce "noextnmi" boot option which masks external NMI at the boot time (supported only for x86)

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 panic() and crash_kexec()
- Don't introduce no-lock version of panic() and crash_kexec()

---

Hidehiro Kawai (4):
panic/x86: Fix re-entrance problem due to panic on NMI
panic/x86: Allow cpus to save registers even if they are looping in NMI context
kexec: Fix race between panic() and crash_kexec() called directly x86/apic: Introduce noextnmi boot option

 Documentation/kernel-parameters.txt |    4 ++++
 arch/x86/kernel/apic/apic.c         |   17 ++++++++++++++++-
 arch/x86/kernel/nmi.c               |   15 +++++++++++----
 arch/x86/kernel/reboot.c            |   11 +++++++++++
 include/linux/kernel.h              |   21 +++++++++++++++++++++
 kernel/kexec.c                      |   20 ++++++++++++++++++++
 kernel/panic.c                      |   23 ++++++++++++++++++++---
 kernel/watchdog.c                   |    5 +++--
 8 files changed, 106 insertions(+), 10 deletions(-)

Signature · 2 lines

Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

From: Peter Zijlstra [mailto:[email protected]]
User-Agent: StGit/0.16

Fwiw, stgit is broken wrt sending email, all your emails have the exact same timestamp, which means that the emails will be ordered on received timestamp when threaded and generate the below mess: