linux-kernel - Re: request_irq() with local bh disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8tJAJKQP3gtF7EY@boqun-archlinux>
Date: Fri, 7 Mar 2025 11:29:04 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Ryo Takakura <ryotkkr98@...il.com>
Cc: bp@...en8.de, davem@...emloft.net, edumazet@...gle.com,
	horms@...nel.org, kuba@...nel.org, kuniyu@...zon.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	pabeni@...hat.com, peterz@...radead.org, x86@...nel.org
Subject: Re: request_irq() with local bh disabled

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();
> >