[PATCH v6 2/4] x86/mce/threshold: Fix concurrency races in storm tracking

[Aaron Tomlin]

Functions cmci_storm_begin(), cmci_storm_end(), and __mce_disable_bank()
modify the per-CPU bitmap mce_poll_banks using non-atomic __set_bit()
and __clear_bit(). In addition, mce_track_storm(), cmci_storm_begin(),
and cmci_storm_end() manipulate per-CPU storm descriptors and invoke
mce_timer_kick() without synchronisation against local interrupts.

While mce_poll_banks and storm_desc are per-CPU, mce_track_storm() and
cmci_storm_end() execute in timer softirq context (via mce_timer_fn())
with local hardirqs enabled, whereas cmci_storm_begin() can be invoked
from CMCI hardirq context (via intel_threshold_interrupt()).

This introduces multiple concurrency races between softirq and hardirq
contexts on the same CPU:
    1.  A hardirq interrupting a softirq's non-atomic read-modify-write
        on mce_poll_banks will have its bit update clobbered when the
        softirq resumes, dropping a stormy bank from polling.

    2.  An incoming CMCI hardirq during mce_track_storm() re-entrantly
        executes mce_track_storm() on the same CPU, racing on
        in_storm_mode, history, and timestamps, which can corrupt the
        bank's storm state machine.

    3.  If a hardirq fires after cmci_storm_end() decrements
        stormy_bank_count to zero, but before mce_timer_kick(false) is
        called, the hardirq's cmci_storm_begin() will increment
        stormy_bank_count to 1 and call mce_timer_kick(true). When the
        softirq resumes, its delayed mce_timer_kick(false) will
        erroneously override storm mode, leaving the CPU with
        stormy_bank_count == 1 whilst the timer reverts to the default
        polling interval.

Resolve these issues by switching to atomic set_bit() and clear_bit()
operations on mce_poll_banks and __mce_disable_bank(). Finally, enclose
mce_track_storm(), cmci_storm_begin(), and cmci_storm_end() with
local_irq_save() and local_irq_restore().

Fixes: 7eae17c4add5 ("x86/mce: Add per-bank CMCI storm mitigation")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxxx>
---
 arch/x86/kernel/cpu/mce/core.c      |  2 +-
 arch/x86/kernel/cpu/mce/threshold.c | 23 +++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 765e8103b0d2..aa604d981358 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2294,7 +2294,7 @@ void mcheck_cpu_clear(struct cpuinfo_x86 *c)
 static void __mce_disable_bank(void *arg)
 {
 	int bank = *((int *)arg);
-	__clear_bit(bank, this_cpu_ptr(mce_poll_banks));
+	clear_bit(bank, this_cpu_ptr(mce_poll_banks));
 	cmci_disable_bank(bank);
 }
 
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index 6c370d5af5bd..83f3e2250ae2 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -85,8 +85,10 @@ static void mce_handle_storm(unsigned int bank, bool on)
 void cmci_storm_begin(unsigned int bank)
 {
 	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
+	unsigned long flags;
 
-	__set_bit(bank, this_cpu_ptr(mce_poll_banks));
+	local_irq_save(flags);
+	set_bit(bank, this_cpu_ptr(mce_poll_banks));
 	storm->banks[bank].in_storm_mode = true;
 
 	/*
@@ -95,32 +97,38 @@ void cmci_storm_begin(unsigned int bank)
 	 */
 	if (++storm->stormy_bank_count == 1)
 		mce_timer_kick(true);
+	local_irq_restore(flags);
 }
 
 void cmci_storm_end(unsigned int bank)
 {
 	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
+	unsigned long flags;
 
+	local_irq_save(flags);
 	if (!mce_flags.amd_threshold)
-		__clear_bit(bank, this_cpu_ptr(mce_poll_banks));
+		clear_bit(bank, this_cpu_ptr(mce_poll_banks));
 	storm->banks[bank].history = 0;
 	storm->banks[bank].in_storm_mode = false;
 
 	/* If no banks left in storm mode, stop polling. */
 	if (!--storm->stormy_bank_count)
 		mce_timer_kick(false);
+	local_irq_restore(flags);
 }
 
 void mce_track_storm(struct mce *mce)
 {
 	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
-	unsigned long now = jiffies, delta;
+	unsigned long flags, now = jiffies, delta;
 	unsigned int shift = 1;
 	u64 history = 0;
 
+	local_irq_save(flags);
+
 	/* No tracking needed for banks that do not support CMCI */
 	if (storm->banks[mce->bank].poll_only)
-		return;
+		goto out;
 
 	/*
 	 * When a bank is in storm mode it is polled once per second and
@@ -149,15 +157,18 @@ void mce_track_storm(struct mce *mce)
 
 	if (storm->banks[mce->bank].in_storm_mode) {
 		if (history & GENMASK_ULL(STORM_END_POLL_THRESHOLD, 0))
-			return;
+			goto out;
 		printk_deferred(KERN_NOTICE "CPU%d BANK%d CMCI storm subsided\n", smp_processor_id(), mce->bank);
 		mce_handle_storm(mce->bank, false);
 		cmci_storm_end(mce->bank);
 	} else {
 		if (hweight64(history) < STORM_BEGIN_THRESHOLD)
-			return;
+			goto out;
 		printk_deferred(KERN_NOTICE "CPU%d BANK%d CMCI storm detected\n", smp_processor_id(), mce->bank);
 		mce_handle_storm(mce->bank, true);
 		cmci_storm_begin(mce->bank);
 	}
+
+out:
+	local_irq_restore(flags);
 }
-- 
2.55.0