Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532274 > unrolled thread

Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage!

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Borislav Petkov <bp@alien8.de> - 2016-11-29 14:30 +0100
    Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Jiri Olsa <jolsa@redhat.com> - 2016-11-29 15:10 +0100
    Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Thomas Gleixner <tglx@linutronix.de> - 2016-11-29 15:10 +0100
      Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Borislav Petkov <bp@alien8.de> - 2016-11-30 09:50 +0100
        Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Thomas Gleixner <tglx@linutronix.de> - 2016-11-30 10:00 +0100
          Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Borislav Petkov <bp@alien8.de> - 2016-11-30 10:10 +0100
            Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage! Thomas Gleixner <tglx@linutronix.de> - 2016-11-30 10:20 +0100

#1532274 — Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage!

On Mon, Nov 21, 2016 at 05:06:54PM +0100, Borislav Petkov wrote:
> IOW, what's the worst thing that can happen if we did this below?
> 
> We basically get rid of the detection and switch the timer to broadcast
> mode immediately on the halting CPU.
> 
> amd_e400_idle() is behind an "if (cpu_has_bug(c, X86_BUG_AMD_APIC_C1E))"
> check so it will run on the affected CPUs only...
> 
> Thoughts?

Actually, here's a better version. The E400 detection works only after
ACPI has been enabled so we piggyback the end of acpi_init().

We don't need the MSR read now - we do

	if (static_cpu_has_bug(X86_BUG_AMD_APIC_C1E))

on the idle path which is as fast as it gets.

Any complaints about this before I go and test it everywhere?

It builds and boots in my guest here ok, not that it means a whole lot.

The good news is, I have collected a fleet of boxes which all have that
erratum so testing should be pretty reliable. Something that doesn't
happen everytime!

:-)

---
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 5391b0ae7cc3..aa8c879e814f 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -94,7 +94,8 @@ static inline unsigned int acpi_processor_cstate_check(unsigned int max_cstate)
 	    boot_cpu_data.x86_model <= 0x05 &&
 	    boot_cpu_data.x86_mask < 0x0A)
 		return 1;
-	else if (amd_e400_c1e_detected)
+
+	else if (boot_cpu_has(X86_BUG_AMD_APIC_C1E))
 		return 1;
 	else
 		return max_cstate;
@@ -131,6 +132,7 @@ static inline bool acpi_has_cpu_in_madt(void)
 	return !!acpi_lapic;
 }
 
+extern void __init __weak arch_post_acpi_init(void);
 #else /* !CONFIG_ACPI */
 
 #define acpi_lapic 0
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 1d2b69fc0ceb..b3918609f6f1 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -204,6 +204,7 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
 
 #define static_cpu_has_bug(bit)		static_cpu_has((bit))
 #define boot_cpu_has_bug(bit)		cpu_has_bug(&boot_cpu_data, (bit))
+#define setup_clear_cpu_bug(bit)	setup_clear_cpu_cap((bit))
 
 #define MAX_CPU_FEATURES		(NCAPINTS * 32)
 #define cpu_have_feature		boot_cpu_has
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 1f6a92903b09..d98c5c432209 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -636,7 +636,6 @@ extern void select_idle_routine(const struct cpuinfo_x86 *c);
 extern void init_amd_e400_c1e_mask(void);
 
 extern unsigned long		boot_option_idle_override;
-extern bool			amd_e400_c1e_detected;
 
 enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT,
 			 IDLE_POLL};
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index ee023919e476..56653b7322f9 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -289,9 +289,6 @@ void stop_this_cpu(void *dummy)
 		halt();
 }
 
-bool amd_e400_c1e_detected;
-EXPORT_SYMBOL(amd_e400_c1e_detected);
-
 static cpumask_var_t amd_e400_c1e_mask;
 
 void amd_e400_remove_cpu(int cpu)
