Fix race issues among panic, NMI and crash_kexec

From: Hidehiro Kawai [mailto:[email protected]]

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 <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Michal Hocko <[email protected]>

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

arch/x86/kernel/nmi.c +12 -4

index 697f90d..5131714 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");
 
 	pr_emerg("Dazed and confused, but trying to continue\n");
 

@@ -255,8 +255,16 @@ void unregister_nmi_handler(unsigned int type, const char *name)
 		 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 @@ void unregister_nmi_handler(unsigned int type, const char *name)
 
 	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");
 }

include/linux/kernel.h +13 -0

index 5582410..57c33da 100644
@@ -443,6 +443,19 @@ extern __scanf(2, 0)
 
 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.

kernel/panic.c +12 -3

index 04e91ff..a105e67 100644
@@ -60,6 +60,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

@@ -70,17 +72,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();
 

@@ -93,8 +95,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();

kernel/watchdog.c +0 -0

index 64ed1c3..00fbaa29 100644

Hi Hidehiro,

[auto build test results on v4.3-rc2 -- if it's inappropriate base, please ignore]

config: x86_64-allnoconfig (attached as .config) reproduce:
git checkout 0077681103150af584e5e592c0238fd010654c26

  # save the attached .config to linux build tree
  make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

kernel/panic.c: In function 'panic':

Quoted 1 line

Quoted 1 line

kernel/panic.c:140:3: error: implicit declaration of function '__crash_kexec' [-Werror=implicit-function-declaration]

      __crash_kexec(NULL);
      ^

cc1: some warnings being treated as errors

vim +/__crash_kexec +140 kernel/panic.c

134 * If we have crashed and we have a crash kernel loaded let it handle
135 * everything else.
136 * If we want to run this after calling panic_notifiers, pass
137 * the "crash_kexec_post_notifiers" option to the kernel.
138 */
139 if (!crash_kexec_post_notifiers)

Quoted 1 line

140 __crash_kexec(NULL);

141
142 /*
143 * Note smp_send_stop is the usual smp shutdown function, which

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Hi Hidehiro,

[auto build test results on v4.3-rc2 -- if it's inappropriate base, please ignore]

config: ia64-allyesconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
git checkout 0077681103150af584e5e592c0238fd010654c26

  # save the attached .config to linux build tree
  make.cross ARCH=ia64

   arch/ia64/include/uapi/asm/cmpxchg.h:56:2: warning: value computed is not used [-Wunused-value]
    ((__typeof__(*(ptr))) __xchg((unsigned long) (x), (ptr), sizeof(*(ptr))))
     ^
   arch/ia64/include/asm/atomic.h:135:30: note: in expansion of macro 'xchg'
    #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
                                 ^

Quoted 1 line

Quoted 1 line

kernel/kexec_core.c:899:3: note: in expansion of macro 'atomic_xchg'

      atomic_xchg(&panic_cpu, -1);
      ^

vim +/atomic_xchg +899 kernel/kexec_core.c

   883
   884		/*
   885		 * Only one CPU is allowed to execute the crash_kexec() code as with
   886		 * panic().  Otherwise parallel calls of panic() and crash_kexec()
   887		 * may stop each other.  To exclude them, we use panic_cpu here too.
   888		 */
   889		this_cpu = raw_smp_processor_id();
   890		old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
   891		if (old_cpu == -1) {
   892			/* This is the 1st CPU which comes here, so go ahead. */
   893			__crash_kexec(regs);
   894
   895			/*
   896			 * Reset panic_cpu to allow another panic()/crash_kexec()
   897			 * call.
   898			 */

Quoted 1 line

899 atomic_xchg(&panic_cpu, -1);

900 }
901 }
902
903 size_t crash_get_memory_size(void)

   904	{
   905		size_t size = 0;
   906
   907		mutex_lock(&kexec_mutex);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Peter saids -tip tree doesn't have panic_on_unrecovered_nmi in the previoius discussion, but it still exists. So, I didn't change anything about panic_on_unrecovered_nmi.

Quoted 5 lines

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

@@ -231,7 +231,7 @@ void unregister_nmi_handler(unsigned int type, const char *name)
 #endif
 

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

Quoted 6 lines

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

 	if (panic_on_unrecovered_nmi)
-		panic("NMI: Not continuing");
+		nmi_panic("NMI: Not continuing");
 
 	pr_emerg("Dazed and confused, but trying to continue\n");
 

arch/x86/kernel/reboot.c +1 -0

@@ -718,6 +718,7 @@ void machine_crash_shutdown(struct pt_regs *regs)
 static nmi_shootdown_cb shootdown_callback;
 
 static atomic_t waiting_for_crash_ipi;
+static int crash_ipi_done;
 
 static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
 {

Quoted 3 lines

Quoted 1 line

Quoted 1 line

kernel/kexec_core.c:899:3: note: in expansion of macro 'atomic_xchg'

      atomic_xchg(&panic_cpu, -1);
      ^

I changed to use atomic_xchg() instead of atomic_set() in V3 because atomic_set() doesn't mean memory barrier. However, I thought again and there is no need of barrier; there is no problem if a competitor sees old value of panic_cpu or new one. So, atomic_set() is sufficient and using it will remove this warning.

I will resend the fixed version later.

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

Quoted 14 lines

Peter saids -tip tree doesn't have panic_on_unrecovered_nmi in the previoius discussion, but it still exists. So, I didn't change anything about panic_on_unrecovered_nmi.

Quoted 9 lines

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

@@ -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");
 

Quoted 2 lines

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

I was looking at unregister_nmi_handler() because that's the function the diff points to. That still doesn't have panic_on_unrecovered_nmi.

It looks like your diff tool is 'broken' and generates nonsense function data.

Quoted 11 lines

arch/x86/kernel/reboot.c +1 -0

@@ -718,6 +718,7 @@ void machine_crash_shutdown(struct pt_regs *regs)
 static nmi_shootdown_cb shootdown_callback;
 
 static atomic_t waiting_for_crash_ipi;
+static int crash_ipi_done;
 
 static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
 {

arch/x86/kernel/reboot.c void nmi_shootdown_cpus(nmi_shootdown_cb callback) · line 783

Quoted 4 lines

arch/x86/kernel/reboot.c +1 -0

@@ -779,6 +780,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	wmb();
 
 	smp_send_nmi_allbutself();
+	crash_ipi_done = 1; /* Kick cpus looping in nmi context */

I would suggest using WRITE_ONCE() for that, because without the volatile the compiler need not actually emit the store until after the whole waiting thing _IF_ it can inline the whole thing.

Currently udelay() will end up being a function call and will therefore force the store to be emitted, but I'd rather not rely on that.

arch/x86/kernel/reboot.c void nmi_shootdown_cpus(nmi_shootdown_cb callback) · line 786

Quoted 3 lines

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

 	msecs = 1000; /* Wait at most a second for the other cpus to stop */
 	while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

kernel/kexec_core.c:899:3: note: in expansion of macro 'atomic_xchg'

      atomic_xchg(&panic_cpu, -1);
      ^

I changed to use atomic_xchg() instead of atomic_set() in V3 because atomic_set() doesn't mean memory barrier. However, I thought again and there is no need of barrier; there is no problem if a competitor sees old value of panic_cpu or new one. So, atomic_set() is sufficient and using it will remove this warning.

Quoted 1 line

I will resend the fixed version later.

So if you rely on the memory barrier; you should have also put a comment on explaining the ordering requirements.

The following operations are potential problems as they do _not_ imply memory barriers, but might be used for implementing such things as RELEASE-class operations:

        atomic_set();

...

Quoted 4 lines

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

So that they can get really stuck when the crash kernel crashes, right? ;-)

Quoted 8 lines · Hidehiro Kawai

Quoted 4 lines

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

So that they can get really stuck when the crash kernel crashes, right? ;-)

No, it is different from my intention.

`mistakenly' in the above means; they issue NMI due to a misconception that the monitored host is stuck in the 1st kernel while it is actually taking a crash dump in the 2nd kernel. To avoid this kind of accident, there is a tool such as fence_kdump which notifies "I'm taking a crash dump, so don't send NMI" to the HA clustering software. However, there is a time window between kernel panic and the notification.

"noextnmi" allows users to avoid this kind of accident all the time of 2nd kernel.

Quoted 18 lines

Quoted 8 lines · Hidehiro Kawai

Quoted 4 lines

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

So that they can get really stuck when the crash kernel crashes, right? ;-)

No, it is different from my intention.

`mistakenly' in the above means; they issue NMI due to a misconception that the monitored host is stuck in the 1st kernel while it is actually taking a crash dump in the 2nd kernel. To avoid this kind of accident, there is a tool such as fence_kdump which notifies "I'm taking a crash dump, so don't send NMI" to the HA clustering software. However, there is a time window between kernel panic and the notification.

Quoted 2 lines

"noextnmi" allows users to avoid this kind of accident all the time of 2nd kernel.

Yes yes, I understand. But if the crash kernel also gets stuck they have no means of recovery, right? (other than power cycling the hardware)

Just playing devils advocate here, I don't actually object to the patch.

I suppose that a sever which uses this feature will equip a BMC and BMC mandatorily supports hard reset command for the server. If the HA clustering software detects no response from the server after relatively long timeout, it might want to insert hard reset to the server by IPMI over LAN.

Quoted 5 lines

I suppose that a sever which uses this feature will equip a BMC and BMC mandatorily supports hard reset command for the server. If the HA clustering software detects no response from the server after relatively long timeout, it might want to insert hard reset to the server by IPMI over LAN.

So why doesn't the capture kernel *automatically* ignore external NMIs, without a cmdline option?

Before it starts capturing, it says: "I'm starting capturing and am ignoring external NMIs from now on."

But how do we check if the starting kernel is a dump capture kernel?

I think using cmdline option is the simplest way.

Quoted 1 line

But how do we check if the starting kernel is a dump capture kernel?

How does that first kernel pass info to the capture kernel?

Quoted 1 line

I think using cmdline option is the simplest way.

More often than not, simplest != correct.

What happens if I pass this option to the first kernel? All of a sudden my *first* kernel doesn't get external NMIs.

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

Quoted 1 line

But how do we check if the starting kernel is a dump capture kernel?

How does that first kernel pass info to the capture kernel?

As I described in the previous mail,

Yes, your first kernel doesn't get external NMIs, but basically you don't have to set "noextnmi" option to the first kernel.

Quoted 5 lines

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

Quoted 1 line

But how do we check if the starting kernel is a dump capture kernel?

How does that first kernel pass info to the capture kernel?

Quoted 1 line

As I described in the previous mail,

I meant: "How does the first kernel pass info to the capture kernel by *not* using the kernel command line"?

The kernel command line is not the channel to pass data to the kdump kernel.

Quoted 2 lines

Yes, your first kernel doesn't get external NMIs, but basically you don't have to set "noextnmi" option to the first kernel.

So it doesn't belong there as a kernel command line parameter in the first place.

IOW, you need a different method to pass data to the second kernel. Be it an ELF header, be it a shared page, whatever.

That's different from my point of view. I'm not going to pass some data from the first kernel to the second kernel. I'm just going to provide a configurable option for the second kernel to users.

I think we should use the ELF header only if the passed information is saved to a crash dump.

Quoted 3 lines

That's different from my point of view. I'm not going to pass some data from the first kernel to the second kernel. I'm just going to provide a configurable option for the second kernel to users.

Dude, WTF?! You're adding a kernel command line which is supposed to be used *only* by the kdump kernel. But nooo, it is there in the open and visible to people. And anyone can type it in during boot. AND THAT SHOULDN'T BE POSSIBLE IN THE FIRST PLACE!

This information is strictly for the kdump kernel - it shouldn't be a generic command line option. How hard it is to understand that simple fact?!

elfcorehdr=[size[KMG]@]offset[KMG] [IA64,PPC,SH,X86,S390]
Specifies physical address of start of kernel core image elf header and optionally the size. Generally kexec loader will pass this option to capture kernel. See Documentation/kdump/kdump.txt for details.

<snip obvious use case>

Quoted 2 lines

I think we should use the ELF header only if the passed information is saved to a crash dump.

So what?! ELF header will contain the additional bit of information that the second kernel wasn't reacting to NMIs. But that's fine, that *is* the desired behavior anyway.

All I'm saying is, this is a strict kdump kernel "command", so to speak, and it doesn't belong with the generic kernel command line parameters.

So, the problem for you is that "noextnmi" option is visible and effective in the first kernel, isn't it?

Quoted 2 lines

So, the problem for you is that "noextnmi" option is visible and effective in the first kernel, isn't it?

No, such an option shouldn't exist at all. You should be passing information *in* *a* *different* *manner* to the kdump kernel - not with a kernel command line option.

I get the feeling I'm starting to sound like a broken record on this mail thread... :-(

One other thing we could probably try to do is use boot_params which is, IIUC, passed to the second kernel. So we can add another bit to boot_params.hdr.loadflags or so and use that. Or something similar.

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

That's different from my point of view. I'm not going to pass some data from the first kernel to the second kernel. I'm just going to provide a configurable option for the second kernel to users.

Dude, WTF?! You're adding a kernel command line which is supposed to be used *only* by the kdump kernel. But nooo, it is there in the open and visible to people. And anyone can type it in during boot. AND THAT SHOULDN'T BE POSSIBLE IN THE FIRST PLACE!

This information is strictly for the kdump kernel - it shouldn't be a generic command line option. How hard it is to understand that simple fact?!

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

Currently, only x86 supports this option.

Signed-off-by Hidehiro Kawai <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Jonathan Corbet <[email protected]>

---
 Documentation/kernel-parameters.txt |    4 ++++
 arch/x86/kernel/apic/apic.c         |   17 ++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

Documentation/kernel-parameters.txt +0 -0

index 22a4b68..8bcaccd 100644

Quoted 7 lines

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

Currently, only x86 supports this option.

You might add that is can be used for debugging purposes as well. External NMIs can be their own source of trouble. :)

