Messages in this thread Patch in this message | | | Date | Mon, 14 Mar 2016 13:50:09 -0400 | From | Jessica Yu <> | Subject | Re: livepatch/module: remove livepatch module notifier |
| |
+++ Petr Mladek [14/03/16 16:06 +0100]: >On Fri 2016-03-11 15:03:48, Jessica Yu wrote: >> Remove the livepatch module notifier in favor of directly enabling and >> disabling patches to modules in the module loader. Hard-coding the >> function calls ensures that ftrace_module_enable() is run before >> klp_module_coming() during module load, and that klp_module_going() is >> run before ftrace_release_mod() during module unload. This way, ftrace >> and livepatch code is run in the correct order during the module >> load/unload sequence without dependence on the module notifier call chain. >> >> Signed-off-by: Jessica Yu <jeyu@redhat.com> >> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> >> Acked-by: Rusty Russell <rusty@rustcorp.com.au> >> --- >> include/linux/livepatch.h | 13 ++++ >> kernel/livepatch/core.c | 147 ++++++++++++++++++++++------------------------ >> kernel/module.c | 10 ++++ >> 3 files changed, 94 insertions(+), 76 deletions(-) >> >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >> index 780f00c..4fafbae 100644 >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -866,103 +866,108 @@ int klp_register_patch(struct klp_patch *patch) >> } >> EXPORT_SYMBOL_GPL(klp_register_patch); >> >> -static int klp_module_notify_coming(struct klp_patch *patch, >> - struct klp_object *obj) >> +int klp_module_coming(struct module *mod) >> { >> - struct module *pmod = patch->mod; >> - struct module *mod = obj->mod; >> int ret; >> + struct klp_patch *patch; >> + struct klp_object *obj; >> >> - ret = klp_init_object_loaded(patch, obj); >> - if (ret) { >> - pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n", >> - pmod->name, mod->name, ret); >> - return ret; >> - } >> + if (WARN_ON(mod->state != MODULE_STATE_COMING)) >> + return -EINVAL; >> >> - if (patch->state == KLP_DISABLED) >> - return 0; >> + mutex_lock(&klp_mutex); >> + /* >> + * Each module has to know that klp_module_coming() >> + * has been called. We never know what module will >> + * get patched by a new patch. >> + */ >> + mod->klp_alive = true; >> >> - pr_notice("applying patch '%s' to loading module '%s'\n", >> - pmod->name, mod->name); >> + list_for_each_entry(patch, &klp_patches, list) { >> + klp_for_each_object(patch, obj) { >> + if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) >> + continue; >> >> - ret = klp_enable_object(obj); >> - if (ret) >> - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", >> - pmod->name, mod->name, ret); >> - return ret; >> -} >> + obj->mod = mod; >> >> -static void klp_module_notify_going(struct klp_patch *patch, >> - struct klp_object *obj) >> -{ >> - struct module *pmod = patch->mod; >> - struct module *mod = obj->mod; >> + ret = klp_init_object_loaded(patch, obj); >> + if (ret) { >> + pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n", >> + patch->mod->name, obj->mod->name, ret); >> + goto err; >> + } >> >> - if (patch->state == KLP_DISABLED) >> - goto disabled; >> + if (patch->state == KLP_DISABLED) >> + break; >> + >> + pr_notice("applying patch '%s' to loading module '%s'\n", >> + patch->mod->name, obj->mod->name); >> + >> + ret = klp_enable_object(obj); >> + if (ret) { >> + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", >> + patch->mod->name, obj->mod->name, ret); >> + goto err; >> + } >> + >> + break; >> + } >> + } >> >> - pr_notice("reverting patch '%s' on unloading module '%s'\n", >> - pmod->name, mod->name); >> + mutex_unlock(&klp_mutex); >> >> - klp_disable_object(obj); >> + return 0; >> >> -disabled: >> +err: >> + /* >> + * If a patch is unsuccessfully applied, return >> + * error to the module loader. >> + */ >> + pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n", >> + patch->mod->name, obj->mod->name, obj->mod->name); > >One more thing. We should add here: > > mod->klp_alive = true; > >Otherwise, there is a small race window when a new patch will try to >patch the module.
Ah, you are right. I think you meant false, though :-) Will fix that.
>> klp_free_object_loaded(obj); >> + mutex_unlock(&klp_mutex); >> + >> + return ret; >> } > >With the above fix: > >Reviewed-by: Petr Mladek <pmladek@suse.cz> > > >Best Regards, >Petr
| |