Re: [PATCH v5 0/3] x86/mce: Fix timer list corruption and avoid redundant polling

[Aaron Tomlin]

On Fri, Sep 04, 2026 at 02:51:42PM -0700, Luck, Tony wrote:
> On Fri, Sep 04, 2026 at 09:54:18AM -0400, Aaron Tomlin wrote:
> > An elegant solution is to protect the storm transitions in both
> > cmci_storm_begin() and cmci_storm_end() using local_irq_save() and
> > local_irq_restore(). This serialises the counter updates and timer kicks
> > against local hardirq preemption:
> > 
> > --- a/arch/x86/kernel/cpu/mce/threshold.c
> > +++ b/arch/x86/kernel/cpu/mce/threshold.c
> > @@ -85,29 +85,37 @@ 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;
> > 
> > +    local_irq_save(flags);
> >      set_bit(bank, this_cpu_ptr(mce_poll_banks));
> >      storm->banks[bank].in_storm_mode = true;
> > 
> >      /*
> >       * If this is the first bank on this CPU to enter storm mode
> >       * start polling.
> >       */
> >      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));
> >      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);
> >  }
> 
> I ran this past an internal AI, and it said there was still a race in
> mce_track_storm(). This test:
> 
> 	if (storm->banks[mce->bank].in_storm_mode) {
> 
> is made with interrupts enabled, so another CMCI immediately after
> picking which of the if/else paths to take could change the value of
> in_storm_mode which then leads to corruption of the storm state machine.
> 
> > Now, regarding the second report, Sashiko appears to be correct. During
> > early boot, acpi_hest_init() -> mce_disable_bank() broadcasts via
> > on_each_cpu() to clear Firmware First banks from mce_poll_banks, but CPUs
> > that are brought online late or physically hotplugged miss this broadcast.
> > 
> > When those CPUs come online, cmci_skip_bank() currently bails out early
> > without clearing mce_poll_banks:
> > 
> >         /* Skip banks in firmware first mode */
> >         if (test_bit(bank, mce_banks_ce_disabled))
> >             return true;
> > 
> > Because mce_poll_banks is statically initialised to ~0UL, the bit remains
> > set, defeating bitmap_empty() on hotplugged CPUs and causing mce_timer_fn()
> > to periodically poll and clear Firmware First status registers.
> > 
> > Clearing the bit in cmci_skip_bank() resolves this cleanly:
> > 
> > --- a/arch/x86/kernel/cpu/mce/intel.c
> > +++ b/arch/x86/kernel/cpu/mce/intel.c
> > @@ -181,8 +181,10 @@ static bool cmci_skip_bank(int bank, u64 *val)
> >      if (test_bit(bank, owned))
> >          return true;
> > 
> >      /* Skip banks in firmware first mode */
> > -    if (test_bit(bank, mce_banks_ce_disabled))
> > +    if (test_bit(bank, mce_banks_ce_disabled)) {
> > +        clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> >          return true;
> > +    }
> 
> This looks right.
> 
> >      rdmsrq(MSR_IA32_MCx_CTL2(bank), *val);
> > 
> > If you are happy with these two changes, I will fold the local_irq_save()
> > fix into Patch 2/3 and the cmci_skip_bank() fix into Patch 3/3 for v6.
> > 
> 
> I don't think these fixes should be folded into existing patches in this
> series. They are distinct changes fixing specific long standing issues.
> They deserve their own patches under the "one change per patch" doctrine.
> 
> Also we still have:
> 
> static void __mce_disable_bank(void *arg)
> {
>         int bank = *((int *)arg);
>         __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
>         cmci_disable_bank(bank);
> }
> 
> That should switch over to the atomic clear_bank() or there should
> be a comment on why non-atomic is OK here and bad everywhere else.
> 
> -Tony

Hi Tony,

Thank you and apologies for the delay.

Yes, you are correct about mce_track_storm(). Since mce_track_storm() can
be called from timer softirq context with interrupts enabled, a CMCI
hardirq on the same CPU can re-entrantly execute mce_track_storm(), racing
on in_storm_mode and corrupting the bank's history and storm state.

Rather than only protecting the helper functions cmci_storm_begin() and
cmci_storm_end(), the proper solution is to enclose mce_track_storm() with
local_irq_save() and local_irq_restore(). This serialises the entire storm
state evaluation, counter transitions, and timer kicks against hardirq
preemption.

I also agree with the "one change per patch" doctrine. As such:
    1.  I will split the firmware-first cmci_skip_bank() fix into its own
        separate patch.

    2.  I will convert __mce_disable_bank() to atomic clear_bit() as part
        of the atomic bitops/concurrency fix so that mce_poll_banks updates
        are uniformly atomic throughout the subsystem.

    3.  The final patch will remain solely focused on the core polling
        optimisation (non-CMCI banks, bitmap_empty, and isolation checks).

I will prepare and send version 6 structured as a 4-patch series
accordingly.

-- 
Aaron Tomlin