Re: [PATCH v5] x86: mce: kexec: switch MCE handler for kexec/kdump

[Borislav Petkov <bp@alien8.de>]

On Thu, Mar 05, 2015 at 06:45:10AM +0000, Naoya Horiguchi wrote:
> ----
> From bf4ce58b8296774a69e5436f43e8dc9eed41a829 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Date: Thu, 5 Mar 2015 15:28:23 +0900
> Subject: [PATCH v5] x86: mce: kexec: switch MCE handler for kexec/kdump
> 
> kexec disables (or "shoots down") all CPUs other than a crashing CPU before
> entering the 2nd kernel. But the MCE handler is still enabled after that,
> so if MCE happens and broadcasts over the CPUs after the main thread starts
> the 2nd kernel (which might not initialize MCE device yet, or might decide
> not to enable it,) MCE handler runs only on the other CPUs (not on the main
> thread,) leading to kernel panic with MCE synchronization. The user-visible
> effect of this bug is kdump failure.
> 
> Our standard MCE handler do_machine_check() assumes some about system's
> status and it's hard to alter it to cover kexec/kdump context, so let's add
> another kdump-specific one and switch to it.
> 
> Note that this problem exists since current MCE handler was implemented in
> 2.6.32, and recently commit 716079f66eac ("mce: Panic when a core has reached
> a timeout") made it more visible by changing the default behavior of the
> synchronization timeout from "ignore" to "panic".
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: <stable@vger.kernel.org>        [2.6.32+]

I don't think you can CC stable on something which looks like a new
feature to me.

It is most likely that distros will pick it up separately.

> ---
> ChangeLog v4 -> v5:
> - drop MCE_UC/AR_SEVERITY re-ordering
> - move most of code to arch/x86/kernel/crash.c
> - export some MCE internal variables/routines via arch/x86/include/asm/mce.h
> 
> ChangeLog v3 -> v4:
> - fixed AR and UC order in enum severity_level because UC is severer than AR
>   by definition. Current code is not affected by this wrong order by chance.
> - check severity in machine_check_under_kdump(), and call mce_panic() if the
>   resultant severity is as bad as or worse than MCE_AR_SEVERITY.
> - use static global variable kdump_cpu instead of mca_cfg->kdump_cpu
> - reduce "#ifdef CONFIG_KEXEC"
> - add "#ifdef CONFIG_X86_MCE" for declaration of machine_check_under_kdump()
>   in mce.h
> - update comment on switch_mce_handler_for_kdump()
> 
> ChangeLog v2 -> v3
> - go to "switch MCE handler" approach
> 
> ChangeLog v1 -> v2
> - clear MSR_IA32_MCG_CTL, MSR_IA32_MCx_CTL, and CR4.MCE instead of using
>   global flag to ignore MCE events.
> - fixed the description of the problem
> ---
>  arch/x86/include/asm/mce.h                | 19 +++++++
>  arch/x86/kernel/cpu/mcheck/mce-internal.h | 13 -----
>  arch/x86/kernel/cpu/mcheck/mce.c          | 10 ++--
>  arch/x86/kernel/crash.c                   | 87 +++++++++++++++++++++++++++++++
>  4 files changed, 111 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 51b26e895933..fbb385611a14 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -248,4 +248,23 @@ struct cper_sec_mem_err;
>  extern void apei_mce_report_mem_error(int corrected,
>  				      struct cper_sec_mem_err *mem_err);
>  
> +enum severity_level {
> +	MCE_NO_SEVERITY,
> +	MCE_DEFERRED_SEVERITY,
> +	MCE_UCNA_SEVERITY = MCE_DEFERRED_SEVERITY,
> +	MCE_KEEP_SEVERITY,
> +	MCE_SOME_SEVERITY,
> +	MCE_AO_SEVERITY,
> +	MCE_UC_SEVERITY,
> +	MCE_AR_SEVERITY,
> +	MCE_PANIC_SEVERITY,
> +};
> +
> +int mce_severity(struct mce *a, int tolerant, char **msg, bool is_excp);
> +
> +extern void mce_panic(char *msg, struct mce *final, char *exp);

mce_panic is doing a lot of MCE-specific stuff like flushing out mcelog
etc. I don't think you need all that in your case - I think in your case
you simply want to panic().

> +extern u64 mce_rdmsrl(u32 msr);
> +extern void mce_wrmsrl(u32 msr, u64 v);

Those wrap error injection. I don't think you need that either - use
generic rd/wrmsr* functions.

> +extern inline void mce_gather_info(struct mce *m, struct pt_regs *regs);

This has a vm86 mode special case. Also probably not needed for you. You
can simply read MSR_IA32_MCG_STATUS in a simplified, private version.

> +extern void (*quirk_no_way_out)(int bank, struct mce *m, struct pt_regs *regs);

Now this is exposing a really MCE-internal function. You probably should add a

	mce_callback(bank, m, regs);

in a prepatch and call it from your code.

With the above simplified versions used, the rest of the patch becomes
almost trivial.

-- 
Regards/Gruss,
    Boris.

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