Quoted 18 lines

Signed-off-by Hidehiro Kawai <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Jonathan Corbet <[email protected]>

---
 Documentation/kernel-parameters.txt |    4 ++++
 arch/x86/kernel/apic/apic.c         |   17 ++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

Documentation/kernel-parameters.txt +0 -0

index 22a4b68..8bcaccd 100644
@@ -2379,6 +2379,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			noexec=on: enable non-executable mappings (default)
 			noexec=off: disable non-executable mappings
 

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 2384

Quoted 3 lines

Documentation/kernel-parameters.txt +3 -0

+	noextnmi	[X86]
+			Mask external NMI.  This option is useful for a
+			dump capture kernel to be shot down by NMI.

That should read: "...not to be shot down", right?

Other than that.

Acked-by Thomas Gleixner <[email protected]>

Quoted 11 lines · Hidehiro Kawai

Quoted 7 lines

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

Currently, only x86 supports this option.

You might add that is can be used for debugging purposes as well. External NMIs can be their own source of trouble. :)

Thanks for your comments! I'll do that.

By the way, I have a pending patch which expands this option like this:

	apic_extnmi={ bsp | all | none }

If apic_extnmi=all is specified, external NMIs are broadcast to all CPUs. This raises the successful rate of kernel panic in the case where an external NMI to CPU 0 is swallowed by other NMI handlers or blocked due to hang-up in NMI context. The patch works without any problems, but I'm going to drop the feature if it will cause long discussion. I'd like to settle this patch set down once. At least, I'm going to change this option to apic_extnmi={bsp|none} style for the future expansion.

