Re: request_irq() with local bh disabled

[Boqun Feng <boqun.feng@gmail.com>]

On Fri, Mar 07, 2025 at 07:57:40AM -0800, Boqun Feng wrote:
> On Fri, Mar 07, 2025 at 10:39:46PM +0900, Ryo Takakura wrote:
> > Hi Boris,
> > 
> > On Fri, 7 Mar 2025 14:13:19 +0100, Borislav Petkov wrote:
> > >On Fri, Mar 07, 2025 at 09:58:51PM +0900, Ryo Takakura wrote:
> > >> I'm so sorry that the commit caused this problem...
> > >> Please let me know if there is anything that I should do.
> > >
> > >It is gone from the tip tree so you can take your time and try to do it right.
> > >
> > >Peter and/or I could help you reproduce the issue and try to figure out what
> > >needs to change there.
> > >
> > >HTH.
> > 
> > Thank you so much for this. I really appreciate it.
> > I'll once again take a look and try to fix the problem.
> > 
> 
> Looks like we missed cases where
> 
> acquire the lock:
> 
> 	netif_addr_lock_bh():
> 	  local_bh_disable();
> 	  spin_lock_nested();
> 
> release the lock:
> 
> 	netif_addr_unlock_bh():
> 	  spin_unlock_bh(); // <- calling __local_bh_disable_ip() directly
> 
> means we should do the following on top of your changes.
> 
> Regards,
> Boqun
> 
> ------------------->8
> diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
> index 0640a147becd..7553309cbed4 100644
> --- a/include/linux/bottom_half.h
> +++ b/include/linux/bottom_half.h
> @@ -22,7 +22,6 @@ extern struct lockdep_map bh_lock_map;
>  
>  static inline void local_bh_disable(void)
>  {
> -	lock_map_acquire_read(&bh_lock_map);
>  	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>  }
>  
> @@ -31,13 +30,11 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
>  
>  static inline void local_bh_enable_ip(unsigned long ip)
>  {
> -	lock_map_release(&bh_lock_map);
>  	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
>  }
>  
>  static inline void local_bh_enable(void)
>  {
> -	lock_map_release(&bh_lock_map);
>  	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>  }
>  
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index e864f9ce1dfe..782d5e9753f6 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -175,6 +175,8 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
>  		lockdep_softirqs_off(ip);
>  		raw_local_irq_restore(flags);
>  	}
> +
> +	lock_map_acquire_read(&bh_lock_map);
>  }
>  EXPORT_SYMBOL(__local_bh_disable_ip);
>  
> @@ -183,6 +185,8 @@ static void __local_bh_enable(unsigned int cnt, bool unlock)
>  	unsigned long flags;
>  	int newcnt;
>  
> +	lock_map_release(&bh_lock_map);
> +
>  	DEBUG_LOCKS_WARN_ON(current->softirq_disable_cnt !=
>  			    this_cpu_read(softirq_ctrl.cnt));
>  
> @@ -208,6 +212,8 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
>  	u32 pending;
>  	int curcnt;
>  
> +	lock_map_release(&bh_lock_map);
> +

Ok, this is not needed because __local_bh_enable() will be called by
__local_bh_enable_ip().

Regards,
Boqun

>  	WARN_ON_ONCE(in_hardirq());
>  	lockdep_assert_irqs_enabled();
>