Re: request_irq() with local bh disabled

Hi Boqun,

Thanks for looking into it.

On Fri, 7 Mar 2025 15:08:58 -0800, Boqun Feng wrote:

On Fri, Mar 07, 2025 at 11:29:04AM -0800, Boqun Feng wrote:

quoted

On Fri, Mar 07, 2025 at 10:33:36AM -0800, Boqun Feng wrote:

quoted

On Fri, Mar 07, 2025 at 07:57:40AM -0800, Boqun Feng wrote:

quoted

On Fri, Mar 07, 2025 at 10:39:46PM +0900, Ryo Takakura wrote:

quoted

Hi Boris,

On Fri, 7 Mar 2025 14:13:19 +0100, Borislav Petkov wrote:

quoted

On Fri, Mar 07, 2025 at 09:58:51PM +0900, Ryo Takakura wrote:

quoted

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

I see! I wasn't aware of !PREEMPT_RT differing from PREEMPT_RT 
where spin_unlock_bh() calls __local_bh_disable_ip() instead of
local_bh_disable().

quoted

quoted

quoted

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).

Sounds good to me.

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.

I see that __local_bh_enable_ip() and ksoftirq_run_end()
are the only call sites of __local_bh_enable() on PREEMPT_RT,  
so this looks good to me as well.

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?

Sounds good, Thanks!

Sincerely,
Ryo Takakura