How do you think about this?

Borislav,

Quoted 5 lines · Borislav Petkov

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

That's different from my point of view. I'm not going to pass some data from the first kernel to the second kernel. I'm just going to provide a configurable option for the second kernel to users.

Quoted 8 lines

Dude, WTF?! You're adding a kernel command line which is supposed to be used *only* by the kdump kernel. But nooo, it is there in the open and visible to people. And anyone can type it in during boot. AND THAT SHOULDN'T BE POSSIBLE IN THE FIRST PLACE!

This information is strictly for the kdump kernel - it shouldn't be a generic command line option. How hard it is to understand that simple fact?!

Calm down!

Disabling that NMI in the first kernel is not going to make the world explode. We have enough command line options a user can type in, which are way worse than that one. If some "expert" types nonsense on the first kernel command line, then it's none of our problems, really.

If Kawai would have marked that option as a debug feature, this discussion would have probably never happened.

Aside of that, the best way to hand in options for the kdump kernel is the command line. We have an existing interface for that.

Let's move on. Nothing to see here.

Quoted 25 lines

Borislav,

Quoted 5 lines · Borislav Petkov

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

That's different from my point of view. I'm not going to pass some data from the first kernel to the second kernel. I'm just going to provide a configurable option for the second kernel to users.

Quoted 8 lines

