From: Miroslav Benes <mbenes@suse.cz>
To: Petr Mladek <pmladek@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Jessica Yu <jeyu@kernel.org>, Jiri Kosina <jikos@kernel.org>,
Joe Lawrence <joe.lawrence@redhat.com>,
linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
Johannes Erdfelt <johannes@erdfelt.com>,
Ingo Molnar <mingo@kernel.org>,
mhiramat@kernel.org, torvalds@linux-foundation.org,
tglx@linutronix.de
Subject: Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
Date: Fri, 28 Jun 2019 09:32:03 +0200 (CEST) [thread overview]
Message-ID: <alpine.LSU.2.21.1906280928410.17146@pobox.suse.cz> (raw)
In-Reply-To: <20190627081334.12793-1-pmladek@suse.com>
On Thu, 27 Jun 2019, Petr Mladek wrote:
> The commit 9f255b632bf12c4dd7 ("module: Fix livepatch/ftrace module text
> permissions race") causes a possible deadlock between register_kprobe()
> and ftrace_run_update_code() when ftrace is using stop_machine().
>
> 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
>
> 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 the commit 2d1e38f56622b9b
> ("kprobes: Cure hotplug lock ordering issues"). Many locks are involved.
> To be on the safe side, text_mutex must become a low level lock taken
> after cpu_hotplug_lock.rw_sem.
>
> This can't be achieved easily with the current ftrace design.
> For example, arm calls set_all_modules_text_rw() already in
> ftrace_arch_code_modify_prepare(), see arch/arm/kernel/ftrace.c.
> This functions is called:
>
> + outside stop_machine() from ftrace_run_update_code()
> + without stop_machine() from ftrace_module_enable()
>
> Fortunately, the problematic fix is needed only on x86_64. It is
> the only architecture that calls set_all_modules_text_rw()
> in ftrace path and supports livepatching at the same time.
>
> Therefore it is enough to move text_mutex handling from the generic
> kernel/trace/ftrace.c into arch/x86/kernel/ftrace.c:
>
> ftrace_arch_code_modify_prepare()
> ftrace_arch_code_modify_post_process()
>
> This patch basically reverts the ftrace part of the problematic
> commit 9f255b632bf12c4dd7 ("module: Fix livepatch/ftrace module
> text permissions race"). And provides x86_64 specific-fix.
>
> Some refactoring of the ftrace code will be needed when livepatching
> is implemented for arm or nds32. These architectures call
> set_all_modules_text_rw() and use stop_machine() at the same time.
>
> Fixes: 9f255b632bf12c4dd7 ("module: Fix livepatch/ftrace module text permissions race")
> Signed-off-by: Petr Mladek <pmladek@suse.com>
Reported-by: Miroslav Benes <mbenes@suse.cz>
> 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;
Should be just "return;", because the function is "static void".
With that
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Miroslav
next prev parent reply other threads:[~2019-06-28 7:32 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-27 8:13 [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() Petr Mladek
2019-06-27 22:17 ` Thomas Gleixner
2019-06-27 22:47 ` Josh Poimboeuf
2019-06-27 23:04 ` Steven Rostedt
2019-06-27 23:09 ` Thomas Gleixner
2019-06-27 23:12 ` Steven Rostedt
2019-06-27 23:19 ` Josh Poimboeuf
2019-06-27 23:25 ` Thomas Gleixner
2019-06-27 23:25 ` Steven Rostedt
2019-06-28 1:13 ` Steven Rostedt
2019-06-28 1:17 ` Josh Poimboeuf
2019-06-28 17:33 ` Jiri Kosina
2019-06-28 17:37 ` Steven Rostedt
2019-06-29 20:56 ` Jiri Kosina
2019-06-29 21:19 ` Steven Rostedt
2019-06-29 21:22 ` [PATCH] ftrace/x86: anotate text_mutex split between ftrace_arch_code_modify_post_process() and ftrace_arch_code_modify_prepare() Jiri Kosina
2019-06-28 7:32 ` Miroslav Benes [this message]
2019-06-28 10:52 ` [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() Petr Mladek
2019-06-28 13:54 ` Steven Rostedt
2019-06-28 15:46 ` Steven Rostedt
2019-06-28 15:51 ` Josh Poimboeuf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LSU.2.21.1906280928410.17146@pobox.suse.cz \
--to=mbenes@suse.cz \
--cc=jeyu@kernel.org \
--cc=jikos@kernel.org \
--cc=joe.lawrence@redhat.com \
--cc=johannes@erdfelt.com \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@kernel.org \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).