On 2016/07/05 at 19:33, Hidehiro Kawai wrote:
> 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 <dwal...@fifo99.com>
> Fixes: f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai...@hitachi.com>
> Cc: Dave Young <dyo...@redhat.com>
> Cc: Baoquan He <b...@redhat.com>
> Cc: Vivek Goyal <vgo...@redhat.com>
> Cc: Eric Biederman <ebied...@xmission.com>
> Cc: Masami Hiramatsu <mhira...@kernel.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: "H. Peter Anvin" <h...@zytor.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Toshi Kani <toshi.k...@hpe.com>
> Cc: "Peter Zijlstra (Intel)" <pet...@infradead.org>
> Cc: Takao Indoh <indou.ta...@jp.fujitsu.com>
> Cc: "Lee, Chun-Yi" <joeyli.ker...@gmail.com>
> Cc: Minfei Huang <mnfhu...@gmail.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Michal Hocko <mho...@suse.com>
> Cc: Vitaly Kuznetsov <vkuzn...@redhat.com>
> Cc: Petr Mladek <pmla...@suse.com>
> Cc: Tejun Heo <t...@kernel.org>
> Cc: Josh Poimboeuf <jpoim...@redhat.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;
Should be atomic_t type?
> +
> + 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();
If crash_kexec_post_notifiers and kdump both are not used, you actually changed
the original behaviour,
it would be better to make it conditionally like:
if (crash_kexec_post_notifiers)
panic_smp_send_stop(); // renaming it to crash_smp_send_stop() is
better?
else
smp_send_stop();
Regards,
Xunlei
>
> /*
> * 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();
>
>
>
> _______________________________________________
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
