Re: request_irq() with local bh disabled

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

On Fri, Mar 07, 2025 at 11:29:04AM -0800, Boqun Feng wrote:
> 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.
> 

So what I have came up so far is as follow:

1. I moved bh_lock_map to only for PREEMPT_RT (since for non-RT we have
   current softirq context tracking).
2. I moved lock_map_acquire_read() and lock_map_release() into
   PREEMPT_RT version of __local_bh_{disable,enable}_ip().
3. I added a lock_map_release() in ksoftirq_run_end() to release the
   conceptual bh_lock_map lock.

Let me know how you think about this. Given 2 & 3 needs some reviews
from PREEMPT_RT, and it's -rc5 already, so I'm going to postpone this
into 6.16 (I will resend this patch if it looks good to you). Sounds
good?

Regards,
Boqun
------------------------------------------------->8
Subject: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

Since commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting on
PREEMPT_RT."), the wait context test for mutex usage within
"in softirq context" fails as it references @softirq_context.

[    0.184549]   | wait context tests |
[    0.184549]   --------------------------------------------------------------------------
[    0.184549]                                  | rcu  | raw  | spin |mutex |
[    0.184549]   --------------------------------------------------------------------------
[    0.184550]                in hardirq context:  ok  |  ok  |  ok  |  ok  |
[    0.185083] in hardirq context (not threaded):  ok  |  ok  |  ok  |  ok  |
[    0.185606]                in softirq context:  ok  |  ok  |  ok  |FAILED|

As a fix, add lockdep map for BH disabled section. This fixes the
issue by letting us catch cases when local_bh_disable() gets called
with preemption disabled where local_lock doesn't get acquired.
In the case of "in softirq context" selftest, local_bh_disable() was
being called with preemption disable as it's early in the boot.

[boqun: Move the lockdep annotations into __local_bh_*() to avoid false
positives because of unpaired local_bh_disable() reported by Borislav
Petkov [1] and Peter Zijlstra [2], and make bh_lock_map only exist for
PREEMPT_RT]

Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://lore.kernel.org/all/20250306122413.GBZ8mT7Z61Tmgnh5Y9@fat_crate.local/ [1]
Link: https://lore.kernel.org/lkml/20250307113955.GK16878@noisy.programming.kicks-ass.net/ [2]
Link: https://lore.kernel.org/r/20250118054900.18639-1-ryotkkr98@gmail.com
---
 kernel/softirq.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 4dae6ac2e83f..3ce136bdcbfe 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -126,6 +126,18 @@ static DEFINE_PER_CPU(struct softirq_ctrl, softirq_ctrl) = {
 	.lock	= INIT_LOCAL_LOCK(softirq_ctrl.lock),
 };
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static struct lock_class_key bh_lock_key;
+struct lockdep_map bh_lock_map = {
+	.name = "local_bh",
+	.key = &bh_lock_key,
+	.wait_type_outer = LD_WAIT_FREE,
+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
+	.lock_type = LD_LOCK_PERCPU,
+};
+EXPORT_SYMBOL_GPL(bh_lock_map);
+#endif
+
 /**
  * local_bh_blocked() - Check for idle whether BH processing is blocked
  *
@@ -148,6 +160,8 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
 
 	WARN_ON_ONCE(in_hardirq());
 
+	lock_map_acquire_read(&bh_lock_map);
+
 	/* First entry of a task into a BH disabled section? */
 	if (!current->softirq_disable_cnt) {
 		if (preemptible()) {
@@ -211,6 +225,8 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
 	WARN_ON_ONCE(in_hardirq());
 	lockdep_assert_irqs_enabled();
 
+	lock_map_release(&bh_lock_map);
+
 	local_irq_save(flags);
 	curcnt = __this_cpu_read(softirq_ctrl.cnt);
 
@@ -261,6 +277,8 @@ static inline void ksoftirqd_run_begin(void)
 /* Counterpart to ksoftirqd_run_begin() */
 static inline void ksoftirqd_run_end(void)
 {
+	/* pairs with the lock_map_acquire_read() in ksoftirqd_run_begin() */
+	lock_map_release(&bh_lock_map);
 	__local_bh_enable(SOFTIRQ_OFFSET, true);
 	WARN_ON_ONCE(in_interrupt());
 	local_irq_enable();
-- 
2.47.1