Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
@ 2019-06-27  8:13 Petr Mladek
  2019-06-27 22:17 ` Thomas Gleixner
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Petr Mladek @ 2019-06-27  8:13 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Poimboeuf, Miroslav Benes, Jessica Yu, Jiri Kosina,
	Joe Lawrence, linux-kernel, live-patching, Johannes Erdfelt,
	Ingo Molnar, mhiramat, torvalds, tglx, Petr Mladek

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>
---
 arch/x86/kernel/ftrace.c |  3 +++
 kernel/trace/ftrace.c    | 10 +---------
 2 files changed, 4 insertions(+), 9 deletions(-)

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);
-- 
2.16.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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-28  7:32 ` [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() Miroslav Benes
  2 siblings, 0 replies; 21+ messages in thread
From: Thomas Gleixner @ 2019-06-27 22:17 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Steven Rostedt, Josh Poimboeuf, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, 27 Jun 2019, Petr Mladek wrote:
> 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

  ^^^^^^^^^ Sigh

> 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>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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-28  7:32 ` [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() Miroslav Benes
  2 siblings, 1 reply; 21+ messages in thread
From: Josh Poimboeuf @ 2019-06-27 22:47 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Steven Rostedt, Miroslav Benes, Jessica Yu, Jiri Kosina,
	Joe Lawrence, linux-kernel, live-patching, Johannes Erdfelt,
	Ingo Molnar, mhiramat, torvalds, tglx

Thanks a lot for fixing this Petr.

On Thu, Jun 27, 2019 at 10:13:34AM +0200, Petr Mladek wrote:
> @@ -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;
>  }

Releasing the lock in a separate function seems a bit surprising and
fragile, would it be possible to do something like this instead?

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index b38c388d1087..89ea1af6fd13 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -37,15 +37,21 @@
 int ftrace_arch_code_modify_prepare(void)
 {
 	mutex_lock(&text_mutex);
+
 	set_kernel_text_rw();
 	set_all_modules_text_rw();
+
+	mutex_unlock(&text_mutex);
 	return 0;
 }
 
 int ftrace_arch_code_modify_post_process(void)
 {
+	mutex_lock(&text_mutex);
+
 	set_all_modules_text_ro();
 	set_kernel_text_ro();
+
 	mutex_unlock(&text_mutex);
 	return 0;
 }

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-27 22:47 ` Josh Poimboeuf
@ 2019-06-27 23:04   ` Steven Rostedt
  2019-06-27 23:09     ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-27 23:04 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Petr Mladek, Miroslav Benes, Jessica Yu, Jiri Kosina,
	Joe Lawrence, linux-kernel, live-patching, Johannes Erdfelt,
	Ingo Molnar, mhiramat, torvalds, tglx

On Thu, 27 Jun 2019 17:47:29 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Thanks a lot for fixing this Petr.
> 
> On Thu, Jun 27, 2019 at 10:13:34AM +0200, Petr Mladek wrote:
> > @@ -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;
> >  }  
> 
> Releasing the lock in a separate function seems a bit surprising and
> fragile, would it be possible to do something like this instead?
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index b38c388d1087..89ea1af6fd13 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -37,15 +37,21 @@
>  int ftrace_arch_code_modify_prepare(void)
>  {
>  	mutex_lock(&text_mutex);
> +
>  	set_kernel_text_rw();
>  	set_all_modules_text_rw();
> +
> +	mutex_unlock(&text_mutex);
>  	return 0;
>  }
>  
>  int ftrace_arch_code_modify_post_process(void)
>  {
> +	mutex_lock(&text_mutex);
> +
>  	set_all_modules_text_ro();
>  	set_kernel_text_ro();
> +
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }

I agree with Josh on this. As the original bug was the race between
ftrace and live patching / modules changing the text from ro to rw and
vice versa. Just protecting the update to the text permissions is more
robust, and should be more self documenting when we need to handle
other architectures for this.

-- Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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
  0 siblings, 2 replies; 21+ messages in thread
From: Thomas Gleixner @ 2019-06-27 23:09 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Poimboeuf, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, 27 Jun 2019, Steven Rostedt wrote:
> On Thu, 27 Jun 2019 17:47:29 -0500
> > Releasing the lock in a separate function seems a bit surprising and
> > fragile, would it be possible to do something like this instead?
> > 
> > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > index b38c388d1087..89ea1af6fd13 100644
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -37,15 +37,21 @@
> >  int ftrace_arch_code_modify_prepare(void)
> >  {
> >  	mutex_lock(&text_mutex);
> > +
> >  	set_kernel_text_rw();
> >  	set_all_modules_text_rw();
> > +
> > +	mutex_unlock(&text_mutex);
> >  	return 0;
> >  }
> >  
> >  int ftrace_arch_code_modify_post_process(void)
> >  {
> > +	mutex_lock(&text_mutex);
> > +
> >  	set_all_modules_text_ro();
> >  	set_kernel_text_ro();
> > +
> >  	mutex_unlock(&text_mutex);
> >  	return 0;
> >  }
> 
> I agree with Josh on this. As the original bug was the race between
> ftrace and live patching / modules changing the text from ro to rw and
> vice versa. Just protecting the update to the text permissions is more
> robust, and should be more self documenting when we need to handle
> other architectures for this.

How is that supposed to work?

    ftrace  	     	
	prepare()
	 setrw()
			setro()
	patch <- FAIL

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-27 23:09     ` Thomas Gleixner
@ 2019-06-27 23:12       ` Steven Rostedt
  2019-06-27 23:19       ` Josh Poimboeuf
  1 sibling, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2019-06-27 23:12 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Josh Poimboeuf, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Fri, 28 Jun 2019 01:09:08 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 27 Jun 2019, Steven Rostedt wrote:
> > On Thu, 27 Jun 2019 17:47:29 -0500  
> > > Releasing the lock in a separate function seems a bit surprising and
> > > fragile, would it be possible to do something like this instead?
> > > 
> > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > > index b38c388d1087..89ea1af6fd13 100644
> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -37,15 +37,21 @@
> > >  int ftrace_arch_code_modify_prepare(void)
> > >  {
> > >  	mutex_lock(&text_mutex);
> > > +
> > >  	set_kernel_text_rw();
> > >  	set_all_modules_text_rw();
> > > +
> > > +	mutex_unlock(&text_mutex);
> > >  	return 0;
> > >  }
> > >  
> > >  int ftrace_arch_code_modify_post_process(void)
> > >  {
> > > +	mutex_lock(&text_mutex);
> > > +
> > >  	set_all_modules_text_ro();
> > >  	set_kernel_text_ro();
> > > +
> > >  	mutex_unlock(&text_mutex);
> > >  	return 0;
> > >  }  
> > 
> > I agree with Josh on this. As the original bug was the race between
> > ftrace and live patching / modules changing the text from ro to rw and
> > vice versa. Just protecting the update to the text permissions is more
> > robust, and should be more self documenting when we need to handle
> > other architectures for this.  
> 
> How is that supposed to work?
> 
>     ftrace  	     	
> 	prepare()
> 	 setrw()
> 			setro()
> 	patch <- FAIL
>

Good point. I guess we the original patch is fine. Josh?

-- Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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
                           ` (3 more replies)
  1 sibling, 4 replies; 21+ messages in thread
From: Josh Poimboeuf @ 2019-06-27 23:19 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Steven Rostedt, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Fri, Jun 28, 2019 at 01:09:08AM +0200, Thomas Gleixner wrote:
> On Thu, 27 Jun 2019, Steven Rostedt wrote:
> > On Thu, 27 Jun 2019 17:47:29 -0500
> > > Releasing the lock in a separate function seems a bit surprising and
> > > fragile, would it be possible to do something like this instead?
> > > 
> > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > > index b38c388d1087..89ea1af6fd13 100644
> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -37,15 +37,21 @@
> > >  int ftrace_arch_code_modify_prepare(void)
> > >  {
> > >  	mutex_lock(&text_mutex);
> > > +
> > >  	set_kernel_text_rw();
> > >  	set_all_modules_text_rw();
> > > +
> > > +	mutex_unlock(&text_mutex);
> > >  	return 0;
> > >  }
> > >  
> > >  int ftrace_arch_code_modify_post_process(void)
> > >  {
> > > +	mutex_lock(&text_mutex);
> > > +
> > >  	set_all_modules_text_ro();
> > >  	set_kernel_text_ro();
> > > +
> > >  	mutex_unlock(&text_mutex);
> > >  	return 0;
> > >  }
> > 
> > I agree with Josh on this. As the original bug was the race between
> > ftrace and live patching / modules changing the text from ro to rw and
> > vice versa. Just protecting the update to the text permissions is more
> > robust, and should be more self documenting when we need to handle
> > other architectures for this.
> 
> How is that supposed to work?
> 
>     ftrace  	     	
> 	prepare()
> 	 setrw()
> 			setro()
> 	patch <- FAIL

/me dodges frozen shark

You are right of course.  My brain has apparently already shut off for
the day.

Maybe a comment or two would help though.

-- 
Josh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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 17:33         ` Jiri Kosina
  3 siblings, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2019-06-27 23:25 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Thomas Gleixner, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, 27 Jun 2019 18:19:52 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> /me dodges frozen shark
> 
> You are right of course.  My brain has apparently already shut off for
> the day.

And I agreed with your miscalculation. I guess I should have looked
deeper into it. Or have less faith in you ;-)

