Messages in this thread | | | Date | Thu, 10 Oct 2019 08:50:11 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write |
| |
On Thu, 10 Oct 2019 11:36:50 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> > > > The reason is that ftrace_module_enable() is called after the module > > > > has set its text to read-only. There's subtle reasons that this needs > > > > to be called afterward, and we need to continue to do so. > > > > > > Please explain.
I knew you were going to say that ;-)
I was too tired last night to go back and see all the issues.
> > > > I don't see any reason what so ever.. > > > > load_module() > > ... > > complete_formation() > > mutex_lock(&module_mutex); > > ... > > module_enable_ro(); > > module_enable_nx(); > > module_enable_x(); > > > > mod->state = MODULE_STATE_COMING; > > mutex_unlock(&module_mutex); > > > > prepare_coming_module() > > ftrace_module_enable(); > > ... > > > > IOW, we're doing ftrace_module_enable() immediately after we flip it > > RO+X. There is nothing in between that we can possibly rely on.
One reason for the above is the module_mutex. The lock order is that module_mutex may be called inside the ftrace_lock, but not vice-versa.
The ftrace_module_init() was called due to the setting of all modules rw when registering a ftrace function while a module was being loaded. We may have eliminated this issue on x86 but other archs still call set_all_modules_text_rw/ro() when enabling function tracing. Thus, the race will still exist there.
See commit a949ae560a511 for the description of it.
After implementing that commit, I found it a bit cleaner to handle modules in general by breaking it up into setting nops first, and then determining if we need to trace that module.
> > > > I was going to put: > > > > blocking_notifier_call_chain(&module_notify_list, > > MODULE_STATE_UNFORMED, mod); > > > > right before module_enable_ro(), in complete_formation(), for jump_label > > and static_call. It looks like ftrace (and possibly klp) want that too. > > Also, you already have ftrace_module_init() right before that. The only > thing inbetween ftrace_module_init() and ftrace_module_enable() is > verify_exported_symbols() and module_bug_finalize().
Yep, see commit a949ae560a511 about that too.
> > Do you really need that for patching stuff?
Because arm and nds32 still use the set_all_modules_text_rw(), this patch would at least remove that for all archs, and only modify the text of a module that isn't running yet. Which I thought was a plus.
Just need to be careful about other archs, or we need to at least make sure they change too.
-- Steve
| |