linux-kernel - Re: [PATCH] module: Do a WARN_ON_ONCE() for assert module mutex not held

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] [day] [month] [year] [list]

Date:	Mon, 18 Jul 2016 12:46:41 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jason Baron <jbaron@...hat.com>,
	"Peter Zijlstra" <peterz@...radead.org>,
	"Jessica Yu" <jeyu@...hat.com>
Subject: Re: [PATCH] module: Do a WARN_ON_ONCE() for assert module mutex not held

Steven Rostedt <rostedt@...dmis.org> writes:
> When running with lockdep enabled, I triggered the WARN_ON() in the
> module code that asserts when module_mutex or rcu_read_lock_sched are
> not held. The issue I have is that this can also be called from the
> dump_stack() code, causing us to enter an infinite loop...

Thanks, applied.

It would be good to see a proper stacktrace though.

Hmm, this caller looks like it might be unprotected:

arch_prepare_optimized_kprobe -> copy_optimized_instructions ->
jump_label_text_reserved -> __jump_label_mod_text_reserved ->
__module_text_address

It holds text_mutex, but preempt is still enabled AFAICT.

Does this help?
Rusty.

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 4b353e0be121..d636ce4af995 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -284,11 +284,14 @@ static int __jump_label_mod_text_reserved(void *start, void *end)
 {
 	struct module *mod;
 
+	preempt_disable();
 	mod = __module_text_address((unsigned long)start);
+	WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
+	preempt_enable();
+
 	if (!mod)
 		return 0;
 
-	WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
 
 	return __jump_label_text_reserved(mod->jump_entries,
 				mod->jump_entries + mod->num_jump_entries,

>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 0 at kernel/module.c:268 module_assert_mutex_or_preempt+0x3c/0x3e
>  Modules linked in: ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6
>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.7.0-rc3-test-00013-g501c2375253c #14
>  Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
>   ffff880215e8fa70 ffff880215e8fa70 ffffffff812fc8e3 0000000000000000
>   ffffffff81d3e55b ffff880215e8fac0 ffffffff8104fc88 ffffffff8104fcab
>   0000000915e88300 0000000000000046 ffffffffa019b29a 0000000000000001
>  Call Trace:
>   [<ffffffff812fc8e3>] dump_stack+0x67/0x90
>   [<ffffffff8104fc88>] __warn+0xcb/0xe9
>   [<ffffffff8104fcab>] ? warn_slowpath_null+0x5/0x1f
>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 0 at kernel/module.c:268 module_assert_mutex_or_preempt+0x3c/0x3e
>  Modules linked in: ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6
>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.7.0-rc3-test-00013-g501c2375253c #14
>  Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
>   ffff880215e8f7a0 ffff880215e8f7a0 ffffffff812fc8e3 0000000000000000
>   ffffffff81d3e55b ffff880215e8f7f0 ffffffff8104fc88 ffffffff8104fcab
>   0000000915e88300 0000000000000046 ffffffffa019b29a 0000000000000001
>  Call Trace:
>   [<ffffffff812fc8e3>] dump_stack+0x67/0x90
>   [<ffffffff8104fc88>] __warn+0xcb/0xe9
>   [<ffffffff8104fcab>] ? warn_slowpath_null+0x5/0x1f
>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 0 at kernel/module.c:268 module_assert_mutex_or_preempt+0x3c/0x3e
>  Modules linked in: ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6
>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.7.0-rc3-test-00013-g501c2375253c #14
>  Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
>   ffff880215e8f4d0 ffff880215e8f4d0 ffffffff812fc8e3 0000000000000000
>   ffffffff81d3e55b ffff880215e8f520 ffffffff8104fc88 ffffffff8104fcab
>   0000000915e88300 0000000000000046 ffffffffa019b29a 0000000000000001
>  Call Trace:
>   [<ffffffff812fc8e3>] dump_stack+0x67/0x90
>   [<ffffffff8104fc88>] __warn+0xcb/0xe9
>   [<ffffffff8104fcab>] ? warn_slowpath_null+0x5/0x1f
>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 0 at kernel/module.c:268 module_assert_mutex_or_preempt+0x3c/0x3e
> [...]
>
> Which gives us rather useless information. Worse yet, there's some race
> that causes this, and I seldom trigger it, so I have no idea what
> happened.
>
> This would not be an issue if that warning was a WARN_ON_ONCE().
>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> ---
> diff --git a/kernel/module.c b/kernel/module.c
> index 5f71aa63ed2a..51c89d86752c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -264,7 +264,7 @@ static void module_assert_mutex_or_preempt(void)
>  	if (unlikely(!debug_locks))
>  		return;
>  
> -	WARN_ON(!rcu_read_lock_sched_held() &&
> +	WARN_ON_ONCE(!rcu_read_lock_sched_held() &&
>  		!lockdep_is_held(&module_mutex));
>  #endif
>  }