* Re: [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race
[not found] ` <20190614170408.1b1162dc@gandalf.local.home>
@ 2019-06-26 8:22 ` Miroslav Benes
2019-06-26 13:37 ` Petr Mladek
0 siblings, 1 reply; 5+ messages in thread
From: Miroslav Benes @ 2019-06-26 8:22 UTC (permalink / raw)
To: Steven Rostedt
Cc: Josh Poimboeuf, Jessica Yu, Petr Mladek, Jiri Kosina,
Joe Lawrence, linux-kernel, live-patching, Johannes Erdfelt,
Ingo Molnar, mhiramat, torvalds, tglx
On Fri, 14 Jun 2019, Steven Rostedt wrote:
> On Thu, 13 Jun 2019 20:07:22 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> > It's possible for livepatch and ftrace to be toggling a module's text
> > permissions at the same time, resulting in the following panic:
> >
>
> [..]
>
> > The above panic occurs when loading two modules at the same time with
> > ftrace enabled, where at least one of the modules is a livepatch module:
> >
> > CPU0 CPU1
> > klp_enable_patch()
> > klp_init_object_loaded()
> > module_disable_ro()
> > ftrace_module_enable()
> > ftrace_arch_code_modify_post_process()
> > set_all_modules_text_ro()
> > klp_write_object_relocations()
> > apply_relocate_add()
> > *patches read-only code* - BOOM
> >
> > A similar race exists when toggling ftrace while loading a livepatch
> > module.
> >
> > Fix it by ensuring that the livepatch and ftrace code patching
> > operations -- and their respective permissions changes -- are protected
> > by the text_mutex.
> >
> > Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
> > Fixes: 444d13ff10fb ("modules: add ro_after_init support")
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Acked-by: Jessica Yu <jeyu@kernel.org>
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > Reviewed-by: Miroslav Benes <mbenes@suse.cz>
>
> This patch looks uncontroversial. I'm going to pull this one in and
> start testing it. And if it works, I'll push to Linus.
Triggered this on s390x. Masami CCed and Linus as well, because the patch
is in master branch and we are after -rc6. Thomas CCed because of commit
2d1e38f56622 ("kprobes: Cure hotplug lock ordering issues").
======================================================
WARNING: possible circular locking dependency detected
5.2.0-rc6 #1 Tainted: G O K
------------------------------------------------------
insmod/1393 is trying to acquire lock:
000000002fdee887 (cpu_hotplug_lock.rw_sem){++++}, at: stop_machine+0x2e/0x60
but task is already holding lock:
000000005b22fb82 (text_mutex){+.+.}, at: ftrace_run_update_code+0x2a/0xa0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (text_mutex){+.+.}:
validate_chain.isra.21+0xb32/0xd70
__lock_acquire+0x4b8/0x928
lock_acquire+0x102/0x230
__mutex_lock+0x88/0x908
mutex_lock_nested+0x32/0x40
register_kprobe+0x254/0x658
init_kprobes+0x11a/0x168
do_one_initcall+0x70/0x318
kernel_init_freeable+0x456/0x508
kernel_init+0x22/0x150
ret_from_fork+0x30/0x34
kernel_thread_starter+0x0/0xc
-> #0 (cpu_hotplug_lock.rw_sem){++++}:
check_prev_add+0x90c/0xde0
validate_chain.isra.21+0xb32/0xd70
__lock_acquire+0x4b8/0x928
lock_acquire+0x102/0x230
cpus_read_lock+0x62/0xd0
stop_machine+0x2e/0x60
arch_ftrace_update_code+0x2e/0x40
ftrace_run_update_code+0x40/0xa0
ftrace_startup+0xb2/0x168
register_ftrace_function+0x64/0x88
klp_patch_object+0x1a2/0x290
klp_enable_patch+0x554/0x980
do_one_initcall+0x70/0x318
do_init_module+0x6e/0x250
load_module+0x1782/0x1990
__s390x_sys_finit_module+0xaa/0xf0
system_call+0xd8/0x2d0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(text_mutex);
lock(cpu_hotplug_lock.rw_sem);
lock(text_mutex);
lock(cpu_hotplug_lock.rw_sem);
*** DEADLOCK ***
3 locks held by insmod/1393:
#0: 00000000a9723159 (klp_mutex){+.+.}, at: klp_enable_patch+0x62/0x980
#1: 00000000bd173ffc (ftrace_lock){+.+.}, at: register_ftrace_function+0x56/0x88
#2: 000000005b22fb82 (text_mutex){+.+.}, at: ftrace_run_update_code+0x2a/0xa0
stack backtrace:
CPU: 0 PID: 1393 Comm: insmod Tainted: G O K 5.2.0-rc6 #1
Hardware name: IBM 2827 H43 400 (KVM/Linux)
Call Trace:
([<00000000682100b4>] show_stack+0xb4/0x130)
[<0000000068adeb8c>] dump_stack+0x94/0xd8
[<00000000682b563c>] print_circular_bug+0x1f4/0x328
[<00000000682b7264>] check_prev_add+0x90c/0xde0
[<00000000682b826a>] validate_chain.isra.21+0xb32/0xd70
[<00000000682ba018>] __lock_acquire+0x4b8/0x928
[<00000000682ba952>] lock_acquire+0x102/0x230
[<0000000068243c12>] cpus_read_lock+0x62/0xd0
[<0000000068336bf6>] stop_machine+0x2e/0x60
[<0000000068355b3e>] arch_ftrace_update_code+0x2e/0x40
[<0000000068355b90>] ftrace_run_update_code+0x40/0xa0
[<000000006835971a>] ftrace_startup+0xb2/0x168
[<0000000068359834>] register_ftrace_function+0x64/0x88
[<00000000682e8a9a>] klp_patch_object+0x1a2/0x290
[<00000000682e7d64>] klp_enable_patch+0x554/0x980
[<00000000681fcaa0>] do_one_initcall+0x70/0x318
[<000000006831449e>] do_init_module+0x6e/0x250
[<0000000068312b52>] load_module+0x1782/0x1990
[<0000000068313002>] __s390x_sys_finit_module+0xaa/0xf0
[<0000000068b03f30>] system_call+0xd8/0x2d0
INFO: lockdep is turned off.
If I am reading the code correctly, ftrace_run_update_code() takes
text_mutex now and then calls stop_machine(), which grabs
cpu_hotplug_lock for reading. do_optimize_kprobes() (see the comment
there) expects cpu_hotplug_lock to be held and takes text_mutex. Whoops.
Maybe there is a simple fix, but reverting the commit in this stage seems
warranted.
Miroslav
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race
2019-06-26 8:22 ` [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race Miroslav Benes
@ 2019-06-26 13:37 ` Petr Mladek
2019-06-26 14:44 ` Thomas Gleixner
0 siblings, 1 reply; 5+ messages in thread
From: Petr Mladek @ 2019-06-26 13:37 UTC (permalink / raw)
To: Miroslav Benes
Cc: Steven Rostedt, Josh Poimboeuf, Jessica Yu, Jiri Kosina,
Joe Lawrence, linux-kernel, live-patching, Johannes Erdfelt,
Ingo Molnar, mhiramat, torvalds, tglx
On Wed 2019-06-26 10:22:45, Miroslav Benes wrote:
> On Fri, 14 Jun 2019, Steven Rostedt wrote:
>
> > On Thu, 13 Jun 2019 20:07:22 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > > It's possible for livepatch and ftrace to be toggling a module's text
> > > permissions at the same time, resulting in the following panic:
> > >
> >
> > [..]
> >
> > > The above panic occurs when loading two modules at the same time with
> > > ftrace enabled, where at least one of the modules is a livepatch module:
> > >
> > > CPU0 CPU1
> > > klp_enable_patch()
> > > klp_init_object_loaded()
> > > module_disable_ro()
> > > ftrace_module_enable()
> > > ftrace_arch_code_modify_post_process()
> > > set_all_modules_text_ro()
> > > klp_write_object_relocations()
> > > apply_relocate_add()
> > > *patches read-only code* - BOOM
> > >
> > > A similar race exists when toggling ftrace while loading a livepatch
> > > module.
> > >
> > > Fix it by ensuring that the livepatch and ftrace code patching
> > > operations -- and their respective permissions changes -- are protected
> > > by the text_mutex.
> > >
> > > Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
> > > Fixes: 444d13ff10fb ("modules: add ro_after_init support")
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Acked-by: Jessica Yu <jeyu@kernel.org>
> > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > Reviewed-by: Miroslav Benes <mbenes@suse.cz>
> >
> > This patch looks uncontroversial. I'm going to pull this one in and
> > start testing it. And if it works, I'll push to Linus.
>
> Triggered this on s390x. Masami CCed and Linus as well, because the patch
> is in master branch and we are after -rc6. Thomas CCed because of commit
> 2d1e38f56622 ("kprobes: Cure hotplug lock ordering issues").
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.2.0-rc6 #1 Tainted: G O K
> ------------------------------------------------------
> insmod/1393 is trying to acquire lock:
> 000000002fdee887 (cpu_hotplug_lock.rw_sem){++++}, at: stop_machine+0x2e/0x60
>
> but task is already holding lock:
> 000000005b22fb82 (text_mutex){+.+.}, at: ftrace_run_update_code+0x2a/0xa0
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (text_mutex){+.+.}:
> validate_chain.isra.21+0xb32/0xd70
> __lock_acquire+0x4b8/0x928
> lock_acquire+0x102/0x230
> __mutex_lock+0x88/0x908
> mutex_lock_nested+0x32/0x40
> register_kprobe+0x254/0x658
> init_kprobes+0x11a/0x168
> do_one_initcall+0x70/0x318
> kernel_init_freeable+0x456/0x508
> kernel_init+0x22/0x150
> ret_from_fork+0x30/0x34
> kernel_thread_starter+0x0/0xc
>
> -> #0 (cpu_hotplug_lock.rw_sem){++++}:
> check_prev_add+0x90c/0xde0
> validate_chain.isra.21+0xb32/0xd70
> __lock_acquire+0x4b8/0x928
> lock_acquire+0x102/0x230
> cpus_read_lock+0x62/0xd0
> stop_machine+0x2e/0x60
> arch_ftrace_update_code+0x2e/0x40
> ftrace_run_update_code+0x40/0xa0
> ftrace_startup+0xb2/0x168
> register_ftrace_function+0x64/0x88
> klp_patch_object+0x1a2/0x290
> klp_enable_patch+0x554/0x980
> do_one_initcall+0x70/0x318
> do_init_module+0x6e/0x250
> load_module+0x1782/0x1990
> __s390x_sys_finit_module+0xaa/0xf0
> system_call+0xd8/0x2d0
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(text_mutex);
> lock(cpu_hotplug_lock.rw_sem);
> lock(text_mutex);
> lock(cpu_hotplug_lock.rw_sem);
It is similar problem that has been solved by 2d1e38f56622b9bb5af8
("kprobes: Cure hotplug lock ordering issues"). This commit solved
it by always taking cpu_hotplug_lock.rw_sem before text_mutex inside.
If we follow the lock ordering then ftrace has to take text_mutex
only when stop_machine() is not called or from code called via
stop_machine() parameter.
This is not easy with the current design. For example, arm calls
set_all_modules_text_rw() already in ftrace_arch_code_modify_prepare(),
see arch/arm/kernel/ftrace.c. And it is called:
+ outside stop_machine() from ftrace_run_update_code()
+ without stop_machine() from ftrace_module_enable()
A conservative solution for 5.2 release would be to move text_mutex
locking from the generic kernel/trace/ftrace.c into
arch/x86/kernel/ftrace.c:
ftrace_arch_code_modify_prepare()
ftrace_arch_code_modify_post_process()
It should be enough to fix the original problem because
x86 is the only architecture that calls set_all_modules_text_rw()
in ftrace path and supports livepatching at the same time.
We would need to do some refactoring when livepatching is enabled
for arm or nds32.
The conservative solution:
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 0927bb158ffc..33786044d5ac 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -22,6 +22,7 @@
#include <linux/init.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/memory.h>
#include <trace/syscall.h>
@@ -35,6 +36,7 @@
int ftrace_arch_code_modify_prepare(void)
{
+ mutex_lock(&text_mutex);
set_kernel_text_rw();
set_all_modules_text_rw();
return 0;
@@ -44,6 +46,7 @@ int ftrace_arch_code_modify_post_process(void)
{
set_all_modules_text_ro();
set_kernel_text_ro();
+ mutex_unlock(&text_mutex);
return 0;
}
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 38277af44f5c..d3034a4a3fcc 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -34,7 +34,6 @@
#include <linux/hash.h>
#include <linux/rcupdate.h>
#include <linux/kprobes.h>
-#include <linux/memory.h>
#include <trace/events/sched.h>
@@ -2611,12 +2610,10 @@ static void ftrace_run_update_code(int command)
{
int ret;
- mutex_lock(&text_mutex);
-
ret = ftrace_arch_code_modify_prepare();
FTRACE_WARN_ON(ret);
if (ret)
- goto out_unlock;
+ return ret;
/*
* By default we use stop_machine() to modify the code.
@@ -2628,9 +2625,6 @@ static void ftrace_run_update_code(int command)
ret = ftrace_arch_code_modify_post_process();
FTRACE_WARN_ON(ret);
-
-out_unlock:
- mutex_unlock(&text_mutex);
}
static void ftrace_run_modify_code(struct ftrace_ops *ops, int command,
@@ -5784,7 +5778,6 @@ void ftrace_module_enable(struct module *mod)
struct ftrace_page *pg;
mutex_lock(&ftrace_lock);
- mutex_lock(&text_mutex);
if (ftrace_disabled)
goto out_unlock;
@@ -5846,7 +5839,6 @@ void ftrace_module_enable(struct module *mod)
ftrace_arch_code_modify_post_process();
out_unlock:
- mutex_unlock(&text_mutex);
mutex_unlock(&ftrace_lock);
process_cached_mods(mod->name);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race
2019-06-26 13:37 ` Petr Mladek
@ 2019-06-26 14:44 ` Thomas Gleixner
2019-06-26 14:52 ` Josh Poimboeuf
2019-06-26 14:59 ` Steven Rostedt
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Gleixner @ 2019-06-26 14:44 UTC (permalink / raw)
To: Petr Mladek
Cc: Miroslav Benes, Steven Rostedt, Josh Poimboeuf, Jessica Yu,
Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds
On Wed, 26 Jun 2019, Petr Mladek wrote:
> On Wed 2019-06-26 10:22:45, Miroslav Benes wrote:
> It is similar problem that has been solved by 2d1e38f56622b9bb5af8
> ("kprobes: Cure hotplug lock ordering issues"). This commit solved
> it by always taking cpu_hotplug_lock.rw_sem before text_mutex inside.
>
> If we follow the lock ordering then ftrace has to take text_mutex
> only when stop_machine() is not called or from code called via
> stop_machine() parameter.
>
> This is not easy with the current design. For example, arm calls
> set_all_modules_text_rw() already in ftrace_arch_code_modify_prepare(),
> see arch/arm/kernel/ftrace.c. And it is called:
>
> + outside stop_machine() from ftrace_run_update_code()
> + without stop_machine() from ftrace_module_enable()
>
> A conservative solution for 5.2 release would be to move text_mutex
> locking from the generic kernel/trace/ftrace.c into
> arch/x86/kernel/ftrace.c:
>
> ftrace_arch_code_modify_prepare()
> ftrace_arch_code_modify_post_process()
>
> It should be enough to fix the original problem because
> x86 is the only architecture that calls set_all_modules_text_rw()
> in ftrace path and supports livepatching at the same time.
Looks correct, but I've paged out all the gory details vs. lock ordering in
that area.
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race
2019-06-26 14:44 ` Thomas Gleixner
@ 2019-06-26 14:52 ` Josh Poimboeuf
2019-06-26 14:59 ` Steven Rostedt
1 sibling, 0 replies; 5+ messages in thread
From: Josh Poimboeuf @ 2019-06-26 14:52 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Petr Mladek, Miroslav Benes, Steven Rostedt, Jessica Yu,
Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds
On Wed, Jun 26, 2019 at 04:44:45PM +0200, Thomas Gleixner wrote:
> On Wed, 26 Jun 2019, Petr Mladek wrote:
> > On Wed 2019-06-26 10:22:45, Miroslav Benes wrote:
> > It is similar problem that has been solved by 2d1e38f56622b9bb5af8
> > ("kprobes: Cure hotplug lock ordering issues"). This commit solved
> > it by always taking cpu_hotplug_lock.rw_sem before text_mutex inside.
> >
> > If we follow the lock ordering then ftrace has to take text_mutex
> > only when stop_machine() is not called or from code called via
> > stop_machine() parameter.
> >
> > This is not easy with the current design. For example, arm calls
> > set_all_modules_text_rw() already in ftrace_arch_code_modify_prepare(),
> > see arch/arm/kernel/ftrace.c. And it is called:
> >
> > + outside stop_machine() from ftrace_run_update_code()
> > + without stop_machine() from ftrace_module_enable()
> >
> > A conservative solution for 5.2 release would be to move text_mutex
> > locking from the generic kernel/trace/ftrace.c into
> > arch/x86/kernel/ftrace.c:
> >
> > ftrace_arch_code_modify_prepare()
> > ftrace_arch_code_modify_post_process()
> >
> > It should be enough to fix the original problem because
> > x86 is the only architecture that calls set_all_modules_text_rw()
> > in ftrace path and supports livepatching at the same time.
>
> Looks correct, but I've paged out all the gory details vs. lock ordering in
> that area.
Looks good to me as well, Petr can you post a proper patch?
--
Josh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race
2019-06-26 14:44 ` Thomas Gleixner
2019-06-26 14:52 ` Josh Poimboeuf
@ 2019-06-26 14:59 ` Steven Rostedt
1 sibling, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2019-06-26 14:59 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Petr Mladek, Miroslav Benes, Josh Poimboeuf, Jessica Yu,
Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds
On Wed, 26 Jun 2019 16:44:45 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > It should be enough to fix the original problem because
> > x86 is the only architecture that calls set_all_modules_text_rw()
> > in ftrace path and supports livepatching at the same time.
>
> Looks correct, but I've paged out all the gory details vs. lock ordering in
> that area.
I don't believe ftrace_lock and text_mutex had an order before Petr's
initial patches. Reversing them shouldn't be an issue here. They were
basically both "leaf" mutexes (not grabbing any mutexes when they are
held).
-- Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-26 14:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1560474114.git.jpoimboe@redhat.com>
[not found] ` <ab43d56ab909469ac5d2520c5d944ad6d4abd476.1560474114.git.jpoimboe@redhat.com>
[not found] ` <20190614170408.1b1162dc@gandalf.local.home>
2019-06-26 8:22 ` [PATCH 1/3] module: Fix livepatch/ftrace module text permissions race Miroslav Benes
2019-06-26 13:37 ` Petr Mladek
2019-06-26 14:44 ` Thomas Gleixner
2019-06-26 14:52 ` Josh Poimboeuf
2019-06-26 14:59 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).