Re: request_irq() with local bh disabled

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

On Fri, Mar 07, 2025 at 10:33:36AM -0800, Boqun Feng wrote:
> 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().
> 

Hmm.. it's a bit complicated than that because __local_bh_enable() is
called twice. We need to remain the lock_map_release() in
__local_bh_enable_ip(), remove the lock_map_release() and add another
one in ksoftirq_run_end().

Let me think and test more on this.

Regards,
Boqun

> Regards,
> Boqun
> 
> >  	WARN_ON_ONCE(in_hardirq());
> >  	lockdep_assert_irqs_enabled();
> >