Message-ID: <20160711083449.GA19320@dhcp-128-65.nay.redhat.com>
Date:	Mon, 11 Jul 2016 16:34:49 +0800
From:	Dave Young <dyoung@...hat.com>
To:	Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Baoquan He <bhe@...hat.com>, 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: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump
 friendly version

On 07/05/16 at 08:33pm, 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.

We have been avoiding extra things in panic path, but unfortunately
crash_kexec_post_notifiers were added. I tend to agree the best place
for this stuff is in 2nd kernel or purgatory instead of in 1st kernel.

As for this patch I'm not sure it is safe to replace the smp_send_stop
with the kdump friendly function. I'm also not sure if the kdump friendly
function is safe for kdump. Will glad to hear opinions from other
arch experts.

BTW, if one want to use crash_kexec_post_notifiers he should take the
risk of unreliable kdump. How about only call smp_send_stop in case no
crash_kexec_post_notifiers being used.

> 
> 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.

It does not fix the Problem 1, it seem not possible to fix it?

> 
> 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();
> 
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

Powered by blists - more mailing lists