On Thu, 10 Oct 2019 14:29:09 +0200
Peter Zijlstra <pet...@infradead.org> wrote:
> Yes, your code is anal about checking the NOPs, so you first have to
Yep, because being paranoid about modifying code is always good ;-)
> write NOPs before you can write CALLs, if it is enabled. But afaict you
> really can do all that from ftrace_module_init(), as long as you do it
> all under the same ftrace_lock section.
>
> If you have two sections, like now, then there is indeed that race that
> ftrace can get enabled in between, and all the confusion that that
> brings.
>
> That is, what's fundamentally buggered about something like this?
>
> ---
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 62a50bf399d6..5f7113f100ce 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5626,6 +5626,48 @@ static int ftrace_process_locs(struct module *mod,
> ftrace_update_code(mod, start_pg);
> if (!mod)
> local_irq_restore(flags);
> +
> + if (ftrace_disabled || !mod)
> + goto out_loop;
> +
> + do_for_each_ftrace_rec(pg, rec) {
[snip]
> +
> + if (ftrace_start_up && cnt) {
> + int failed = __ftrace_replace_code(rec, 1);
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.
-- Steve
> + if (failed) {
> + ftrace_bug(failed, rec);
> + goto out_loop;
> + }
> + }
> +
> + } while_for_each_ftrace_rec();
> +
> + out_loop:
> +
> ret = 0;
> out:
> mutex_unlock(&ftrace_lock);
> @@ -5793,73 +5835,6 @@ void ftrace_release_mod(struct module *mod)
>
> void ftrace_module_enable(struct module *mod)
> {
> - struct dyn_ftrace *rec;
> - struct ftrace_page *pg;
> -
> - mutex_lock(&ftrace_lock);
> -
> - if (ftrace_disabled)
> - goto out_unlock;
> -
> - /*
> - * If the tracing is enabled, go ahead and enable the record.
> - *
> - * The reason not to enable the record immediately is the
> - * inherent check of ftrace_make_nop/ftrace_make_call for
> - * correct previous instructions. Making first the NOP
> - * conversion puts the module to the correct state, thus
> - * passing the ftrace_make_call check.
> - *
> - * We also delay this to after the module code already set the
> - * text to read-only, as we now need to set it back to read-write
> - * so that we can modify the text.
> - */
> - if (ftrace_start_up)
> - ftrace_arch_code_modify_prepare();
> -
> - do_for_each_ftrace_rec(pg, rec) {
> - int cnt;
> - /*
> - * do_for_each_ftrace_rec() is a double loop.
> - * module text shares the pg. If a record is
> - * not part of this module, then skip this pg,
> - * which the "break" will do.
> - */
> - if (!within_module_core(rec->ip, mod) &&
> - !within_module_init(rec->ip, mod))
> - break;
> -
> - cnt = 0;
> -
> - /*
> - * When adding a module, we need to check if tracers are
> - * currently enabled and if they are, and can trace this record,
> - * we need to enable the module functions as well as update the
> - * reference counts for those function records.
> - */
> - if (ftrace_start_up)
> - cnt += referenced_filters(rec);
> -
> - /* This clears FTRACE_FL_DISABLED */
> - rec->flags = cnt;
> -
> - if (ftrace_start_up && cnt) {
> - int failed = __ftrace_replace_code(rec, 1);
> - if (failed) {
> - ftrace_bug(failed, rec);
> - goto out_loop;
> - }
> - }
> -
> - } while_for_each_ftrace_rec();
> -
> - out_loop:
> - if (ftrace_start_up)
> - ftrace_arch_code_modify_post_process();
> -
> - out_unlock:
> - mutex_unlock(&ftrace_lock);
> -
> process_cached_mods(mod->name);
> }
>
