On Thu, May 30, 2019 at 03:54:14PM +0200, Petr Mladek wrote: > On Wed 2019-05-29 14:02:24, Josh Poimboeuf wrote: > > The above panic occurs when loading two modules at the same time with > > ftrace enabled, where at least one of the modules is a livepatch module: > > > > CPU0 CPU1 > > klp_enable_patch() > > klp_init_object_loaded() > > module_disable_ro() > > ftrace_module_enable() > > ftrace_arch_code_modify_post_process() > > set_all_modules_text_ro() > > klp_write_object_relocations() > > apply_relocate_add() > > *patches read-only code* - BOOM > > This patch looks fine and fixes the race: > > Reviewed-by: Petr Mladek <pmladek@xxxxxxxx> > > > That said, the semantic of text_mutex is a bit unclear: > > + It serializes RO/RW setting but not NX True. module_enable_nx() is a static function which is only called internally. I should probably rename it to __module_enable_nx() so the locking semantics match the others. > + Nothing prevents manipulation of the access rights > by external code before the module is ready-enough. > I mean before the sections are set RO by the module > loader itself. > > Most sections are ready in MODULE_STATE_COMMING state. > Only ro_after_init sections need to stay RW longer, > see my question below. > > > > diff --git a/kernel/module.c b/kernel/module.c > > index 6e6712b3aaf5..3c056b56aefa 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -3519,7 +3534,7 @@ static noinline int do_init_module(struct module *mod) > > /* Switch to core kallsyms now init is done: kallsyms may be walking! */ > > rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); > > #endif > > - module_enable_ro(mod, true); > > + __module_enable_ro(mod, true); > > The "true" parameter causes that also ro_after_init section is > set read only. What is the purpose of this section, please? > > I ask because module_enable_ro(mod, true) can be called > earlier from klp_init_object_loaded() from do_one_initcall(). > > For example, it could some MODULE_STATE_LIVE notifier > when it requires write access to ro_after_init section. Hm, I think you're right. klp_init_object_loaded() should change the module_enable_ro() argument to false when it's called from a patch module's initcall. Maybe we can instead remove __module_enable_ro()'s after_init argument and just make it smarter? It should only do ro_after_init frobbing if the module state is MODULE_STATE_LIVE. > Anyway, the above is a separate problem. This patch looks > fine for the original problem. Thanks for the review. I'll post another version, with the above changes and with the patches split up like Miroslav suggested. -- Josh
