[PATCH v4] lockdep: Fix wait context check on softirq for PREEMPT_RT

[PATCH v4] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Boqun Feng]

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
---
v3 -> v4:

*	Move the dummy lockdep_map (for detection) to PREEMPT_RT only.

v3: https://lore.kernel.org/r/20250118054900.18639-1-ryotkkr98@gmail.com

I would need a tag from RT to send it for v6.16, please take a look,
thanks!

 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.39.5 (Apple Git-154)

Re: [PATCH v4] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ingo Molnar]

* Boqun Feng <boqun.feng@gmail.com> wrote:

> 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

That's a weird SOB chain. Following back the history of the submission 
I believe this line went missing:

  From: Ryo Takakura <ryotkkr98@gmail.com>

I added it back in to the commit.

Thanks,

	Ingo

[tip: locking/urgent] lockdep: Fix wait context check on softirq for PREEMPT_RT

[tip-bot2 for Ryo Takakura]

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     61c39d8c83e2077f33e0a2c8980a76a7f323f0ce
Gitweb:        https://git.kernel.org/tip/61c39d8c83e2077f33e0a2c8980a76a7f323f0ce
Author:        Ryo Takakura <ryotkkr98@gmail.com>
AuthorDate:    Fri, 21 Mar 2025 07:33:22 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 25 Mar 2025 10:46:44 +01:00

lockdep: Fix wait context check on softirq for PREEMPT_RT

Since:

  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:

    | wait context tests |
    --------------------------------------------------------------------------
                                   | rcu  | raw  | spin |mutex |
    --------------------------------------------------------------------------
                 in hardirq context:  ok  |  ok  |  ok  |  ok  |
  in hardirq context (not threaded):  ok  |  ok  |  ok  |  ok  |
                 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 and Peter Zijlstra, and make bh_lock_map
	 only exist for PREEMPT_RT. ]

[ mingo: Restored authorship and improved the bh_lock_map definition. ]

Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20250321143322.79651-1-boqun.feng@gmail.com
---
 kernel/softirq.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 4dae6ac..513b194 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();

Re: [PATCH v4] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Boqun Feng]

On Tue, Mar 25, 2025 at 10:42:36AM +0100, Ingo Molnar wrote:
> 
> * Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > 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
> 
> That's a weird SOB chain. Following back the history of the submission 
> I believe this line went missing:
> 
>   From: Ryo Takakura <ryotkkr98@gmail.com>
> 
> I added it back in to the commit.
> 

Thanks! Looks like I lost the "From:" field when I post the draft of v4
at:

	https://lore.kernel.org/lkml/Z8t8imzJVhWyDvhC@boqun-archlinux/

I must re-apply that email as a patch to my branch, hence the "From:"
field got changed. Sorry for the mistakes.

Regards,
Boqun

> Thanks,
> 
> 	Ingo

Re: [PATCH v4] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ingo Molnar]

* Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Mar 25, 2025 at 10:42:36AM +0100, Ingo Molnar wrote:
> > 
> > * Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> > > 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
> > 
> > That's a weird SOB chain. Following back the history of the submission 
> > I believe this line went missing:
> > 
> >   From: Ryo Takakura <ryotkkr98@gmail.com>
> > 
> > I added it back in to the commit.
> > 
> 
> Thanks! Looks like I lost the "From:" field when I post the draft of v4
> at:
> 
> 	https://lore.kernel.org/lkml/Z8t8imzJVhWyDvhC@boqun-archlinux/
> 
> I must re-apply that email as a patch to my branch, hence the "From:"
> field got changed. Sorry for the mistakes.

No worries - sometimes when rebasing with a conflict or applying with a 
conflict, Git can drop authorship without much of a warning - I've ran 
into that myself - so it happens.

Thanks,

	Ingo