This series include two related groups:
[1-3/4]: protect nr_cpus from rebooting by broadcast mce
[4/4]: improve "kexec -l" robustness against broadcast mce
When I tried to fix [1], Thomas raised concern about the nr_cpus' vulnerability
to unexpected rebooting by broadcast mce. After analysis, I think only the
following first case suffers from the rebooting by broadcast mce. [1-3/4] aims
to fix that issue.
*** Back ground ***
On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
The option 'nosmt' has already complied with the above rule by Thomas's patch.
For detail, refer to 506a66f3748 (Revert "x86/apic: Ignore secondary threads if
nosmt=force")
But for nr_cpus option, the exposure to broadcast MCE is a little complicated,
and can be categorized into three cases.
-1. boot up by BIOS. Since no one set CR4.MCE=1, nr_cpus risks rebooting by
broadcast MCE.
-2. boot up by "kexec -p nr_cpus=". Since the 1st kernel has all cpus'
CR4.MCE=1 set before kexec -p, nr_cpus is free of rebooting by broadcast MCE.
Furthermore, the crashed kernel's wreckage, including page table and text, is
not touched by capture kernel. Hence if MCE event happens on capped cpu,
do_machine_check->__mc_check_crashing_cpu() runs smoothly and returns
immediately, the capped cpu is still pinned on "halt".
-3. boot up by "kexec -l nr_cpus=". As "kexec -p", it is free of rebooting by
broadcast MCE. But the 1st kernel's wreckage is discarded and changed. when
capped cpus execute do_machine_check(), they may crack the new kernel. But
this is not related with broadcast MCE, and need an extra fix.
*** Solution ***
"nr_cpus" can not follow the same way as "nosmt". Because nr_cpus limits the
allocation of percpu area and some other kthread memory, which is critical to
cpu hotplug framework. Instead, developing a dedicated SIPI callback
make_capped_cpu_stable() for capped cpu, which does not lean on percpu area to
work.
[1]: https://lkml.org/lkml/2019/7/5/3
To: Gleixner <[email protected]>
To: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
To: [email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Daniel Drake <[email protected]>
Cc: Jacob Pan <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: [email protected]
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: [email protected]
---
Pingfan Liu (4):
x86/apic: correct the ENO in generic_processor_info()
x86/apic: record capped cpu in generic_processor_info()
x86/smp: send capped cpus to a stable state when smp_init()
x86/smp: disallow MCE handler on rebooting AP
arch/x86/include/asm/apic.h | 1 +
arch/x86/include/asm/smp.h | 3 ++
arch/x86/kernel/apic/apic.c | 23 ++++++++----
arch/x86/kernel/cpu/common.c | 7 ++++
arch/x86/kernel/smp.c | 8 +++++
arch/x86/kernel/smpboot.c | 83 ++++++++++++++++++++++++++++++++++++++++++++
kernel/smp.c | 6 ++++
7 files changed, 124 insertions(+), 7 deletions(-)
--
2.7.5
When capped by nr_cpu_ids, the current code return -EINVAL or -ENODEV. It
is better to return -EINVAL for both cases.
Signed-off-by: Pingfan Liu <[email protected]>
To: Thomas Gleixner <[email protected]>
To: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
To: [email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Daniel Drake <[email protected]>
Cc: Jacob Pan <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: [email protected]
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: [email protected]
---
arch/x86/kernel/apic/apic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index f529136..f4f603a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2346,7 +2346,7 @@ int generic_processor_info(int apicid, int version)
" Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
disabled_cpus++;
- return -ENODEV;
+ return -EINVAL;
}
if (num_processors >= nr_cpu_ids) {
--
2.7.5
No matter the cpu is capped by nr_cpus option, recording the mapping
between all cpus' id and apic id
Later this mapping will be used by BSP to sent SIPI to bring capped cpu to
stable state
Signed-off-by: Pingfan Liu <[email protected]>
To: Thomas Gleixner <[email protected]>
To: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
To: [email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Daniel Drake <[email protected]>
Cc: Jacob Pan <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: [email protected]
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: [email protected]
---
arch/x86/include/asm/smp.h | 2 ++
arch/x86/kernel/apic/apic.c | 21 +++++++++++++++------
arch/x86/kernel/cpu/common.c | 4 ++++
3 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index e1356a3..5f63399 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -196,5 +196,7 @@ extern void nmi_selftest(void);
#define nmi_selftest() do { } while (0)
#endif
+extern struct cpumask *cpu_capped_mask;
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index f4f603a..6a57bad3 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2296,9 +2296,10 @@ static int allocate_logical_cpuid(int apicid)
int generic_processor_info(int apicid, int version)
{
- int cpu, max = nr_cpu_ids;
+ int thiscpu, cpu, max = nr_cpu_ids;
bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
phys_cpu_present_map);
+ bool capped = false;
/*
* boot_cpu_physical_apicid is designed to have the apicid
@@ -2322,7 +2323,7 @@ int generic_processor_info(int apicid, int version)
if (disabled_cpu_apicid != BAD_APICID &&
disabled_cpu_apicid != read_apic_id() &&
disabled_cpu_apicid == apicid) {
- int thiscpu = num_processors + disabled_cpus;
+ thiscpu = num_processors + disabled_cpus;
pr_warning("APIC: Disabling requested cpu."
" Processor %d/0x%x ignored.\n",
@@ -2338,7 +2339,7 @@ int generic_processor_info(int apicid, int version)
*/
if (!boot_cpu_detected && num_processors >= nr_cpu_ids - 1 &&
apicid != boot_cpu_physical_apicid) {
- int thiscpu = max + disabled_cpus - 1;
+ thiscpu = max + disabled_cpus - 1;
pr_warning(
"APIC: NR_CPUS/possible_cpus limit of %i almost"
@@ -2346,20 +2347,28 @@ int generic_processor_info(int apicid, int version)
" Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
disabled_cpus++;
- return -EINVAL;
+ capped = true;
}
if (num_processors >= nr_cpu_ids) {
- int thiscpu = max + disabled_cpus;
+ thiscpu = max + disabled_cpus;
pr_warning("APIC: NR_CPUS/possible_cpus limit of %i "
"reached. Processor %d/0x%x ignored.\n",
max, thiscpu, apicid);
disabled_cpus++;
- return -EINVAL;
+ capped = true;
}
+ if (capped) {
+ /* record the mapping between capped cpu and apicid */
+ if (thiscpu < NR_CPUS && cpu_capped_mask != NULL) {
+ cpuid_to_apicid[thiscpu] = apicid;
+ cpumask_set_cpu(thiscpu, cpu_capped_mask);
+ }
+ return -EINVAL;
+ }
if (apicid == boot_cpu_physical_apicid) {
/*
* x86_bios_cpu_apicid is required to have processors listed
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 1147217..4d87df5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -66,6 +66,9 @@ u32 elf_hwcap2 __read_mostly;
cpumask_var_t cpu_initialized_mask;
cpumask_var_t cpu_callout_mask;
cpumask_var_t cpu_callin_mask;
+/* size of NR_CPUS is required. */
+struct cpumask __cpu_capped_mask __initdata;
+struct cpumask *cpu_capped_mask;
/* representing cpus for which sibling maps can be computed */
cpumask_var_t cpu_sibling_setup_mask;
@@ -84,6 +87,7 @@ void __init setup_cpu_local_masks(void)
alloc_bootmem_cpumask_var(&cpu_callin_mask);
alloc_bootmem_cpumask_var(&cpu_callout_mask);
alloc_bootmem_cpumask_var(&cpu_sibling_setup_mask);
+ cpu_capped_mask = &__cpu_capped_mask;
}
static void default_init(struct cpuinfo_x86 *c)
--
2.7.5
*** Back ground ***
On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
The option 'nosmt' has already complied with the above rule by Thomas's
patch. For detail, refer to 506a66f3748 (Revert "x86/apic: Ignore secondary
threads if nosmt=force")
But for nr_cpus option, the exposure to broadcast MCE is a little
complicated, and can be categorized into three cases.
-1. boot up by BIOS. Since no one set CR4.MCE=1, nr_cpus risks rebooting by
broadcast MCE.
-2. boot up by "kexec -p nr_cpus=". Since the 1st kernel has all cpus'
CR4.MCE=1 set before kexec -p, nr_cpus is free of rebooting by broadcast
MCE. Furthermore, the crashed kernel's wreckage, including page table and
text, is not touched by capture kernel. Hence if MCE event happens on
capped cpu, do_machine_check->__mc_check_crashing_cpu() runs smoothly and
returns immediately, the capped cpu is still pinned on "halt".
-3. boot up by "kexec -l nr_cpus=". As "kexec -p", it is free of rebooting
by broadcast MCE. But the 1st kernel's wreckage is discarded and changed.
when capped cpus execute do_machine_check(), they may crack the new kernel.
But this is not related with broadcast MCE, and need an extra fix.
In a word, only case 1 suffers from unexpected rebooting due to broadcast
MCE.
*** Solution ***
When fixing case 1, "nr_cpus" can not follow the same way as "nosmt".
Because nr_cpus limits the allocation of percpu area and some other kthread
memory, which is critical to cpu hotplug framework.
Instead, developing a dedicated SIPI callback make_capped_cpu_stable() for
capped cpu, which does not lean on percpu area to work.
BTW this patch has side effect to suppress case 3. It brings capped cpu to
make_capped_cpu_stable() in the 2nd kernel's context, instead of leaving
them with the 1st kernel's context.
Signed-off-by: Pingfan Liu <[email protected]>
To: Thomas Gleixner <[email protected]>
To: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
To: [email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Daniel Drake <[email protected]>
Cc: Jacob Pan <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: [email protected]
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: [email protected]
---
arch/x86/include/asm/apic.h | 1 +
arch/x86/include/asm/smp.h | 1 +
arch/x86/kernel/apic/apic.c | 2 +-
arch/x86/kernel/cpu/common.c | 3 ++
arch/x86/kernel/smpboot.c | 83 ++++++++++++++++++++++++++++++++++++++++++++
kernel/smp.c | 6 ++++
6 files changed, 95 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index e647aa0..02232e3 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -49,6 +49,7 @@ static inline void generic_apic_probe(void)
#ifdef CONFIG_X86_LOCAL_APIC
+extern int cpuid_to_apicid[];
extern int apic_verbosity;
extern int local_apic_timer_c2_ok;
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 5f63399..f67ce77 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -197,6 +197,7 @@ extern void nmi_selftest(void);
#endif
extern struct cpumask *cpu_capped_mask;
+extern struct cpumask *cpu_capped_done_mask;
#endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 6a57bad3..b86c9e6 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2244,7 +2244,7 @@ static int nr_logical_cpuids = 1;
/*
* Used to store mapping between logical CPU IDs and APIC IDs.
*/
-static int cpuid_to_apicid[] = {
+int cpuid_to_apicid[] = {
[0 ... NR_CPUS - 1] = -1,
};
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4d87df5..1a62b37 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -69,6 +69,8 @@ cpumask_var_t cpu_callin_mask;
/* size of NR_CPUS is required. */
struct cpumask __cpu_capped_mask __initdata;
struct cpumask *cpu_capped_mask;
+struct cpumask __cpu_capped_done_mask __initdata;
+struct cpumask *cpu_capped_done_mask;
/* representing cpus for which sibling maps can be computed */
cpumask_var_t cpu_sibling_setup_mask;
@@ -88,6 +90,7 @@ void __init setup_cpu_local_masks(void)
alloc_bootmem_cpumask_var(&cpu_callout_mask);
alloc_bootmem_cpumask_var(&cpu_sibling_setup_mask);
cpu_capped_mask = &__cpu_capped_mask;
+ cpu_capped_done_mask = &__cpu_capped_done_mask;
}
static void default_init(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fdbd47c..d247fbd 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1014,6 +1014,89 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
return 0;
}
+/* SIPI callback for the capped cpu */
+static void notrace make_capped_cpu_stable(void *unused)
+{
+ unsigned int apic_id;
+ int i, cpu = -1;
+
+ /* protect against MCE broadcast */
+ native_write_cr4(X86_CR4_MCE);
+ x2apic_setup();
+ apic_id = read_apic_id();
+ for (i = 0; i < NR_CPUS; i++)
+ if (cpuid_to_apicid[i] == apic_id) {
+ cpu = i;
+ break;
+ }
+
+ /* trampoline_*.S has loaded idt with 0, 0 */
+
+ /* No response to any interrupt */
+ disable_local_APIC();
+ /* Done with the temporary stack, signal the master to go ahead */
+ if (cpu != -1) {
+ cpumask_clear_cpu(cpu, cpu_capped_mask);
+ cpumask_set_cpu(cpu, cpu_capped_done_mask);
+ }
+
+ /* run without stack and can not have the __init attribute */
+ do {
+ asm volatile("hlt" : : : "memory");
+ } while (true);
+}
+
+static void __init do_stable_cpu(int cpu)
+{
+ static char capped_tmp_stack[512];
+ int cpu0_nmi_registered = 0, apicid = cpuid_to_apicid[cpu];
+ unsigned long start_ip = real_mode_header->trampoline_start;
+ unsigned long timeout, boot_error = 0;
+
+ /* invalid percpu area */
+ initial_gs = 0;
+ /*
+ * Borrow the value of cpu 0. Since capped cpu segment shadow register
+ * can cache the content, and keep it unchanged.
+ */
+ early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
+ initial_code = (unsigned long)make_capped_cpu_stable;
+ initial_stack = (unsigned long)&capped_tmp_stack;
+
+ if (apic->wakeup_secondary_cpu)
+ boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
+ else
+ boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
+ &cpu0_nmi_registered);
+ if (cpu0_nmi_registered)
+ unregister_nmi_handler(NMI_LOCAL, "wake_cpu0");
+
+ if (!boot_error) {
+ /* Wait 10s total for first sign of life from capped cpu */
+ boot_error = -1;
+ timeout = jiffies + 10*HZ;
+ while (time_before(jiffies, timeout)) {
+ if (cpumask_test_cpu(cpu, cpu_capped_done_mask))
+ break;
+ schedule();
+ }
+ }
+}
+
+void __init bring_capped_cpu_stable(void)
+{
+ int cpu;
+
+ /* Guest does not suffer from MCE broadcast */
+ if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+ return;
+
+ for_each_cpu(cpu, cpu_capped_mask)
+ do_stable_cpu(cpu);
+ cpu_capped_mask = NULL;
+ cpu_capped_done_mask = NULL;
+}
+
/*
* NOTE - on most systems this is a PHYSICAL apic ID, but on multiquad
* (ie clustered apic addressing mode), this is a LOGICAL apic ID.
diff --git a/kernel/smp.c b/kernel/smp.c
index 7dbcb40..de528b7 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -574,6 +574,10 @@ void __init setup_nr_cpu_ids(void)
nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
}
+void __weak __init bring_capped_cpu_stable(void)
+{
+}
+
/* Called by boot processor to activate the rest. */
void __init smp_init(void)
{
@@ -593,6 +597,8 @@ void __init smp_init(void)
cpu_up(cpu);
}
+ /* force cpus capped by nr_cpus option into stable state */
+ bring_capped_cpu_stable();
num_nodes = num_online_nodes();
num_cpus = num_online_cpus();
pr_info("Brought up %d node%s, %d CPU%s\n",
--
2.7.5
"kexec -l" sends the rest cpu to halt state with local apic disabled. But
they can still respond to MCE. Meanwhile the execution of MCE handler
relies on the 1st kernel's page table and text, which may be cracked during
the 2nd kernel bootup. Hence Before sending SIPI to AP in 2nd kernel, an
MCE event makes AP take the risk of running in weird context.
Heavily suppress it by disallowing MCE handler on rebooting AP.
Note: after this patch, "kexec -l" still has a little window vulnerable to
weird context, despite AP uses tlb cache and icache. Consider the
scenario: The 1st kernel code native_halt() in stop_this_cpu() is modified
during the 2nd kernel bootup. Then AP is waken up by MCE after the
modification, and will continue in a weired context. This needs extra
effort.
Signed-off-by: Pingfan Liu <[email protected]>
To: Thomas Gleixner <[email protected]>
To: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
To: [email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Daniel Drake <[email protected]>
Cc: Jacob Pan <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: [email protected]
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: [email protected]
---
arch/x86/kernel/smp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 96421f9..55b0f11 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -157,11 +157,15 @@ void native_send_call_func_ipi(const struct cpumask *mask)
static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
{
+ struct desc_ptr null_ptr = { 0 };
+
/* We are registered on stopping cpu too, avoid spurious NMI */
if (raw_smp_processor_id() == atomic_read(&stopping_cpu))
return NMI_HANDLED;
cpu_emergency_vmxoff();
+ /* prevent from dispatching MCE handler */
+ load_idt(&null_ptr);
stop_this_cpu(NULL);
return NMI_HANDLED;
@@ -173,8 +177,12 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
asmlinkage __visible void smp_reboot_interrupt(void)
{
+ struct desc_ptr null_ptr = { 0 };
+
ipi_entering_ack_irq();
cpu_emergency_vmxoff();
+ /* prevent from dispatching MCE handler */
+ load_idt(&null_ptr);
stop_this_cpu(NULL);
irq_exit();
}
--
2.7.5
Add Tony and Xunlei in cc.
On 08/05/19 at 04:58pm, Pingfan Liu wrote:
> This series include two related groups:
> [1-3/4]: protect nr_cpus from rebooting by broadcast mce
> [4/4]: improve "kexec -l" robustness against broadcast mce
>
> When I tried to fix [1], Thomas raised concern about the nr_cpus' vulnerability
> to unexpected rebooting by broadcast mce. After analysis, I think only the
> following first case suffers from the rebooting by broadcast mce. [1-3/4] aims
> to fix that issue.
I did not understand and read the MCE details, but we previously had a
MCE problem, Xunlei fixed in below commit:
commit 5bc329503e8191c91c4c40836f062ef771d8ba83
Author: Xunlei Pang <[email protected]>
Date: Mon Mar 13 10:50:19 2017 +0100
x86/mce: Handle broadcasted MCE gracefully with kexec
I wonder if this is same issue or not. Also the old discussion is in
below thread:
https://lore.kernel.org/patchwork/patch/753530/
Tony raised similar questions, but I'm not sure if it is still a problem
or it has been fixed.
>
> *** Back ground ***
>
> On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
> broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
>
> The option 'nosmt' has already complied with the above rule by Thomas's patch.
> For detail, refer to 506a66f3748 (Revert "x86/apic: Ignore secondary threads if
> nosmt=force")
>
> But for nr_cpus option, the exposure to broadcast MCE is a little complicated,
> and can be categorized into three cases.
>
> -1. boot up by BIOS. Since no one set CR4.MCE=1, nr_cpus risks rebooting by
> broadcast MCE.
>
> -2. boot up by "kexec -p nr_cpus=". Since the 1st kernel has all cpus'
> CR4.MCE=1 set before kexec -p, nr_cpus is free of rebooting by broadcast MCE.
> Furthermore, the crashed kernel's wreckage, including page table and text, is
> not touched by capture kernel. Hence if MCE event happens on capped cpu,
> do_machine_check->__mc_check_crashing_cpu() runs smoothly and returns
> immediately, the capped cpu is still pinned on "halt".
>
> -3. boot up by "kexec -l nr_cpus=". As "kexec -p", it is free of rebooting by
> broadcast MCE. But the 1st kernel's wreckage is discarded and changed. when
> capped cpus execute do_machine_check(), they may crack the new kernel. But
> this is not related with broadcast MCE, and need an extra fix.
>
> *** Solution ***
> "nr_cpus" can not follow the same way as "nosmt". Because nr_cpus limits the
> allocation of percpu area and some other kthread memory, which is critical to
> cpu hotplug framework. Instead, developing a dedicated SIPI callback
> make_capped_cpu_stable() for capped cpu, which does not lean on percpu area to
> work.
>
> [1]: https://lkml.org/lkml/2019/7/5/3
>
> To: Gleixner <[email protected]>
> To: Andy Lutomirski <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> To: [email protected]
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Qian Cai <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Daniel Drake <[email protected]>
> Cc: Jacob Pan <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Eric Biederman <[email protected]>
> Cc: [email protected]
> Cc: Dave Young <[email protected]>
> Cc: Baoquan He <[email protected]>
> Cc: [email protected]
>
> ---
> Pingfan Liu (4):
> x86/apic: correct the ENO in generic_processor_info()
> x86/apic: record capped cpu in generic_processor_info()
> x86/smp: send capped cpus to a stable state when smp_init()
> x86/smp: disallow MCE handler on rebooting AP
>
> arch/x86/include/asm/apic.h | 1 +
> arch/x86/include/asm/smp.h | 3 ++
> arch/x86/kernel/apic/apic.c | 23 ++++++++----
> arch/x86/kernel/cpu/common.c | 7 ++++
> arch/x86/kernel/smp.c | 8 +++++
> arch/x86/kernel/smpboot.c | 83 ++++++++++++++++++++++++++++++++++++++++++++
> kernel/smp.c | 6 ++++
> 7 files changed, 124 insertions(+), 7 deletions(-)
>
> --
> 2.7.5
>
Thanks
Dave
On Wed, Aug 07, 2019 at 11:00:41AM +0800, Dave Young wrote:
> Add Tony and Xunlei in cc.
> On 08/05/19 at 04:58pm, Pingfan Liu wrote:
> > This series include two related groups:
> > [1-3/4]: protect nr_cpus from rebooting by broadcast mce
> > [4/4]: improve "kexec -l" robustness against broadcast mce
> >
> > When I tried to fix [1], Thomas raised concern about the nr_cpus' vulnerability
> > to unexpected rebooting by broadcast mce. After analysis, I think only the
> > following first case suffers from the rebooting by broadcast mce. [1-3/4] aims
> > to fix that issue.
>
> I did not understand and read the MCE details, but we previously had a
> MCE problem, Xunlei fixed in below commit:
> commit 5bc329503e8191c91c4c40836f062ef771d8ba83
> Author: Xunlei Pang <[email protected]>
> Date: Mon Mar 13 10:50:19 2017 +0100
>
> x86/mce: Handle broadcasted MCE gracefully with kexec
>
> I wonder if this is same issue or not. Also the old discussion is in
> below thread:
> https://lore.kernel.org/patchwork/patch/753530/
>
> Tony raised similar questions, but I'm not sure if it is still a problem
> or it has been fixed.
>
Xunlei's patch is the precondition of the stability for the case 2: boot up by "kexec -p nr_cpus="
For case1/3, extra effort is needed.
Thanks,
Pingfan
> >
> > *** Back ground ***
> >
> > On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
> > broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
> >
> > The option 'nosmt' has already complied with the above rule by Thomas's patch.
> > For detail, refer to 506a66f3748 (Revert "x86/apic: Ignore secondary threads if
> > nosmt=force")
> >
> > But for nr_cpus option, the exposure to broadcast MCE is a little complicated,
> > and can be categorized into three cases.
> >
> > -1. boot up by BIOS. Since no one set CR4.MCE=1, nr_cpus risks rebooting by
> > broadcast MCE.
> >
> > -2. boot up by "kexec -p nr_cpus=". Since the 1st kernel has all cpus'
> > CR4.MCE=1 set before kexec -p, nr_cpus is free of rebooting by broadcast MCE.
> > Furthermore, the crashed kernel's wreckage, including page table and text, is
> > not touched by capture kernel. Hence if MCE event happens on capped cpu,
> > do_machine_check->__mc_check_crashing_cpu() runs smoothly and returns
> > immediately, the capped cpu is still pinned on "halt".
> >
> > -3. boot up by "kexec -l nr_cpus=". As "kexec -p", it is free of rebooting by
> > broadcast MCE. But the 1st kernel's wreckage is discarded and changed. when
> > capped cpus execute do_machine_check(), they may crack the new kernel. But
> > this is not related with broadcast MCE, and need an extra fix.
> >
> > *** Solution ***
> > "nr_cpus" can not follow the same way as "nosmt". Because nr_cpus limits the
> > allocation of percpu area and some other kthread memory, which is critical to
> > cpu hotplug framework. Instead, developing a dedicated SIPI callback
> > make_capped_cpu_stable() for capped cpu, which does not lean on percpu area to
> > work.
> >
> > [1]: https://lkml.org/lkml/2019/7/5/3
> >
> > To: Gleixner <[email protected]>
> > To: Andy Lutomirski <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: Dave Hansen <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > To: [email protected]
> > Cc: Masami Hiramatsu <[email protected]>
> > Cc: Qian Cai <[email protected]>
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Daniel Drake <[email protected]>
> > Cc: Jacob Pan <[email protected]>
> > Cc: Michal Hocko <[email protected]>
> > Cc: Eric Biederman <[email protected]>
> > Cc: [email protected]
> > Cc: Dave Young <[email protected]>
> > Cc: Baoquan He <[email protected]>
> > Cc: [email protected]
> >
> > ---
> > Pingfan Liu (4):
> > x86/apic: correct the ENO in generic_processor_info()
> > x86/apic: record capped cpu in generic_processor_info()
> > x86/smp: send capped cpus to a stable state when smp_init()
> > x86/smp: disallow MCE handler on rebooting AP
> >
> > arch/x86/include/asm/apic.h | 1 +
> > arch/x86/include/asm/smp.h | 3 ++
> > arch/x86/kernel/apic/apic.c | 23 ++++++++----
> > arch/x86/kernel/cpu/common.c | 7 ++++
> > arch/x86/kernel/smp.c | 8 +++++
> > arch/x86/kernel/smpboot.c | 83 ++++++++++++++++++++++++++++++++++++++++++++
> > kernel/smp.c | 6 ++++
> > 7 files changed, 124 insertions(+), 7 deletions(-)
> >
> > --
> > 2.7.5
> >
>
> Thanks
> Dave
On Wed, 7 Aug 2019, Pingfan Liu wrote:
> On Wed, Aug 07, 2019 at 11:00:41AM +0800, Dave Young wrote:
> > Add Tony and Xunlei in cc.
> > On 08/05/19 at 04:58pm, Pingfan Liu wrote:
> > > This series include two related groups:
> > > [1-3/4]: protect nr_cpus from rebooting by broadcast mce
> > > [4/4]: improve "kexec -l" robustness against broadcast mce
> > >
> > > When I tried to fix [1], Thomas raised concern about the nr_cpus' vulnerability
> > > to unexpected rebooting by broadcast mce. After analysis, I think only the
> > > following first case suffers from the rebooting by broadcast mce. [1-3/4] aims
> > > to fix that issue.
> >
> > I did not understand and read the MCE details, but we previously had a
> > MCE problem, Xunlei fixed in below commit:
> > commit 5bc329503e8191c91c4c40836f062ef771d8ba83
> > Author: Xunlei Pang <[email protected]>
> > Date: Mon Mar 13 10:50:19 2017 +0100
> >
> > x86/mce: Handle broadcasted MCE gracefully with kexec
> >
> > I wonder if this is same issue or not. Also the old discussion is in
> > below thread:
> > https://lore.kernel.org/patchwork/patch/753530/
> >
> > Tony raised similar questions, but I'm not sure if it is still a problem
> > or it has been fixed.
> >
>
> Xunlei's patch is the precondition of the stability for the case 2: boot
> up by "kexec -p nr_cpus="
Correct. The only dangerous issue which is then left is that an MCE hits
_before_ all CPUs have CR.MCE=1 set. That's a general issue also for cold
boot. Thanks to the brilliant hardware design, all we can do is pray....
> For case1/3, extra effort is needed.
>
> Thanks,
> Pingfan
> > >
> > > *** Back ground ***
> > >
> > > On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
> > > broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
Pingfan, please trim your replies and remove the useless gunk after your answer.
Thanks,
tglx
Fixes: 294b1ea98966 ("x86/apic: record capped cpu in generic_processor_info()")
Signed-off-by: kbuild test robot <[email protected]>
---
common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4d87df5be9124..b95721e7376d4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -67,7 +67,7 @@ cpumask_var_t cpu_initialized_mask;
cpumask_var_t cpu_callout_mask;
cpumask_var_t cpu_callin_mask;
/* size of NR_CPUS is required. */
-struct cpumask __cpu_capped_mask __initdata;
+static struct cpumask __cpu_capped_mask __initdata;
struct cpumask *cpu_capped_mask;
/* representing cpus for which sibling maps can be computed */
Hi Pingfan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pingfan-Liu/x86-mce-protect-nr_cpus-from-rebooting-by-broadcast-mce/20190806-101748
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-rc1-7-g2b96cd8-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> arch/x86/kernel/cpu/common.c:70:16: sparse: sparse: symbol '__cpu_capped_mask' was not declared. Should it be static?
arch/x86/kernel/cpu/common.c:135:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:136:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:137:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:138:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:165:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:166:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Pingfan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pingfan-Liu/x86-mce-protect-nr_cpus-from-rebooting-by-broadcast-mce/20190806-101748
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-rc1-7-g2b96cd8-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
arch/x86/kernel/cpu/common.c:70:16: sparse: sparse: symbol '__cpu_capped_mask' was not declared. Should it be static?
>> arch/x86/kernel/cpu/common.c:72:16: sparse: sparse: symbol '__cpu_capped_done_mask' was not declared. Should it be static?
arch/x86/kernel/cpu/common.c:138:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:139:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:140:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:141:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:168:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
arch/x86/kernel/cpu/common.c:169:43: sparse: sparse: cast truncates bits from constant value (fffff becomes ffff)
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Fixes: 946eeafe59c7 ("x86/smp: send capped cpus to a stable state when smp_init()")
Signed-off-by: kbuild test robot <[email protected]>
---
common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 1a62b37a36256..00374883fb842 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -69,7 +69,7 @@ cpumask_var_t cpu_callin_mask;
/* size of NR_CPUS is required. */
struct cpumask __cpu_capped_mask __initdata;
struct cpumask *cpu_capped_mask;
-struct cpumask __cpu_capped_done_mask __initdata;
+static struct cpumask __cpu_capped_done_mask __initdata;
struct cpumask *cpu_capped_done_mask;
/* representing cpus for which sibling maps can be computed */
Hi Pingfan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pingfan-Liu/x86-mce-protect-nr_cpus-from-rebooting-by-broadcast-mce/20190806-101748
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
ld: arch/x86/kernel/smpboot.o: in function `bring_capped_cpu_stable':
>> smpboot.c:(.init.text+0x172): undefined reference to `initial_gs'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Pingfan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pingfan-Liu/x86-mce-protect-nr_cpus-from-rebooting-by-broadcast-mce/20190806-101748
config: i386-randconfig-a004-201931 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
ld: arch/x86/kernel/smpboot.o: in function `do_stable_cpu':
>> arch/x86/kernel/smpboot.c:1057: undefined reference to `initial_gs'
vim +1057 arch/x86/kernel/smpboot.c
1048
1049 static void __init do_stable_cpu(int cpu)
1050 {
1051 static char capped_tmp_stack[512];
1052 int cpu0_nmi_registered = 0, apicid = cpuid_to_apicid[cpu];
1053 unsigned long start_ip = real_mode_header->trampoline_start;
1054 unsigned long timeout, boot_error = 0;
1055
1056 /* invalid percpu area */
> 1057 initial_gs = 0;
1058 /*
1059 * Borrow the value of cpu 0. Since capped cpu segment shadow register
1060 * can cache the content, and keep it unchanged.
1061 */
1062 early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(0);
1063 initial_code = (unsigned long)make_capped_cpu_stable;
1064 initial_stack = (unsigned long)&capped_tmp_stack;
1065
1066 if (apic->wakeup_secondary_cpu)
1067 boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
1068 else
1069 boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
1070 &cpu0_nmi_registered);
1071 if (cpu0_nmi_registered)
1072 unregister_nmi_handler(NMI_LOCAL, "wake_cpu0");
1073
1074 if (!boot_error) {
1075 /* Wait 10s total for first sign of life from capped cpu */
1076 boot_error = -1;
1077 timeout = jiffies + 10*HZ;
1078 while (time_before(jiffies, timeout)) {
1079 if (cpumask_test_cpu(cpu, cpu_capped_done_mask))
1080 break;
1081 schedule();
1082 }
1083 }
1084 }
1085
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Aug 7, 2019 at 9:07 PM Thomas Gleixner <[email protected]> wrote:
>
[...]
> > > >
> > > > *** Back ground ***
> > > >
> > > > On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
> > > > broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
>
> Pingfan, please trim your replies and remove the useless gunk after your answer.
OK. I will.
Thanks,
Pingfan
>
> Thanks,
>
> tglx
On Thu, 8 Aug 2019, Pingfan Liu wrote:
> On Wed, Aug 7, 2019 at 9:07 PM Thomas Gleixner <[email protected]> wrote:
> >
> [...]
> > > > >
> > > > > *** Back ground ***
> > > > >
> > > > > On x86 it's required to have all logical CPUs set CR4.MCE=1. Otherwise, a
> > > > > broadcast MCE observing CR4.MCE=0b on any core will shutdown the machine.
> >
> > Pingfan, please trim your replies and remove the useless gunk after your answer.
> OK. I will.
>
> Thanks,
> Pingfan
> >
> > Thanks,
> >
> > tglx
>
First attempt to do so failed :)