[PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Thomas Gleixner]

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>

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Josh Poimboeuf]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Thomas Gleixner]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Josh Poimboeuf]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Thomas Gleixner]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Josh Poimboeuf]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Miroslav Benes]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Petr Mladek]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Josh Poimboeuf]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Jiri Kosina]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Jiri Kosina]

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

Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update_code()

[Steven Rostedt]

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

[PATCH] ftrace/x86: anotate text_mutex split between ftrace_arch_code_modify_post_process() and ftrace_arch_code_modify_prepare()

[Jiri Kosina]

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