Dude, WTF?! You're adding a kernel command line which is supposed to be used *only* by the kdump kernel. But nooo, it is there in the open and visible to people. And anyone can type it in during boot. AND THAT SHOULDN'T BE POSSIBLE IN THE FIRST PLACE!

This information is strictly for the kdump kernel - it shouldn't be a generic command line option. How hard it is to understand that simple fact?!

Calm down!

Disabling that NMI in the first kernel is not going to make the world explode. We have enough command line options a user can type in, which are way worse than that one. If some "expert" types nonsense on the first kernel command line, then it's none of our problems, really.

Quoted 7 lines

If Kawai would have marked that option as a debug feature, this discussion would have probably never happened.

Aside of that, the best way to hand in options for the kdump kernel is the command line. We have an existing interface for that.

Let's move on. Nothing to see here.

So Boris kind of has a point: there are numerous problems with boot options as kexec parameter interface:

- boot option strings are not a well defined programmatic interface:
- failures are not obvious (they are often ignored)
- inserting/setting parameters is awkward as well.

- boot options are not an ABI, so when options have dual use with kexec it's easy to break things inadvertently: without that failure being apparent other than reintroducing obscure kexec failure modes again.

- in the booted up kexec kernel it's not really obvious which options got set by kexec and which got set by the user - as there's no separation of namespaces.

