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: <Z8sXdDFJTjYbpAcq@tardis>
Date: Fri, 7 Mar 2025 07:57:40 -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: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);
+
 	WARN_ON_ONCE(in_hardirq());
 	lockdep_assert_irqs_enabled();