Re: [PATCH v2] interrupt: Disable interrupt before modifying hardirq_disable counter

[lyude]

Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>

On Sat, 2026-08-29 at 14:34 -0700, Boqun Feng wrote:
> Currently a softirq may be pending longer then expected if the
> triggering interrupt happens in-between hardirq_disable_enter() and
> _local_interrupt_disable() in local_interrupt_disable():
> 
>     local_interrupt_disable():
>       hardirq_disable_enter();
>       <interrupt>
>       ...
>       __irq_exit_rcu():
>         // false because hardirq_disable_count() is not 0
>         if (.. && !hardirq_disable_count() && ..) {
> 	  invoke_softirq();
> 	}
>       _local_interrupt_disable();
> 
> , it'll defer the softirq to the next interrupt which can be forever.
> 
> The order between hardirq_disable_enter() and
> _local_interrupt_disable()
> is to optimize re-disabling interrupts if they are already disabled,
> but
> as 1) local_interrupt_disable() is not widely used yet and 2) the
> proper
> way to achieve this optimization may need fixing up the counter at
> entry/exit time [1], so reverse the order for now to avoid the
> softirq
> pending issue.
> 
> Because of this fix, the part of saving the current state is
> separated
> from irq disabling, and the logic of local_interrupt_disable()
> becomes:
> 
>     local_irq_save(flags);
>     if (counter++ == 0) {
>       this_cpu(local_interrupt_disable_state) = flags;
>     }
> 
> Therefore change the helper function _local_interrupt_disable() to
> _local_interrupt_save_state() which only saves the current irqflags
> (when interrupts get disabled the first time).
> 
> Link: https://lore.kernel.org/lkml/87v78wezid.ffs@fw13/ [1]
> Reported-by: Thomas Gleixner <tglx@xxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/87jypbfu1t.ffs@fw13/
> Fixes: e901c1510e24 ("irq,spin_lock: Add counted interrupt
> disabling/enabling")
> Reviewed-by: Bradley Morgan <brads@xxxxxxxxxxxxxx>
> Signed-off-by: Boqun Feng <boqun@xxxxxxxxxx>
> ---
> v1 -> v2:
> 
> * Use imperative mood in the last paragraph of the change log.
> * Add "Closes" tag to the email of the explanation of the issue.
> * Apply the RoB tag from Bradley Morgan
> 
>  include/linux/interrupt_rc.h | 19 ++++++++-----------
>  kernel/softirq.c             | 17 ++++-------------
>  2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/interrupt_rc.h
> b/include/linux/interrupt_rc.h
> index b9a7f05ecf42..e68e1bedba66 100644
> --- a/include/linux/interrupt_rc.h
> +++ b/include/linux/interrupt_rc.h
> @@ -20,11 +20,8 @@
>  /* Per-CPU interrupt disabling state for
> local_interrupt_{disable,enable}(). */
>  DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
>  
> -static __always_inline void __local_interrupt_disable(void)
> +static __always_inline void __local_interrupt_save_state(unsigned
> long flags)
>  {
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
>  	raw_cpu_write(local_interrupt_disable_state, flags);
>  }
>  
> @@ -36,9 +33,9 @@ static __always_inline void
> __local_interrupt_enable(void)
>  }
>  
>  #ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
> -static __always_inline void _local_interrupt_disable(void)
> +static __always_inline void _local_interrupt_save_state(unsigned
> long flags)
>  {
> -	__local_interrupt_disable();
> +	__local_interrupt_save_state(flags);
>  }
>  
>  static __always_inline void _local_interrupt_enable(void)
> @@ -46,27 +43,27 @@ static __always_inline void
> _local_interrupt_enable(void)
>  	__local_interrupt_enable();
>  }
>  #else
> -extern void _local_interrupt_disable(void);
> +extern void _local_interrupt_save_state(unsigned long flags);
>  extern void _local_interrupt_enable(void);
>  #endif
>  
>  #else /* !MODULE */
> -extern void _local_interrupt_disable(void);
> +extern void _local_interrupt_save_state(unsigned long flags);
>  extern void _local_interrupt_enable(void);
>  #endif /* !MODULE */
>  
>  static inline void local_interrupt_disable(void)
>  {
>  	int new_count;
> +	unsigned long flags;
>  
>  	WARN_ON_ONCE(in_nmi());
>  
> +	local_irq_save(flags);
>  	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();
> +		_local_interrupt_save_state(flags);
>  }
>  
>  static inline void local_interrupt_enable(void)
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 7980a4a232f9..5d02c36c40e3 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -91,11 +91,11 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
>  
>  DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
>  
> -void _local_interrupt_disable(void)
> +void _local_interrupt_save_state(unsigned long flags)
>  {
> -	__local_interrupt_disable();
> +	__local_interrupt_save_state(flags);
>  }
> -EXPORT_SYMBOL(_local_interrupt_disable);
> +EXPORT_SYMBOL(_local_interrupt_save_state);
>  
>  void _local_interrupt_enable(void)
>  {
> @@ -749,16 +749,7 @@ static inline void __irq_exit_rcu(void)
>  #endif
>  	account_hardirq_exit(current);
>  	preempt_count_sub(HARDIRQ_OFFSET);
> -	/*
> -	 * Interrupts may happen between hardirq_disable_enter() and
> -	 * local_irq_save() in local_interrupt_disable(), if
> irq_exit() invokes
> -	 * softirq here, we may have a softirq handler calling
> -	 * local_interrupt_disable() but it won't disable the IRQ
> because
> -	 * hardirq disabling count is already 1, hence we need to
> prevent
> -	 * invoking softirq when a local_interrupt_disable() is
> ongoing.
> -	 */
> -	if (!in_interrupt() && !hardirq_disable_count() &&
> -	    local_softirq_pending()) {
> +	if (!in_interrupt() && local_softirq_pending()) {
>  		/*
>  		 * If we left hrtimers unarmed, make sure to arm
> them now,
>  		 * before enabling interrupts to run softirq.