Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 10 Oct 2019 19:01:11 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Oct 10, 2019 at 10:55:15AM -0400, Steven Rostedt wrote:
> 
> > OK, so basically this moves the enabling of function tracing from
> > within the ftrace_module_enable() code without releasing the
> > ftrace_lock mutex.
> > 
> > But we have an issue with the state of the module here, as it is still
> > set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:
> > 
> > 
> > 	CPU0				CPU1
> > 	----				----
> >  echo function > current_tracer
> > 				modprobe foo
> > 				  enable foo functions to be traced
> > 				  (foo function records not disabled)
> >  echo nop > current_tracer
> > 
> >    disable all functions being
> >    traced including foo functions
> > 
> >    arch calls set_all_modules_text_rw()
> >     [skips UNFORMED modules, which foo still is ]
> > 
> > 				  set foo's text to read-only
> > 				  foo's state to COMING
> > 
> >    tries to disable foo's functions
> >    foo's text is read-only
> > 
> >    BUG trying to write to ro text!!!
> > 
> > 
> > Like I said, this is very subtle. It may no longer be a bug on x86
> > with your patches, but it will bug on ARM or anything else that still
> > uses set_all_modules_text_rw() in the ftrace prepare code.  
> 
> I can't immediately follow, but I think we really should go there.
> 
> For now, something like this might work:

Hmm, I'm lost at what the below is trying to do with respect to the
above.

-- Steve

> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -34,6 +34,8 @@
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
> +static int ftrace_poke_late = 0;
> +
>  int ftrace_arch_code_modify_prepare(void)
>      __acquires(&text_mutex)
>  {
> @@ -43,12 +45,15 @@ int ftrace_arch_code_modify_prepare(void
>  	 * ftrace has it set to "read/write".
>  	 */
>  	mutex_lock(&text_mutex);
> +	ftrace_poke_late = 1;
>  	return 0;
>  }
>  
>  int ftrace_arch_code_modify_post_process(void)
>      __releases(&text_mutex)
>  {
> +	text_poke_finish();
> +	ftrace_poke_late = 0;
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }
> @@ -116,7 +121,10 @@ ftrace_modify_code_direct(unsigned long
>  		return ret;
>  
>  	/* replace the text with the new text */
> -	text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
> +	if (ftrace_poke_late)
> +		text_poke_queue((void *)ip, new_code, MCOUNT_INSN_SIZE, NULL);
> +	else
> +		text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
>  	return 0;
>  }
>