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
+ 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.
Anyway, the above is a separate problem. This patch looks
fine for the original problem.
Best Regards,
Petr
