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

[Steven Rostedt <rostedt@goodmis.org>]

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