> 
> Maybe a comment or two would help though.

Agreed, this is fragile. I'll just add a patch on top of it with some
comments and pull this in today.

-- Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-27 23:19       ` Josh Poimboeuf
@ 2019-06-27 23:25         ` Thomas Gleixner
  2019-06-27 23:25         ` Steven Rostedt
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 21+ messages in thread
From: Thomas Gleixner @ 2019-06-27 23:25 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Steven Rostedt, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, 27 Jun 2019, Josh Poimboeuf wrote:
> On Fri, Jun 28, 2019 at 01:09:08AM +0200, Thomas Gleixner wrote:
> > On Thu, 27 Jun 2019, Steven Rostedt wrote:
> > > I agree with Josh on this. As the original bug was the race between
> > > ftrace and live patching / modules changing the text from ro to rw and
> > > vice versa. Just protecting the update to the text permissions is more
> > > robust, and should be more self documenting when we need to handle
> > > other architectures for this.
> > 
> > How is that supposed to work?
> > 
> >     ftrace  	     	
> > 	prepare()
> > 	 setrw()
> > 			setro()
> > 	patch <- FAIL
> 
> /me dodges frozen shark
> 
> You are right of course.  My brain has apparently already shut off for
> the day.
> 
> Maybe a comment or two would help though.

Agreed. That would indeed be useful.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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
  3 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-28  1:13 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Thomas Gleixner, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, 27 Jun 2019 18:19:52 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> Maybe a comment or two would help though.
> 

I'm adding the following change.  Care to add a "reviewed-by" for this
one?

-- Steve

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 33786044d5ac..d7e93b2783fd 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -36,6 +36,11 @@
 
 int ftrace_arch_code_modify_prepare(void)
 {
+	/*
+	 * Need to grab text_mutex to prevent a race from module loading
+	 * and live kernel patching from changing the text permissions while
+	 * ftrace has it set to "read/write".
+	 */
 	mutex_lock(&text_mutex);
 	set_kernel_text_rw();
 	set_all_modules_text_rw();

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28  1:13         ` Steven Rostedt
@ 2019-06-28  1:17           ` Josh Poimboeuf
  0 siblings, 0 replies; 21+ messages in thread
From: Josh Poimboeuf @ 2019-06-28  1:17 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Thomas Gleixner, Petr Mladek, Miroslav Benes, Jessica Yu,
	Jiri Kosina, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, Jun 27, 2019 at 09:13:04PM -0400, Steven Rostedt wrote:
> On Thu, 27 Jun 2019 18:19:52 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > Maybe a comment or two would help though.
> > 
> 
> I'm adding the following change.  Care to add a "reviewed-by" for this
> one?
> 
> -- Steve
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 33786044d5ac..d7e93b2783fd 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -36,6 +36,11 @@
>  
>  int ftrace_arch_code_modify_prepare(void)
>  {
> +	/*
> +	 * Need to grab text_mutex to prevent a race from module loading
> +	 * and live kernel patching from changing the text permissions while
> +	 * ftrace has it set to "read/write".
> +	 */
>  	mutex_lock(&text_mutex);
>  	set_kernel_text_rw();
>  	set_all_modules_text_rw();

For the patch+comment:

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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-28  7:32 ` Miroslav Benes
  2019-06-28 10:52   ` Petr Mladek
  2 siblings, 1 reply; 21+ messages in thread
From: Miroslav Benes @ 2019-06-28  7:32 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Steven Rostedt, Josh Poimboeuf, Jessica Yu, Jiri Kosina,
	Joe Lawrence, linux-kernel, live-patching, Johannes Erdfelt,
	Ingo Molnar, mhiramat, torvalds, tglx

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28  7:32 ` [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() Miroslav Benes
@ 2019-06-28 10:52   ` Petr Mladek
  2019-06-28 13:54     ` Steven Rostedt
  0 siblings, 1 reply; 21+ messages in thread
From: Petr Mladek @ 2019-06-28 10:52 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Johannes Erdfelt, Steven Rostedt, Jessica Yu, Jiri Kosina,
	mhiramat, Ingo Molnar, tglx, torvalds, Joe Lawrence,
	Josh Poimboeuf, linux-kernel, live-patching

On Fri 2019-06-28 09:32:03, Miroslav Benes wrote:
> On Thu, 27 Jun 2019, Petr Mladek wrote:
> > @@ -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".

Grr, I usually check compiler warnings but I evidently skipped it
in this case :-(

Steven, should I send a fixed/folloup patch or will you just
fix it when pushing?

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28 10:52   ` Petr Mladek
@ 2019-06-28 13:54     ` Steven Rostedt
  2019-06-28 15:46       ` Steven Rostedt
  0 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-28 13:54 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Miroslav Benes, Johannes Erdfelt, Jessica Yu, Jiri Kosina,
	mhiramat, Ingo Molnar, tglx, torvalds, Joe Lawrence,
	Josh Poimboeuf, linux-kernel, live-patching

On Fri, 28 Jun 2019 12:52:32 +0200
Petr Mladek <pmladek@suse.com> wrote:

> On Fri 2019-06-28 09:32:03, Miroslav Benes wrote:
> > On Thu, 27 Jun 2019, Petr Mladek wrote:  
> > > @@ -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".  
> 
> Grr, I usually check compiler warnings but I evidently skipped it
> in this case :-(
> 
> Steven, should I send a fixed/folloup patch or will you just
> fix it when pushing?
> 

I'll fix it up.

-- Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28 13:54     ` Steven Rostedt
@ 2019-06-28 15:46       ` Steven Rostedt
  2019-06-28 15:51         ` Josh Poimboeuf
  0 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-28 15:46 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Miroslav Benes, Johannes Erdfelt, Jessica Yu, Jiri Kosina,
	mhiramat, Ingo Molnar, tglx, torvalds, Joe Lawrence,
	Josh Poimboeuf, linux-kernel, live-patching

On Fri, 28 Jun 2019 09:54:24 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 28 Jun 2019 12:52:32 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > On Fri 2019-06-28 09:32:03, Miroslav Benes wrote:  
> > > On Thu, 27 Jun 2019, Petr Mladek wrote:    
> > > > @@ -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".    
> > 
> > Grr, I usually check compiler warnings but I evidently skipped it
> > in this case :-(
> > 
> > Steven, should I send a fixed/folloup patch or will you just
> > fix it when pushing?
> >   
> 
> I'll fix it up.

Also note, this would have been caught with my test suite, as it checks
for any new warnings.

-- Steve


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28 15:46       ` Steven Rostedt
@ 2019-06-28 15:51         ` Josh Poimboeuf
  0 siblings, 0 replies; 21+ messages in thread
From: Josh Poimboeuf @ 2019-06-28 15:51 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Petr Mladek, Miroslav Benes, Johannes Erdfelt, Jessica Yu,
	Jiri Kosina, mhiramat, Ingo Molnar, tglx, torvalds, Joe Lawrence,
	linux-kernel, live-patching

On Fri, Jun 28, 2019 at 11:46:27AM -0400, Steven Rostedt wrote:
> On Fri, 28 Jun 2019 09:54:24 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 28 Jun 2019 12:52:32 +0200
> > Petr Mladek <pmladek@suse.com> wrote:
> > 
> > > On Fri 2019-06-28 09:32:03, Miroslav Benes wrote:  
> > > > On Thu, 27 Jun 2019, Petr Mladek wrote:    
> > > > > @@ -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".    
> > > 
> > > Grr, I usually check compiler warnings but I evidently skipped it
> > > in this case :-(
> > > 
> > > Steven, should I send a fixed/folloup patch or will you just
> > > fix it when pushing?
> > >   
> > 
> > I'll fix it up.
> 
> Also note, this would have been caught with my test suite, as it checks
> for any new warnings.

Sadly, I noticed this while reviewing the code, but then somehow got
distracted and it vanished.  Another review fail...

-- 
Josh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-27 23:19       ` Josh Poimboeuf
                           ` (2 preceding siblings ...)
  2019-06-28  1:13         ` Steven Rostedt
@ 2019-06-28 17:33         ` Jiri Kosina
  2019-06-28 17:37           ` Steven Rostedt
  3 siblings, 1 reply; 21+ messages in thread
From: Jiri Kosina @ 2019-06-28 17:33 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Thomas Gleixner, Steven Rostedt, Petr Mladek, Miroslav Benes,
	Jessica Yu, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Thu, 27 Jun 2019, Josh Poimboeuf wrote:

> > How is that supposed to work?
> > 
> >     ftrace  	     	
> > 	prepare()
> > 	 setrw()
> > 			setro()
> > 	patch <- FAIL
> 
> /me dodges frozen shark
> 
> You are right of course.  My brain has apparently already shut off for
> the day.
> 
> Maybe a comment or two would help though.

I'd actually prefer (perhaps in parallel to the comment) using the 
__acquires() and __releases() anotations, so that sparse and friends don't 
get confused by that either.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28 17:33         ` Jiri Kosina
@ 2019-06-28 17:37           ` Steven Rostedt
  2019-06-29 20:56             ` Jiri Kosina
  0 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-28 17:37 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Josh Poimboeuf, Thomas Gleixner, Petr Mladek, Miroslav Benes,
	Jessica Yu, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Fri, 28 Jun 2019 19:33:30 +0200 (CEST)
Jiri Kosina <jikos@kernel.org> wrote:

> On Thu, 27 Jun 2019, Josh Poimboeuf wrote:
> 
> > > How is that supposed to work?
> > > 
> > >     ftrace  	     	
> > > 	prepare()
> > > 	 setrw()
> > > 			setro()
> > > 	patch <- FAIL  
> > 
> > /me dodges frozen shark
> > 
> > You are right of course.  My brain has apparently already shut off for
> > the day.
> > 
> > Maybe a comment or two would help though.  
> 
> I'd actually prefer (perhaps in parallel to the comment) using the 
> __acquires() and __releases() anotations, so that sparse and friends don't 
> get confused by that either.
> 

Care to send a patch? :-)

-- Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  2019-06-28 17:37           ` Steven Rostedt
@ 2019-06-29 20:56             ` Jiri Kosina
  2019-06-29 21:19               ` Steven Rostedt
  0 siblings, 1 reply; 21+ messages in thread
From: Jiri Kosina @ 2019-06-29 20:56 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Poimboeuf, Thomas Gleixner, Petr Mladek, Miroslav Benes,
	Jessica Yu, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Fri, 28 Jun 2019, Steven Rostedt wrote:

> > > > How is that supposed to work?
> > > > 
> > > >     ftrace  	     	
> > > > 	prepare()
> > > > 	 setrw()
> > > > 			setro()
> > > > 	patch <- FAIL  
> > > 
> > > /me dodges frozen shark
> > > 
> > > You are right of course.  My brain has apparently already shut off for
> > > the day.
> > > 
> > > Maybe a comment or two would help though.  
> > 
> > I'd actually prefer (perhaps in parallel to the comment) using the 
> > __acquires() and __releases() anotations, so that sparse and friends don't 
> > get confused by that either.
> > 
> 
> Care to send a patch? :-)

From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] ftrace/x86: anotate text_mutex split between ftrace_arch_code_modify_post_process() and ftrace_arch_code_modify_prepare()

ftrace_arch_code_modify_prepare() is acquiring text_mutex, while the 
corresponding release is happening in ftrace_arch_code_modify_post_process().

This has already been documented in the code, but let's also make the fact 
that this is intentional clear to the semantic analysis tools such as 
sparse.

Fixes: 39611265edc1a ("ftrace/x86: Add a comment to why we take text_mutex in ftrace_arch_code_modify_prepare()")
Fixes: d5b844a2cf507 ("ftrace/x86: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()")
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/kernel/ftrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index d7e93b2783fd..76228525acd0 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -35,6 +35,7 @@
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void)
+    __acquires(&text_mutex)
 {
 	/*
 	 * Need to grab text_mutex to prevent a race from module loading
@@ -48,6 +49,7 @@ int ftrace_arch_code_modify_prepare(void)
 }
 
 int ftrace_arch_code_modify_post_process(void)
+    __releases(&text_mutex)
 {
 	set_all_modules_text_ro();
 	set_kernel_text_ro();

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()
  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
  0 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-29 21:19 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Josh Poimboeuf, Thomas Gleixner, Petr Mladek, Miroslav Benes,
	Jessica Yu, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

On Sat, 29 Jun 2019 22:56:47 +0200 (CEST)
Jiri Kosina <jikos@kernel.org> wrote:

> > Care to send a patch? :-)  
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] ftrace/x86: anotate text_mutex split between ftrace_arch_code_modify_post_process() and ftrace_arch_code_modify_prepare()

Care to send a proper patch ;-)

As in, a separate email. Makes it much easier to apply directly with
my scripts.

-- Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH] ftrace/x86: anotate text_mutex split between ftrace_arch_code_modify_post_process() and ftrace_arch_code_modify_prepare()
  2019-06-29 21:19               ` Steven Rostedt
@ 2019-06-29 21:22                 ` Jiri Kosina
  0 siblings, 0 replies; 21+ messages in thread
From: Jiri Kosina @ 2019-06-29 21:22 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Poimboeuf, Thomas Gleixner, Petr Mladek, Miroslav Benes,
	Jessica Yu, Joe Lawrence, linux-kernel, live-patching,
	Johannes Erdfelt, Ingo Molnar, mhiramat, torvalds

From: Jiri Kosina <jkosina@suse.cz>

ftrace_arch_code_modify_prepare() is acquiring text_mutex, while the
corresponding release is happening in ftrace_arch_code_modify_post_process().

This has already been documented in the code, but let's also make the fact
that this is intentional clear to the semantic analysis tools such as sparse.

Fixes: 39611265edc1a ("ftrace/x86: Add a comment to why we take text_mutex in ftrace_arch_code_modify_prepare()")
Fixes: d5b844a2cf507 ("ftrace/x86: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()")
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/kernel/ftrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index d7e93b2783fd..76228525acd0 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -35,6 +35,7 @@
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void)
+    __acquires(&text_mutex)
 {
 	/*
 	 * Need to grab text_mutex to prevent a race from module loading
@@ -48,6 +49,7 @@ int ftrace_arch_code_modify_prepare(void)
 }
 
 int ftrace_arch_code_modify_post_process(void)
+    __releases(&text_mutex)
 {
 	set_all_modules_text_ro();
 	set_kernel_text_ro();
	
-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() Miroslav Benes
2019-06-28 10:52   ` Petr Mladek
2019-06-28 13:54     ` Steven Rostedt
2019-06-28 15:46       ` Steven Rostedt
2019-06-28 15:51         ` Josh Poimboeuf

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org live-patching@archiver.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/ public-inbox