On Thu, 10 Oct 2019 11:36:50 +0200 Peter Zijlstra <[email protected]> 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
