Re: [PATCH] locking: Revert switching guards to _irq_{disable,enable}()

[Thomas Gleixner <tglx@kernel.org>]

On Thu, Aug 27 2026 at 06:14, Boqun Feng wrote:
> On Thu, Aug 27, 2026 at 10:30:50AM +0200, Thomas Gleixner wrote:
>> The main problem is that you cover only half of it and there are
>> completely correct cases where this simply blows up in your face:
>> 
>>   local_irq_disable();                          // does not affect CNT
>>   ....
>>   guard(raw_spinlock)(&l1);                     // does not affect CNT
>>      foo()
>>        guard(raw_spinlock_irqsave)(&l2);        // observes CNT = 0
>> 
>> so the unlocking of &l2 will enable interrupts prematurely.
>> 
>
> If we are talking the switch in this patch, then no, the unlocking
> of &l2 will NOT enable interrupts prematurely. The above code expands as
> the following (using pseudo code to describe how
> raw_spin_lock_irq_disable(), raw_spin_lock_irq_enable(),
> local_interrupt_disable(), and local_interrupt_enable() work)
>
>    local_irq_disable();                          // does not affect CNT
>    ....
>    guard(raw_spinlock)(&l1);                     // does not affect CNT
>       foo()
>         guard(raw_spinlock_irqsave)(&l2): 
>           raw_spin_lock_irq_disable():
>             local_interrupt_disable():
> 	      CNT++;
>               this_cpu(state) = local_irq_save(); // record the current state
>             raw_spin_lock(&l2);
> 	  ...
>           raw_spin_lock_irq_enable():
>             raw_spin_unlock(&l2);
>             local_interrupt_enable():
>               CNT--;
>               if (CNT == 0)
>                 local_irq_restore(this_cpu(state)); // recover the previous state
>
> So local_interrupt_disable() and local_interrupt_enable() only recover
> to the previous state, as a result it'll not enable interrupt
> prematurely here. In other words, the following code works:
>
>     local_irq_disable();
>     local_interrupt_disable();
>     local_interrupt_enable();  // interrupt is not re-enabled here
>                                // similar to how preempt_disable() does
> 			       // in a nested preemption disable
> 			       // critical section.
>     local_irq_enable();

Fair enough. I misread that part.

But my main observation that the counter is inconsistent still stands
and I think that's a fundamental flaw because there is no way that code
can rely on that counter until everything has been converted over and
the interrupt/exception/nmi/syscall entry/exit code has been fixed up.

Just let me look at local_interrupt_disable() and __irq_exit_rcu()
again.

local_interrupt_disable()
   new_count = hardirq_disable_enter();

   /* Interrupts can happen here, but it's OK, see __irq_exit_rcu(). */

   if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
      _local_interrupt_disable();

This is absolutely not ok. Why?

The counter is incremented _before_ interrupts are actually
disabled. Now in __irq_exit_rcu():

        if (!in_interrupt() && !hardirq_disable_count() &&
            local_softirq_pending()) {

which prevents soft interrupt handling in a completely legitimate
situation. As a consequence _nothing_ will handle the pending soft
interrupt until:

     - an interrupt coming in which observes consistent state

     - a local_bh_enable() processes them

That explains that recently quite a few more spurious 'local soft irq
pending' printk's have been observed by people as there is no guarantee
that either one of those events happens _before_ a CPU reaches
idle. Even in the non-idle case deferring this to the next 'by chance'
handling is fundamentally broken. This needs to be removed ASAP.

But coming back to the problem underneath. The ordering in
local_interrupt_disable() is simply wrong. You need to disable first and
then update the counter. Reverse order for enable() obviously update
counter and enable, which you got right.

And to make stuff work correctly you need the fixups I pointed out in my
previous reply to the various entry/exit functions. It's exactly the
same problem as we handle in interrupt/exception/nmi entry/exit code
vs. RCU, lockdep, tracing etc.

So if disable() does:

   if (!count)
   	arch_local_irq_disable();
   count++;

then an interrupt hitting before arch_local_irq_disable() will always
observe the correct state. After that it can't be delivered.

Now with exceptions that's a different story because they can hit after
local_irq_disable() and before the count is incremented.

I thought some more about the state handling there and I think we can
avoid irq_state_t completely:

  irqentry_enter_from_kernel_mode()
     count++;
     ...

  irqentry_exit_to_kernel_mode_after_preempt()
     ...
     count--;

For a regular interrupt which hit _before_ disable() managed to disable
it at the CPU level, this will go from 0 -> 1 and on return from 1 -> 0.

For an exception which hits between disabling and incrementing the
counter this will go from 0 -> 1 as well, but there is nothing which can
be done about that and exception handlers need to consult regs->eflags
to figure out the state of the context they interrupted. If the
exception hits afterwards then it will set the correct state.  But it
does not matter in that case because everything there needs to do
irqsave() so interrupts can't be enabled accidentaly. The only exception
to that rule is the conditional enable:

   if (regs->eflags & X86_EFLAGS_IF)
   	local_irq_enable();

And for that to work correctly you want overall consistent counter
state. Otherwise your counter is just a random number generator.

With that fixed the disable race becomes:

disable()
     if (!count)

-> Interrupt before interrupts are disabled in the CPU.

  irqentry_enter_from_kernel_mode()
     count++;   // Correct state because the CPU disabled interrupts

  ...
  __irq_exit_rcu()
     if (!in_interrupt() && local_softirq_pending()) {
        handle_softirqs()
          ...
          local_irq_enable();                -> Count goes to 0
          ...
          guard(spinlock_irq)(&lock)
            local_interrupt_disable()
              // Observes count == 0
              if (!count)
                 arch_local_irq_disable();
     ...
     
  irqentry_exit_to_kernel_mode_after_preempt()
     ...
     count--;

And yes, this only works correctly when _all_ state is consistent. You
can't get it to work properly with half of it without creating hard to
debug problems.

Thanks,

        tglx