Message-ID: <20160705113325.5864.74840.stgit@softrs>
Date:	Tue, 05 Jul 2016 20:33:25 +0900
From:	Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Dave Young <dyoung@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Baoquan He <bhe@...hat.com>
Cc:	Michal Hocko <mhocko@...e.com>, Toshi Kani <toshi.kani@....com>,
	"Peter Zijlstra \(Intel\)" <peterz@...radead.org>,
	Minfei Huang <mnfhuang@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Daniel Walker <dwalker@...o99.com>,
	Ingo Molnar <mingo@...nel.org>,
	Takao Indoh <indou.takao@...fujitsu.com>, x86@...nel.org,
	"Lee, Chun-Yi" <joeyli.kernel@...il.com>,
	Borislav Petkov <bp@...e.de>, Vivek Goyal <vgoyal@...hat.com>,
	Petr Mladek <pmladek@...e.com>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, kexec@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	HATAYAMA Daisuke <d.hatayama@...fujitsu.com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Tejun Heo <tj@...nel.org>,
	Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump
 friendly version

This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).

If crash_kexec_post_notifiers boot option is specified, other CPUs
are stopped by smp_send_stop() instead of machine_crash_shutdown()
in crash_kexec() path.  This behavior change leads two problems.

 Problem 1:
 octeon_generic_shutdown() for MIPS OCTEON assumes that other CPUs are
 still online and try to stop their watchdog timer.  If
 smp_send_stop() is called before octeon_generic_shutdown(), stopping
 watchdog timer will fail because other CPUs have been offlined by
 smp_send_stop().

   panic()
     if crash_kexec_post_notifiers == 1
       smp_send_stop()
       atomic_notifier_call_chain()
       kmsg_dump()
     crash_kexec()
       machine_crash_shutdown()
         octeon_generic_shutdown() // shutdown watchdog for ONLINE CPUs

 Problem 2:
 Most of architectures stop other CPUs in machine_crash_shutdown()
 path, and they also do something needed for kdump.  For example,
 they save registers, disable virtualization extensions, and so on.
 However, if smp_send_stop() stops other CPUs before
 machine_crash_shutdown(), we miss those operations.

How do we fix these problems?  In the first place, we should stop
other CPUs as soon as possible when panic() was called, otherwise
other CPUs may wipe out a clue to the cause of the failure.  So, we
replace smp_send_stop() with more suitable one for kdump.

This patch solves Problem 2 by replacing smp_send_stop() in panic()
with panic_smp_send_stop().  This is a weak function which calls
smp_send_stop(), and architecture dependent code may override this
with appropriate one.  This patch only provides x86-specific version.

Changes in V3:
- Revise comments, description, and symbol names

Changes in V2:
- Replace smp_send_stop() call with crash_kexec version which
  saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this moment

Reported-by: Daniel Walker <dwalker@...o99.com>
Fixes: f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)
Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
Cc: Dave Young <dyoung@...hat.com>
Cc: Baoquan He <bhe@...hat.com>
Cc: Vivek Goyal <vgoyal@...hat.com>
Cc: Eric Biederman <ebiederm@...ssion.com>
Cc: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...hat.com>
Cc: "H. Peter Anvin" <hpa@...or.com>
Cc: Borislav Petkov <bp@...e.de>
Cc: Toshi Kani <toshi.kani@....com>
Cc: "Peter Zijlstra (Intel)" <peterz@...radead.org>
Cc: Takao Indoh <indou.takao@...fujitsu.com>
Cc: "Lee, Chun-Yi" <joeyli.kernel@...il.com>
Cc: Minfei Huang <mnfhuang@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Michal Hocko <mhocko@...e.com>
Cc: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: Petr Mladek <pmladek@...e.com>
Cc: Tejun Heo <tj@...nel.org>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>
---
 arch/x86/kernel/crash.c |   14 ++++++++++----
 kernel/panic.c          |   43 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9ef978d..3305433 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -133,15 +133,21 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
 	disable_local_APIC();
 }
 
-static void kdump_nmi_shootdown_cpus(void)
+/* Override the weak function in kernel/panic.c */
+void panic_smp_send_stop(void)
 {
-	nmi_shootdown_cpus(kdump_nmi_callback);
+	static int cpus_stopped;
+
+	if (cpus_stopped)
+		return;
 
+	nmi_shootdown_cpus(kdump_nmi_callback);
 	disable_local_APIC();
+	cpus_stopped = 1;
 }
 
 #else
-static void kdump_nmi_shootdown_cpus(void)
+void panic_smp_send_stop(void)
 {
 	/* There are no cpus to shootdown */
 }
@@ -160,7 +166,7 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
 	/* The kernel is broken so disable interrupts */
 	local_irq_disable();
 
-	kdump_nmi_shootdown_cpus();
+	panic_smp_send_stop();
 
 	/*
 	 * VMCLEAR VMCSs loaded on this cpu if needed.
diff --git a/kernel/panic.c b/kernel/panic.c
index 8aa7449..da8062d2 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -71,6 +71,32 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 	panic_smp_self_stop();
 }
 
+/*
+ * Stop other CPUs in panic.  Architecture dependent code may override this
+ * with more suitable version.  For example, if the architecture supports
+ * crash dump, it should save registers of each stopped CPU and disable
+ * per-CPU features such as virtualization extensions.
+ */
+void __weak panic_smp_send_stop(void)
+{
+	static int cpus_stopped;
+
+	/*
+	 * This function can be called twice in panic path, but obviously
+	 * we execute this only once.
+	 */
+	if (cpus_stopped)
+		return;
+
+	/*
+	 * Note smp_send_stop is the usual smp shutdown function, which
+	 * unfortunately means it may not be hardened to work in a panic
+	 * situation.
+	 */
+	smp_send_stop();
+	cpus_stopped = 1;
+}
+
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
 
 /*
@@ -125,7 +151,7 @@ void panic(const char *fmt, ...)
 	 * Only one CPU is allowed to execute the panic code from here. For
 	 * 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().
+	 * with panic_smp_send_stop().
 	 *
 	 * `old_cpu == PANIC_CPU_INVALID' means this is the 1st CPU which
 	 * comes here, so go ahead.
@@ -165,12 +191,7 @@ void panic(const char *fmt, ...)
 		__crash_kexec(NULL);
 	}
 
-	/*
-	 * Note smp_send_stop is the usual smp shutdown function, which
-	 * unfortunately means it may not be hardened to work in a panic
-	 * situation.
-	 */
-	smp_send_stop();
+	panic_smp_send_stop();
 
 	/*
 	 * Run any panic handlers, including those that might need to
@@ -198,10 +219,10 @@ void panic(const char *fmt, ...)
 
 	/*
 	 * We may have ended up stopping the CPU holding the lock (in
-	 * smp_send_stop()) while still having some valuable data in the console
-	 * buffer.  Try to acquire the lock then release it regardless of the
-	 * result.  The release will also print the buffers out.  Locks debug
-	 * should be disabled to avoid reporting bad unlock balance when
+	 * panic_smp_send_stop()) while still having some valuable data in the
+	 * console buffer.  Try to acquire the lock then release it regardless
+	 * of the result.  The release will also print the buffers out.  Locks
+	 * debug should be disabled to avoid reporting bad unlock balance when
 	 * panic() is not being callled from OOPS.
 	 */
 	debug_locks_off();

Powered by blists - more mailing lists