| 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
| ||
|
Message-Id: <20250308080951.345854-1-ryotkkr98@gmail.com>
Date: Sat, 8 Mar 2025 17:09:51 +0900
From: Ryo Takakura <ryotkkr98@...il.com>
To: boqun.feng@...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,
mingo@...nel.org,
netdev@...r.kernel.org,
pabeni@...hat.com,
peterz@...radead.org,
ryotkkr98@...il.com,
x86@...nel.org
Subject: 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:
>> 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
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().
>> > > 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
>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@...il.com>
>Signed-off-by: Boqun Feng <boqun.feng@...il.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
Powered by blists - more mailing lists