- likewise, if the user specifies an option in conflict with a kexec requirement it's not really obvious what's happening: the user setting should probably dominate - I'm not sure that's the case with the current kexec code.

Boot options are basically a user interface.

On the other hand the hack of (ab-)using boot parameters as kexec parameter passing is an existing, many years old practice and we cannot just stop it without offering an alternative (and better!) interface first.

We could improve things by either adding a separate kexec-only parameter passing facility (like programmatic boot parameters are) - or by creating some sort of boot parameter alias that clearly identifies kexec parameters.

So for example when introducing 'noextnmi' we'd also add a facility to add a 'kexec_noextnmi' alias - which clearly identifies this boot parameter as a kexec related one.

Every 'kexec' inserted parameter would be prefixed by kexec_ - and then the separation of namespaces (and the prioritization of user vs. kexec requirements) becomes well defined as well..

We should also probably print a warning if a kexec_* parameter is passed in that has no matching handler in the kexec()-ed kernel.

I do concur that this patch is probably OK given existing practices.

Quoted 14 lines

By the way, I have a pending patch which expands this option like this:

	apic_extnmi={ bsp | all | none }

If apic_extnmi=all is specified, external NMIs are broadcast to all CPUs. This raises the successful rate of kernel panic in the case where an external NMI to CPU 0 is swallowed by other NMI handlers or blocked due to hang-up in NMI context. The patch works without any problems, but I'm going to drop the feature if it will cause long discussion. I'd like to settle this patch set down once. At least, I'm going to change this option to apic_extnmi={bsp|none} style for the future expansion.

Quoted 1 line

How do you think about this?

Do it right away with all three variants. They make a lot of sense to me.

