linux-kernel - Re: [PATCH v2] lockdep: Fix wait context check on softirq for PREEMPT

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: <f3ca9b15-6365-4639-b93c-33f540448b66@redhat.com>
Date: Mon, 6 Jan 2025 14:13:57 -0500
From: Waiman Long <llong@...hat.com>
To: Ryo Takakura <ryotkkr98@...il.com>, bigeasy@...utronix.de,
 boqun.feng@...il.com, clrkwllms@...nel.org, mingo@...hat.com,
 peterz@...radead.org, rostedt@...dmis.org, tglx@...utronix.de,
 will@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v2] lockdep: Fix wait context check on softirq for
 PREEMPT_RT


On 12/26/24 11:08 PM, Ryo Takakura 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.
>
> Signed-off-by: Ryo Takakura <ryotkkr98@...il.com>
> ---
>
> Hi!
>
> This version fixes the issue by adding lockdep map for BH disabled
> section. Thanks Boqun for the suggestion!
>
> Changes since v1:
> https://lore.kernel.org/lkml/20241202012017.14910-1-ryotkkr98@gmail.com/
>
> - Fix the issue by adding lockdep map for BH disabled section.
>    This let us catch mutex usage within "in softirq context" selftest
>    as it disables BH section.
>
> Also, may I add Boqun as <suggested-by> tag?
>
> ---
>   include/linux/bottom_half.h |  8 ++++++++
>   kernel/softirq.c            | 12 ++++++++++++
>   2 files changed, 20 insertions(+)
>
> diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
> index fc53e0ad5..f77dc746c 100644
> --- a/include/linux/bottom_half.h
> +++ b/include/linux/bottom_half.h
> @@ -4,6 +4,7 @@
>   
>   #include <linux/instruction_pointer.h>
>   #include <linux/preempt.h>
> +#include <linux/lockdep.h>
>   
>   #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
>   extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
> @@ -15,8 +16,13 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
>   }
>   #endif
>   
> +#ifdef CONFIG_LOCKDEP
> +extern struct lockdep_map bh_lock_map;
> +#endif
> +
>   static inline void local_bh_disable(void)
>   {
> +	lock_map_acquire_read(&bh_lock_map);
>   	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>   }
>   
> @@ -25,11 +31,13 @@ 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 4dae6ac2e..a8211ba9a 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -1073,3 +1073,15 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
>   {
>   	return from;
>   }
> +
> +#ifdef CONFIG_LOCKDEP
> +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

All the locking code use CONFIG_DEBUG_LOCK_ALLOC to determine if 
lockdep_map should be used. Should you also use CONFIG_DEBUG_LOCK_ALLOC 
instead of CONFIG_LOCKDEP to be consistent with the others?

Cheers,
Longman