Re: [V5 PATCH 3/4] kexec: Fix race between panic() and crash_kexec() called directly

[Borislav Petkov <bp@alien8.de>]

On Fri, Nov 20, 2015 at 06:36:48PM +0900, Hidehiro Kawai wrote:
> Currently, panic() and crash_kexec() can be called at the same time.
> For example (x86 case):
> 
> CPU 0:
>   oops_end()
>     crash_kexec()
>       mutex_trylock() // acquired
>         nmi_shootdown_cpus() // stop other cpus
> 
> CPU 1:
>   panic()
>     crash_kexec()
>       mutex_trylock() // failed to acquire
>     smp_send_stop() // stop other cpus
>     infinite loop
> 
> If CPU 1 calls smp_send_stop() before nmi_shootdown_cpus(), kdump
> fails.
> 
> In another case:
> 
> CPU 0:
>   oops_end()
>     crash_kexec()
>       mutex_trylock() // acquired
>         <NMI>
>         io_check_error()
>           panic()
>             crash_kexec()
>               mutex_trylock() // failed to acquire
>             infinite loop
> 
> Clearly, this is an undesirable result.
> 
> To fix this problem, this patch changes crash_kexec() to exclude
> others by using atomic_t panic_cpu.
> 
> V5:
> - Add missing dummy __crash_kexec() for !CONFIG_KEXEC_CORE case
> - Replace atomic_xchg() with atomic_set() in crash_kexec() because
>   it is used as a release operation and there is no need of memory
>   barrier effect.  This change also removes an unused value warning
> 
> V4:
> - Use new __crash_kexec(), no exclusion check version of crash_kexec(),
>   instead of checking if panic_cpu is the current cpu or not
> 
> V2:
> - Use atomic_cmpxchg() instead of spin_trylock() on panic_lock
>   to exclude concurrent accesses
> - Don't introduce no-lock version of crash_kexec()
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> ---
>  include/linux/kexec.h |    2 ++
>  kernel/kexec_core.c   |   26 +++++++++++++++++++++++++-
>  kernel/panic.c        |    4 ++--
>  3 files changed, 29 insertions(+), 3 deletions(-)

...

> +void crash_kexec(struct pt_regs *regs)
> +{
> +	int old_cpu, this_cpu;
> +
> +	/*
> +	 * Only one CPU is allowed to execute the crash_kexec() code as with
> +	 * panic().  Otherwise parallel calls of panic() and crash_kexec()
> +	 * may stop each other.  To exclude them, we use panic_cpu here too.
> +	 */
> +	this_cpu = raw_smp_processor_id();
> +	old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu);
> +	if (old_cpu == -1) {
> +		/* This is the 1st CPU which comes here, so go ahead. */
> +		__crash_kexec(regs);
> +
> +		/*
> +		 * Reset panic_cpu to allow another panic()/crash_kexec()
> +		 * call.

So can we make __crash_kexec() return error values?

* failed to grab kexec_mutex -> reset panic_cpu

* no kexec_crash_image -> no need to reset it, all future crash_kexec()
calls won't work so no need to run into that path anymore. However, this could
be problematic if we want the other CPUs to panic. Do we care?

* machine_kexec successful -> doesn't matter

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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