On Thu, Oct 10, 2019 at 08:50:11AM -0400, Steven Rostedt wrote: > On Thu, 10 Oct 2019 11:36:50 +0200 > Peter Zijlstra <pet...@infradead.org> wrote:
> > > 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 still don't get it. So you do both, the initial NOPs and the CALL patching from ftrace_module_init(). > > > 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. They call that from ftrace_arch_code_modofy_prepare(), and the patch I just send makes that unused. So all should be good ;-)
