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

[Aaron Tomlin]

On Fri, Sep 11, 2026 at 04:12:04PM -0400, Aaron Tomlin wrote:
> 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);
>  }
>  

Hi Tony,

This was an artifact. Sorry about that.

Indeed, the atomic bit operations became completely redundant the moment
local_irq_save() was introduced to enclose mce_track_storm(); the use of
non-atomic __clear_bit(bank, this_cpu_ptr(mce_poll_banks)) is fine.

    mce_track_storm
    {
      local_irq_save(flags)

      if (storm->banks[mce->bank].in_storm_mode)

        cmci_storm_end(mce->bank)
        {

          local_irq_save(flags)

          if (!mce_flags.amd_threshold)
              clear_bit(bank, this_cpu_ptr(mce_poll_banks))


> 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
> 

-- 
Aaron Tomlin