Re: [PATCH] kdump: Fix crash_kexec - smp_send_stop race in panic

[Chris Metcalf <cmetcalf@tilera.com>]

On 11/11/2011 7:28 AM, Michael Holzheu wrote:
> Hello Chris,
>
> On Thu, 2011-11-10 at 10:11 -0500, Chris Metcalf wrote:
>> On 11/10/2011 9:22 AM, Michael Holzheu wrote:
> [snip]
>
>> If a cleaner API seems useful (either for power reasons or restartability
>> or whatever), I suppose a standard global function name could be specified
>> that's the thing you execute when you get an smp_send_stop IPI (in tile's
>> case it's "smp_stop_cpu_interrupt()") and the panic() code could instead
>> just do an atomic_inc_return() of a global panic counter, and if it wasn't
>> the first panicking cpu, call directly into the smp_stop handler routine to
>> quiesce itself.  Then the panicking cpu could finish whatever it needs to
>> do and then halt, reboot, etc., all the cpus.
> Thanks for the info. So introducing a "weak" function that can stop the
> CPU it is running on could solve the problem. Every architecture can
> override the function with something appropriate. E.g. "tile" can use
> the lower-power "nap" instruction there.
>
> What about the following patch.

Seems reasonable to me.

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

>
> Michael
> ---
> From: Michael Holzheu<holzheu@linux.vnet.ibm.com>
> Subject: kdump: fix crash_kexec()/smp_send_stop() race in panic
>
> When two CPUs call panic at the same time there is a possible race
> condition that can stop kdump.  The first CPU calls crash_kexec() and the
> second CPU calls smp_send_stop() in panic() before crash_kexec() finished
> on the first CPU.  So the second CPU stops the first CPU and therefore
> kdump fails:
>
> 1st CPU:
> panic()->crash_kexec()->mutex_trylock(&kexec_mutex)->  do kdump
>
> 2nd CPU:
> panic()->crash_kexec()->kexec_mutex already held by 1st CPU
>         ->smp_send_stop()->  stop 1st CPU (stop kdump)
>
> This patch fixes the problem by introducing a spinlock in panic that
> allows only one CPU to process crash_kexec() and the subsequent panic
> code.
>
> Signed-off-by: Michael Holzheu<holzheu@linux.vnet.ibm.com>
> ---
>   kernel/panic.c |   18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
>
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -49,6 +49,15 @@ static long no_blink(int state)
>   long (*panic_blink)(int state);
>   EXPORT_SYMBOL(panic_blink);
>
> +/*
> + * Stop ourself in panic -- architecture code may override this
> + */
> +void __attribute__ ((weak)) panic_smp_self_stop(void)
> +{
> +	while (1)
> +		cpu_relax();
> +}
> +
>   /**
>    *	panic - halt the system
>    *	@fmt: The text string to print
> @@ -59,6 +68,7 @@ EXPORT_SYMBOL(panic_blink);
>    */
>   NORET_TYPE void panic(const char * fmt, ...)
>   {
> +	static DEFINE_SPINLOCK(panic_lock);
>   	static char buf[1024];
>   	va_list args;
>   	long i, i_next = 0;
> @@ -68,8 +78,14 @@ NORET_TYPE void panic(const char * fmt,
>   	 * It's possible to come here directly from a panic-assertion and
>   	 * not have preempt disabled. Some functions called from here want
>   	 * preempt to be disabled. No point enabling it later though...
> +	 *
> +	 * 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().
>   	 */
> -	preempt_disable();
> +	if (!spin_trylock(&panic_lock))
> +		panic_smp_self_stop();
>
>   	console_verbose();
>   	bust_spinlocks(1);
>
>

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec