On Thu, 10 Oct 2019 19:01:11 +0200
Peter Zijlstra <pet...@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;
> }
>