This patch introduces new boot option "noextnmi" which disables external NMI. This option is useful for the dump capture kernel so that an HA application or administrator wouldn't mistakenly shoot down the kernel by NMI.

Currently, only x86 supports this option.

Signed-off-by Hidehiro Kawai <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Jonathan Corbet <[email protected]>

---
 Documentation/kernel-parameters.txt |    4 ++++
 arch/x86/kernel/apic/apic.c         |   17 ++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

Documentation/kernel-parameters.txt +4 -0

index 22a4b68..8bcaccd 100644
@@ -2379,6 +2379,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			noexec=on: enable non-executable mappings (default)
 			noexec=off: disable non-executable mappings
 
+	noextnmi	[X86]
+			Mask external NMI.  This option is useful for a
+			dump capture kernel to be shot down by NMI.
+
 	nosmap		[X86]
 			Disable SMAP (Supervisor Mode Access Prevention)
 			even if it is supported by processor.

arch/x86/kernel/apic/apic.c +16 -1

index 24e94ce..fd47128 100644
@@ -82,6 +82,12 @@
 static unsigned int disabled_cpu_apicid __read_mostly = BAD_APICID;
 
 /*
+ * Don't enable external NMI via LINT1 on BSP.  This is useful for
+ * the dump capture kernel.
+ */
+static bool apic_noextnmi;
+
+/*
  * Map cpu index to physical APIC ID
  */
 DEFINE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid, BAD_APICID);

@@ -1161,6 +1167,8 @@ void __init init_bsp_APIC(void)
 	value = APIC_DM_NMI;
 	if (!lapic_is_integrated())		/* 82489DX */
 		value |= APIC_LVT_LEVEL_TRIGGER;
+	if (apic_noextnmi)
+		value |= APIC_LVT_MASKED;
 	apic_write(APIC_LVT1, value);
 }
 

@@ -1380,7 +1388,7 @@ void setup_local_APIC(void)
 	/*
 	 * only the BP should see the LINT1 NMI signal, obviously.
 	 */
-	if (!cpu)
+	if (!cpu && !apic_noextnmi)
 		value = APIC_DM_NMI;
 	else
 		value = APIC_DM_NMI | APIC_LVT_MASKED;

@@ -2548,3 +2556,10 @@ static int __init apic_set_disabled_cpu_apicid(char *arg)
 	return 0;
 }
 early_param("disable_cpu_apicid", apic_set_disabled_cpu_apicid);
+
+static int __init apic_set_noextnmi(char *arg)
+{
+	apic_noextnmi = true;
+	return 0;
+}
+early_param("noextnmi", apic_set_noextnmi);

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

I just have a look at this thread. I am wondering why we don't use existing is_kdump_kernel() directly to disable external NMI if it's in kdump kernel. Then no need to introduce another boot option "noextnmi" which is used only for kdump kernel.

We souldn't enable this feature silently. Some users wouldn't like to enable this feature. For example, a user enables a watchdog timer which raises an external NMI when the counter is not reset for a specific duration. Then, the second kernel hangs up while saving crash dump, and NMI is delivered to the CPU. The kernel gets panic due to the NMI, prints some information to the display and serial console, and then automatically reboot. In this case, users don't want to block external NMIs.

Hi,

Quoted 4 lines

I just have a look at this thread. I am wondering why we don't use existing is_kdump_kernel() directly to disable external NMI if it's in kdump kernel. Then no need to introduce another boot option "noextnmi" which is used only for kdump kernel.

As I stated in another mail, there is a case where we don't want to mask external NMIs in the second kernel. So, we need some configurable way. Please see the following quotation.

Quoted 9 lines

We souldn't enable this feature silently. Some users wouldn't like to enable this feature. For example, a user enables a watchdog timer which raises an external NMI when the counter is not reset for a specific duration. Then, the second kernel hangs up while saving crash dump, and NMI is delivered to the CPU. The kernel gets panic due to the NMI, prints some information to the display and serial console, and then automatically reboot. In this case, users don't want to block external NMIs.

Regards,

Hidehiro Kawai
Hitachi, Ltd. Research & Development Group