@@ -301,26 +298,14 @@ void amd_e400_remove_cpu(int cpu)
 }
 
 /*
- * AMD Erratum 400 aware idle routine. We check for C1E active in the interrupt
- * pending message MSR. If we detect C1E, then we handle it the same
- * way as C3 power states (local apic timer and TSC stop)
+ * AMD Erratum 400 aware idle routine.
+ *
+ *
+ * We handle it the same way as C3 power states (local apic timer and TSC stop)
  */
 static void amd_e400_idle(void)
 {
-	if (!amd_e400_c1e_detected) {
-		u32 lo, hi;
-
-		rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
-
-		if (lo & K8_INTP_C1E_ACTIVE_MASK) {
-			amd_e400_c1e_detected = true;
-			if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
-				mark_tsc_unstable("TSC halt in AMD C1E");
-			pr_info("System has AMD C1E enabled\n");
-		}
-	}
-
-	if (amd_e400_c1e_detected) {
+	if (static_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) {
 		int cpu = smp_processor_id();
 
 		if (!cpumask_test_cpu(cpu, amd_e400_c1e_mask)) {
@@ -419,6 +404,30 @@ void __init init_amd_e400_c1e_mask(void)
 		zalloc_cpumask_var(&amd_e400_c1e_mask, GFP_KERNEL);
 }
 
+void __init arch_post_acpi_init(void)
+{
+	u32 lo, hi;
+
+	if (!boot_cpu_has_bug(X86_BUG_AMD_APIC_C1E))
+		return;
+
+	/*
+	 * AMD E400 detection needs to happen *after* ACPI has been enabled. We
+	 * check for C1E active in the interrupt pending message MSR. If we detect
+	 * C1E enabled, machine is affected.
+	 */
+	rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
+
+	if (!(lo & K8_INTP_C1E_ACTIVE_MASK)) {
+		setup_clear_cpu_bug(X86_BUG_AMD_APIC_C1E);
+		return;
+	}
+
+	if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
+		mark_tsc_unstable("TSC halt in AMD C1E");
+	pr_info("System has AMD C1E enabled\n");
+}
+
 static int __init idle_setup(char *str)
 {
 	if (!str)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 56190d00fd87..da42fe354ffc 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -34,6 +34,7 @@
 #include <linux/reboot.h>
 #include <linux/delay.h>
 #ifdef CONFIG_X86
+#include <asm/acpi.h>
 #include <asm/mpspec.h>
 #endif
 #include <linux/acpi_iort.h>
@@ -1166,6 +1167,13 @@ static int __init acpi_bus_init(void)
 struct kobject *acpi_kobj;
 EXPORT_SYMBOL_GPL(acpi_kobj);
 
+/*
+ * Arch-specific one to override this.
+ */
+void __init __weak arch_post_acpi_init(void)
+{
+}
+
 static int __init acpi_init(void)
 {
 	int result;
@@ -1198,6 +1206,9 @@ static int __init acpi_init(void)
 	acpi_debugger_init();
 	acpi_setup_sb_notify_handler();
 	acpi_set_processor_mapping();
+
+	arch_post_acpi_init();
+
 	return 0;
 }
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 2237d3f24f0e..8eec3255d6f2 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -141,7 +141,7 @@ static void lapic_timer_check_state(int state, struct acpi_processor *pr,
 	if (cpu_has(&cpu_data(pr->id), X86_FEATURE_ARAT))
 		return;
 
-	if (amd_e400_c1e_detected)
+	if (boot_cpu_has(X86_BUG_AMD_APIC_C1E))
 		type = ACPI_STATE_C1;
 
 	/*

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [next] | [standalone]

#1532316

On Tue, Nov 29, 2016 at 02:16:49PM +0100, Borislav Petkov wrote:
> On Mon, Nov 21, 2016 at 05:06:54PM +0100, Borislav Petkov wrote:
> > IOW, what's the worst thing that can happen if we did this below?
> > 
> > We basically get rid of the detection and switch the timer to broadcast
> > mode immediately on the halting CPU.
> > 
> > amd_e400_idle() is behind an "if (cpu_has_bug(c, X86_BUG_AMD_APIC_C1E))"
> > check so it will run on the affected CPUs only...
> > 
> > Thoughts?
> 
> Actually, here's a better version. The E400 detection works only after
> ACPI has been enabled so we piggyback the end of acpi_init().
> 
> We don't need the MSR read now - we do
> 
> 	if (static_cpu_has_bug(X86_BUG_AMD_APIC_C1E))
> 
> on the idle path which is as fast as it gets.
> 
> Any complaints about this before I go and test it everywhere?
> 
> It builds and boots in my guest here ok, not that it means a whole lot.
> 
> The good news is, I have collected a fleet of boxes which all have that
> erratum so testing should be pretty reliable. Something that doesn't
> happen everytime!

+1 one test server over here.. will check ;-)

jirka

[toc] | [prev] | [next] | [standalone]

#1532322

On Tue, 29 Nov 2016, Borislav Petkov wrote:
> On Mon, Nov 21, 2016 at 05:06:54PM +0100, Borislav Petkov wrote:
> > IOW, what's the worst thing that can happen if we did this below?
> > 
> > We basically get rid of the detection and switch the timer to broadcast
> > mode immediately on the halting CPU.
> > 
> > amd_e400_idle() is behind an "if (cpu_has_bug(c, X86_BUG_AMD_APIC_C1E))"
> > check so it will run on the affected CPUs only...
> > 
> > Thoughts?
> 
> Actually, here's a better version. The E400 detection works only after
> ACPI has been enabled so we piggyback the end of acpi_init().
> 
> We don't need the MSR read now - we do
> 
> 	if (static_cpu_has_bug(X86_BUG_AMD_APIC_C1E))
> 
> on the idle path which is as fast as it gets.
> 
> Any complaints about this before I go and test it everywhere?

The issue is that you obvioulsy start with the assumption, that the machine
has this bug. As a consequence the machine is brute forced into tick
broadcast mode, which cannot be reverted when you clear that misfeature
after ACPI init. So in case of !NOHZ and !HIGHRES the periodic tick is
forced into broadcast mode, which is not what you want.

As far as I understood the whole magic, this C1E misfeature takes only
effect _after_ ACPI has been initialized. So instead of setting the bug in
early boot and therefor forcing the broadcast nonsense, we should only set
it when ACPI has actually detected it.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1533070

On Tue, Nov 29, 2016 at 02:59:01PM +0100, Thomas Gleixner wrote:
> The issue is that you obvioulsy start with the assumption, that the machine
> has this bug. As a consequence the machine is brute forced into tick
> broadcast mode, which cannot be reverted when you clear that misfeature
> after ACPI init. So in case of !NOHZ and !HIGHRES the periodic tick is
> forced into broadcast mode, which is not what you want.
> 
> As far as I understood the whole magic, this C1E misfeature takes only
> effect _after_ ACPI has been initialized. So instead of setting the bug in
> early boot and therefor forcing the broadcast nonsense, we should only set
> it when ACPI has actually detected it.

Problem is, select_idle_routine() runs a lot earlier than acpi_init() so
there's a window where we don't definitively know yet whether the box is
actually going to enter C1E or not.

  [ I presume the reason why we have to do the proper detection after
    ACPI has been initialized is because the frickelware decides whether
    to do C1E entry or not and then sets those bits in the MSR (or not). ]

If in that window we enter idle and we're on an affected machine and we
*don't* switch to broadcast mode, we risk not waking up from C1E, i.e.,
the main reason this fix was even done.

So, if we "prematurely" switch to broadcast mode on the affected CPUs,
we're ok, it will be detected properly later and we're in broadcast
mode already.

Now, on those machines which are not affected and we clear
X86_BUG_AMD_APIC_C1E because they don't enter C1E at all, I was thinking
of maybe doing amd_e400_remove_cpu() and clearing that e400 mask and
even freeing it so that they can do default_idle().

But you're saying tick_broadcast_enter() is irreversible?

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1533079

On Wed, 30 Nov 2016, Borislav Petkov wrote:
> On Tue, Nov 29, 2016 at 02:59:01PM +0100, Thomas Gleixner wrote:
> > The issue is that you obvioulsy start with the assumption, that the machine
> > has this bug. As a consequence the machine is brute forced into tick
> > broadcast mode, which cannot be reverted when you clear that misfeature
> > after ACPI init. So in case of !NOHZ and !HIGHRES the periodic tick is
> > forced into broadcast mode, which is not what you want.
> > 
> > As far as I understood the whole magic, this C1E misfeature takes only
> > effect _after_ ACPI has been initialized. So instead of setting the bug in
> > early boot and therefor forcing the broadcast nonsense, we should only set
> > it when ACPI has actually detected it.
> 
> Problem is, select_idle_routine() runs a lot earlier than acpi_init() so
> there's a window where we don't definitively know yet whether the box is
> actually going to enter C1E or not.
> 
>   [ I presume the reason why we have to do the proper detection after
>     ACPI has been initialized is because the frickelware decides whether
>     to do C1E entry or not and then sets those bits in the MSR (or not). ]
> 
> If in that window we enter idle and we're on an affected machine and we
> *don't* switch to broadcast mode, we risk not waking up from C1E, i.e.,
> the main reason this fix was even done.
> 
> So, if we "prematurely" switch to broadcast mode on the affected CPUs,
> we're ok, it will be detected properly later and we're in broadcast
> mode already.

Right, that's the safe bet. But I'm quite sure that the C1E crap only
starts to work _after_ ACPI initialization.

> Now, on those machines which are not affected and we clear
> X86_BUG_AMD_APIC_C1E because they don't enter C1E at all, I was thinking
> of maybe doing amd_e400_remove_cpu() and clearing that e400 mask and
> even freeing it so that they can do default_idle().
> 
> But you're saying tick_broadcast_enter() is irreversible?

tick_force_broadcast() is irreversible

Thanks,

	tglx
	

[toc] | [prev] | [next] | [standalone]

#1533091

On Wed, Nov 30, 2016 at 09:54:58AM +0100, Thomas Gleixner wrote:
> Right, that's the safe bet. But I'm quite sure that the C1E crap only
> starts to work _after_ ACPI initialization.

Yap, I think it is an ACPI decision whether to enter C1E or not. And all
those boxes which are unaffected - they actually are but since the FW
doesn't enter C1E, they don't hit the bug.

> tick_force_broadcast() is irreversible

So if I let the cores in that small window use amd_e400_idle() and
then when I detect the machine doesn't enter C1E after all, I do
tick_broadcast_exit() on all cores in amd_e400_c1e_mask, then clear it
and free it, that would work?

Or do you see a better solution?

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1533099

On Wed, 30 Nov 2016, Borislav Petkov wrote:
> On Wed, Nov 30, 2016 at 09:54:58AM +0100, Thomas Gleixner wrote:
> > Right, that's the safe bet. But I'm quite sure that the C1E crap only
> > starts to work _after_ ACPI initialization.
> 
> Yap, I think it is an ACPI decision whether to enter C1E or not. And all
> those boxes which are unaffected - they actually are but since the FW
> doesn't enter C1E, they don't hit the bug.
> 
> > tick_force_broadcast() is irreversible
> 
> So if I let the cores in that small window use amd_e400_idle() and
> then when I detect the machine doesn't enter C1E after all, I do
> tick_broadcast_exit() on all cores in amd_e400_c1e_mask, then clear it
> and free it, that would work?
> 
> Or do you see a better solution?

Start with the bug cleared and select amd_e400_idle(), which will then use
default_idle() and not do any of the broadcast crap.

After ACPI gets initialized check the C1E misfeature and if it's detected,
then set the CPU BUG and send an IPI to all online CPUs.

I think this is safe because the CPU running the ACPI initialization is not
in idle and if any of the other CPUs would be stuck due to the C1E
enablement, then the IPI will kick them out.

At least worth a try.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web