* [PATCH v2 0/2] Livepatch module notifier cleanup @ 2016-03-11 20:03 Jessica Yu 2016-03-11 20:03 ` [PATCH v2 1/2] modules: split part of complete_formation() into prepare_coming_module() Jessica Yu 2016-03-11 20:03 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu 0 siblings, 2 replies; 16+ messages in thread From: Jessica Yu @ 2016-03-11 20:03 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Rusty Russell Cc: Steven Rostedt, live-patching, linux-kernel, Jessica Yu These are the remaining 2 patches that came from the original ftrace/livepatch module notifier patchset found here: https://lkml.org/lkml/2016/2/8/1180 Basically, the patchset does a bit of module.c cleanup (patch 1) in preparation for the klp_module_{coming,going} calls (patch 2). We decided to stop relying on the module notifier callchain in favor of hard-coding the appropriate livepatch function calls that handle coming and going modules. Hard-coding these calls will guarantee that ftrace and livepatch exit/initialization routines are called in the correct order without relying on module notifiers. The patches should be nearly exactly the same as those from the previous discussion, except in patch 2 I've added back the #if IS_ENABLED(CONFIG_LIVEPATCH) guard in livepatch.h. Patches based on linux-next. Previous discussion (v4) found here: https://lkml.org/lkml/2016/2/8/1180 v2: - we don't need to change mod->state to GOING when cleaning up during failed module load - modify mod->state check to allow klp_module_going() to be called in both COMING and GOING states Jessica Yu (2): modules: split part of complete_formation() into prepare_coming_module() livepatch/module: remove livepatch module notifier include/linux/livepatch.h | 13 ++++ kernel/livepatch/core.c | 147 ++++++++++++++++++++++------------------------ kernel/module.c | 36 +++++++++--- 3 files changed, 112 insertions(+), 84 deletions(-) -- 2.4.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] modules: split part of complete_formation() into prepare_coming_module() 2016-03-11 20:03 [PATCH v2 0/2] Livepatch module notifier cleanup Jessica Yu @ 2016-03-11 20:03 ` Jessica Yu 2016-03-14 15:12 ` Petr Mladek 2016-03-11 20:03 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu 1 sibling, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-03-11 20:03 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Rusty Russell Cc: Steven Rostedt, live-patching, linux-kernel, Jessica Yu Put all actions in complete_formation() that are performed after module->state is set to MODULE_STATE_COMING into a separate function prepare_coming_module(). This split prepares for the removal of the livepatch module notifiers in favor of hard-coding function calls to klp_module_{coming,going} in the module loader. The complete_formation -> prepare_coming_module split will also make error handling easier since we can jump to the appropriate error label to do any module GOING cleanup after all the COMING-actions have completed. Signed-off-by: Jessica Yu <jeyu@redhat.com> Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com> Acked-by: Rusty Russell <rusty@rustcorp.com.au> --- kernel/module.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 87cfeb2..1981ae0 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3312,6 +3312,14 @@ out_unlocked: return err; } +static int prepare_coming_module(struct module *mod) +{ + ftrace_module_enable(mod); + blocking_notifier_call_chain(&module_notify_list, + MODULE_STATE_COMING, mod); + return 0; +} + static int complete_formation(struct module *mod, struct load_info *info) { int err; @@ -3335,9 +3343,6 @@ static int complete_formation(struct module *mod, struct load_info *info) mod->state = MODULE_STATE_COMING; mutex_unlock(&module_mutex); - ftrace_module_enable(mod); - blocking_notifier_call_chain(&module_notify_list, - MODULE_STATE_COMING, mod); return 0; out: @@ -3459,13 +3464,17 @@ static int load_module(struct load_info *info, const char __user *uargs, if (err) goto ddebug_cleanup; + err = prepare_coming_module(mod); + if (err) + goto bug_cleanup; + /* Module is ready to execute: parsing args may do that. */ after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, -32768, 32767, mod, unknown_module_param_cb); if (IS_ERR(after_dashes)) { err = PTR_ERR(after_dashes); - goto bug_cleanup; + goto coming_cleanup; } else if (after_dashes) { pr_warn("%s: parameters '%s' after `--' ignored\n", mod->name, after_dashes); @@ -3474,7 +3483,7 @@ static int load_module(struct load_info *info, const char __user *uargs, /* Link in to syfs. */ err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp); if (err < 0) - goto bug_cleanup; + goto coming_cleanup; /* Get rid of temporary copy. */ free_copy(info); @@ -3484,15 +3493,16 @@ static int load_module(struct load_info *info, const char __user *uargs, return do_init_module(mod); + coming_cleanup: + blocking_notifier_call_chain(&module_notify_list, + MODULE_STATE_GOING, mod); + bug_cleanup: /* module_bug_cleanup needs module_mutex protection */ mutex_lock(&module_mutex); module_bug_cleanup(mod); mutex_unlock(&module_mutex); - blocking_notifier_call_chain(&module_notify_list, - MODULE_STATE_GOING, mod); - /* we can't deallocate the module until we clear memory protection */ module_disable_ro(mod); module_disable_nx(mod); -- 2.4.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] modules: split part of complete_formation() into prepare_coming_module() 2016-03-11 20:03 ` [PATCH v2 1/2] modules: split part of complete_formation() into prepare_coming_module() Jessica Yu @ 2016-03-14 15:12 ` Petr Mladek 0 siblings, 0 replies; 16+ messages in thread From: Petr Mladek @ 2016-03-14 15:12 UTC (permalink / raw) To: Jessica Yu Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, live-patching, linux-kernel On Fri 2016-03-11 15:03:47, Jessica Yu wrote: > Put all actions in complete_formation() that are performed after > module->state is set to MODULE_STATE_COMING into a separate function > prepare_coming_module(). This split prepares for the removal of the > livepatch module notifiers in favor of hard-coding function calls to > klp_module_{coming,going} in the module loader. > > The complete_formation -> prepare_coming_module split will also make error > handling easier since we can jump to the appropriate error label to do any > module GOING cleanup after all the COMING-actions have completed. > > Signed-off-by: Jessica Yu <jeyu@redhat.com> > Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com> > Acked-by: Rusty Russell <rusty@rustcorp.com.au> > --- > kernel/module.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/kernel/module.c b/kernel/module.c > index 87cfeb2..1981ae0 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3312,6 +3312,14 @@ out_unlocked: > return err; > } > > +static int prepare_coming_module(struct module *mod) > +{ > + ftrace_module_enable(mod); > + blocking_notifier_call_chain(&module_notify_list, > + MODULE_STATE_COMING, mod); > + return 0; > +} Nit, just in case you send a new version. Please, move this below complete_formation(). It is nice when the funtions are defined in the same order as they are called :-) It is a cosmetic change and it should not invalidate other acks. In each case, the patch looks fine: Reviewed-by: Petr Mladek <pmladek@suse.cz> Best Regards, Petr ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] livepatch/module: remove livepatch module notifier 2016-03-11 20:03 [PATCH v2 0/2] Livepatch module notifier cleanup Jessica Yu 2016-03-11 20:03 ` [PATCH v2 1/2] modules: split part of complete_formation() into prepare_coming_module() Jessica Yu @ 2016-03-11 20:03 ` Jessica Yu 2016-03-14 15:06 ` Petr Mladek 2016-03-14 20:01 ` [PATCH v2 2/2] " Josh Poimboeuf 1 sibling, 2 replies; 16+ messages in thread From: Jessica Yu @ 2016-03-11 20:03 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Rusty Russell Cc: Steven Rostedt, live-patching, linux-kernel, Jessica Yu 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/include/linux/livepatch.h b/include/linux/livepatch.h index c056797..bd830d5 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -24,6 +24,8 @@ #include <linux/module.h> #include <linux/ftrace.h> +#if IS_ENABLED(CONFIG_LIVEPATCH) + #include <asm/livepatch.h> enum klp_state { @@ -132,4 +134,15 @@ int klp_unregister_patch(struct klp_patch *); int klp_enable_patch(struct klp_patch *); int klp_disable_patch(struct klp_patch *); +/* Called from the module loader during module coming/going states */ +int klp_module_coming(struct module *mod); +void klp_module_going(struct module *mod); + +#else /* !CONFIG_LIVEPATCH */ + +static inline int klp_module_coming(struct module *mod) { return 0; } +static inline void klp_module_going(struct module *mod) { } + +#endif /* CONFIG_LIVEPATCH */ + #endif /* _LINUX_LIVEPATCH_H_ */ 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 @@ -99,12 +99,12 @@ static void klp_find_object_module(struct klp_object *obj) /* * We do not want to block removal of patched modules and therefore * we do not take a reference here. The patches are removed by - * a going module handler instead. + * klp_module_going() instead. */ mod = find_module(obj->name); /* - * Do not mess work of the module coming and going notifiers. - * Note that the patch might still be needed before the going handler + * Do not mess work of klp_module_coming() and klp_module_going(). + * Note that the patch might still be needed before klp_module_going() * is called. Module functions can be called even in the GOING state * until mod->exit() finishes. This is especially important for * patches that modify semantic of the functions. @@ -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); klp_free_object_loaded(obj); + mutex_unlock(&klp_mutex); + + return ret; } -static int klp_module_notify(struct notifier_block *nb, unsigned long action, - void *data) +/* Module can be either COMING or GOING */ +void klp_module_going(struct module *mod) { - int ret; - struct module *mod = data; struct klp_patch *patch; struct klp_object *obj; - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING) - return 0; + if (WARN_ON(mod->state != MODULE_STATE_GOING && + mod->state != MODULE_STATE_COMING)) + return; mutex_lock(&klp_mutex); - /* - * Each module has to know that the notifier has been called. - * We never know what module will get patched by a new patch. + * Each module has to know that klp_module_going() + * has been called. We never know what module will + * get patched by a new patch. */ - if (action == MODULE_STATE_COMING) - mod->klp_alive = true; - else /* MODULE_STATE_GOING */ - mod->klp_alive = false; + mod->klp_alive = false; 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; - if (action == MODULE_STATE_COMING) { - obj->mod = mod; - ret = klp_module_notify_coming(patch, obj); - if (ret) { - obj->mod = NULL; - pr_warn("patch '%s' is in an inconsistent state!\n", - patch->mod->name); - } - } else /* MODULE_STATE_GOING */ - klp_module_notify_going(patch, obj); + if (patch->state != KLP_DISABLED) { + pr_notice("reverting patch '%s' on unloading module '%s'\n", + patch->mod->name, obj->mod->name); + klp_disable_object(obj); + } + klp_free_object_loaded(obj); break; } } mutex_unlock(&klp_mutex); - - return 0; } -static struct notifier_block klp_module_nb = { - .notifier_call = klp_module_notify, - .priority = INT_MIN+1, /* called late but before ftrace notifier */ -}; - static int __init klp_init(void) { int ret; @@ -973,21 +978,11 @@ static int __init klp_init(void) return -EINVAL; } - ret = register_module_notifier(&klp_module_nb); - if (ret) - return ret; - klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj); - if (!klp_root_kobj) { - ret = -ENOMEM; - goto unregister; - } + if (!klp_root_kobj) + return -ENOMEM; return 0; - -unregister: - unregister_module_notifier(&klp_module_nb); - return ret; } module_init(klp_init); diff --git a/kernel/module.c b/kernel/module.c index 1981ae0..932fb35 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -53,6 +53,7 @@ #include <asm/sections.h> #include <linux/tracepoint.h> #include <linux/ftrace.h> +#include <linux/livepatch.h> #include <linux/async.h> #include <linux/percpu.h> #include <linux/kmemleak.h> @@ -984,6 +985,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, mod->exit(); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); ftrace_release_mod(mod); async_synchronize_full(); @@ -3258,6 +3260,7 @@ fail: module_put(mod); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); ftrace_release_mod(mod); free_module(mod); wake_up_all(&module_wq); @@ -3314,7 +3317,13 @@ out_unlocked: static int prepare_coming_module(struct module *mod) { + int err; + ftrace_module_enable(mod); + err = klp_module_coming(mod); + if (err) + return err; + blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); return 0; @@ -3496,6 +3505,7 @@ static int load_module(struct load_info *info, const char __user *uargs, coming_cleanup: blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); bug_cleanup: /* module_bug_cleanup needs module_mutex protection */ -- 2.4.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier 2016-03-11 20:03 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu @ 2016-03-14 15:06 ` Petr Mladek 2016-03-14 17:50 ` Jessica Yu 2016-03-14 20:01 ` [PATCH v2 2/2] " Josh Poimboeuf 1 sibling, 1 reply; 16+ messages in thread From: Petr Mladek @ 2016-03-14 15:06 UTC (permalink / raw) To: Jessica Yu Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, live-patching, linux-kernel 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. > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-03-14 15:06 ` Petr Mladek @ 2016-03-14 17:50 ` Jessica Yu 2016-03-15 9:10 ` Petr Mladek 0 siblings, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-03-14 17:50 UTC (permalink / raw) To: Petr Mladek Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, live-patching, linux-kernel +++ 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-03-14 17:50 ` Jessica Yu @ 2016-03-15 9:10 ` Petr Mladek 0 siblings, 0 replies; 16+ messages in thread From: Petr Mladek @ 2016-03-15 9:10 UTC (permalink / raw) To: Jessica Yu Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, live-patching, linux-kernel On Mon 2016-03-14 13:50:09, Jessica Yu wrote: > +++ Petr Mladek [14/03/16 16:06 +0100]: > >On Fri 2016-03-11 15:03:48, Jessica Yu wrote: > >>+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. Yeah, I meant false :-) Thanks, Petr ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier 2016-03-11 20:03 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu 2016-03-14 15:06 ` Petr Mladek @ 2016-03-14 20:01 ` Josh Poimboeuf 1 sibling, 0 replies; 16+ messages in thread From: Josh Poimboeuf @ 2016-03-14 20:01 UTC (permalink / raw) To: Jessica Yu Cc: Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Rusty Russell, Steven Rostedt, live-patching, linux-kernel On Fri, Mar 11, 2016 at 03:03:48PM -0500, Jessica Yu wrote: > +/* Module can be either COMING or GOING */ IMO this comment doesn't really add anything: the below WARN_ON already says as much. Also the location of the comment right above the function is confusing: someone not familiar with the code might wonder why the function is called klp_module_going() if the module can be either coming or going. So I think the comment can just be removed. > +void klp_module_going(struct module *mod) > { > - int ret; > - struct module *mod = data; > struct klp_patch *patch; > struct klp_object *obj; > > - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING) > - return 0; > + if (WARN_ON(mod->state != MODULE_STATE_GOING && > + mod->state != MODULE_STATE_COMING)) > + return; -- Josh ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 0/4] Fix ordering of ftrace/livepatch calls on module load and unload @ 2016-02-09 4:50 Jessica Yu 2016-02-09 4:50 ` [PATCH v4 4/4] livepatch/module: remove livepatch module notifier Jessica Yu 0 siblings, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-02-09 4:50 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Rusty Russell, Steven Rostedt, Ingo Molnar Cc: live-patching, linux-kernel, Jessica Yu As explained here [1], livepatch modules are failing to initialize properly because the ftrace coming module notifier (which calls ftrace_module_enable()) runs *after* the livepatch module notifier (which enables the patch(es)). Thus livepatch attempts to apply patches to modules before ftrace_module_enable() is even called for the corresponding module(s). As a result, patch modules break. Ftrace code must run before livepatch on module load, and the reverse is true on module unload. For ftrace and livepatch, order of initialization (plus exit/cleanup code) is important for loading and unloading modules, and using module notifiers to perform this work is not ideal since it is not always clear what gets called when. In this patchset, dependence on the module notifier call chain is removed in favor of hard coding the corresponding function calls in the module loader. This promotes better code visibility and ensures that ftrace and livepatch code get called in the correct order on patch module load and unload. Tested the changes with a test livepatch module that patches 9p and nilfs2, and verified that the issue described in [1] is fixed. Patches are based on linux-next. v1: https://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com v2: https://lkml.kernel.org/g/1454375856-27757-1-git-send-email-jeyu@redhat.com v3: https://lkml.kernel.org/g/1454728097-7106-1-git-send-email-jeyu@redhat.com v4: - Split part of complete_formation() into prepare_coming_module() to make error handling a bit easier - Minor tweak: change mod->state to going before calling going notifiers in the load_module error path - Swapped out obj->mod = NULL assignment in klp_module_coming() for a call to klp_free_object_loaded() v3: - Fix incorrect comments - Rename klp_module_{enable,disable} to klp_module_{coming,going} - Remove externs from livepatch.h - Fix error handling in kernel/module.c v2: - Instead of splitting the ftrace and livepatch notifiers into coming + going notifiers and adjusting their priorities, remove ftrace and livepatch notifiers completely and hard-code the necessary function calls in the module loader. [1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com Jessica Yu (4): modules: split part of complete_formation() into prepare_coming_module() modules: set mod->state to MODULE_STATE_GOING before going notifiers are called ftrace/module: remove ftrace module notifier livepatch/module: remove livepatch module notifier include/linux/ftrace.h | 6 +- include/linux/livepatch.h | 9 +++ kernel/livepatch/core.c | 145 ++++++++++++++++++++++------------------------ kernel/module.c | 40 ++++++++++--- kernel/trace/ftrace.c | 36 +----------- 5 files changed, 116 insertions(+), 120 deletions(-) -- 2.4.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-09 4:50 [PATCH v4 0/4] Fix ordering of ftrace/livepatch calls on module load and unload Jessica Yu @ 2016-02-09 4:50 ` Jessica Yu 2016-02-09 23:43 ` Rusty Russell 0 siblings, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-02-09 4:50 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Rusty Russell, Steven Rostedt, Ingo Molnar Cc: live-patching, linux-kernel, Jessica Yu 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. This fixes a notifier ordering issue in which the ftrace module notifier (and hence ftrace_module_enable()) for coming modules was being called after klp_module_notify(), which caused livepatch modules to initialize incorrectly. Signed-off-by: Jessica Yu <jeyu@redhat.com> Reviewed-by: Petr Mladek <pmladek@suse.com> Acked-by: Rusty Russell <rusty@rustcorp.com.au> --- include/linux/livepatch.h | 9 +++ kernel/livepatch/core.c | 145 ++++++++++++++++++++++------------------------ kernel/module.c | 9 +++ 3 files changed, 87 insertions(+), 76 deletions(-) diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index a882865..bd830d5 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *); int klp_enable_patch(struct klp_patch *); int klp_disable_patch(struct klp_patch *); +/* Called from the module loader during module coming/going states */ +int klp_module_coming(struct module *mod); +void klp_module_going(struct module *mod); + +#else /* !CONFIG_LIVEPATCH */ + +static inline int klp_module_coming(struct module *mod) { return 0; } +static inline void klp_module_going(struct module *mod) { } + #endif /* CONFIG_LIVEPATCH */ #endif /* _LINUX_LIVEPATCH_H_ */ diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index bc2c85c..e902377 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -99,12 +99,12 @@ static void klp_find_object_module(struct klp_object *obj) /* * We do not want to block removal of patched modules and therefore * we do not take a reference here. The patches are removed by - * a going module handler instead. + * klp_module_going() instead. */ mod = find_module(obj->name); /* - * Do not mess work of the module coming and going notifiers. - * Note that the patch might still be needed before the going handler + * Do not mess work of klp_module_coming() and klp_module_going(). + * Note that the patch might still be needed before klp_module_going() * is called. Module functions can be called even in the GOING state * until mod->exit() finishes. This is especially important for * patches that modify semantic of the functions. @@ -866,103 +866,106 @@ 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); klp_free_object_loaded(obj); + mutex_unlock(&klp_mutex); + + return ret; } -static int klp_module_notify(struct notifier_block *nb, unsigned long action, - void *data) +void klp_module_going(struct module *mod) { - int ret; - struct module *mod = data; struct klp_patch *patch; struct klp_object *obj; - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING) - return 0; + if (WARN_ON(mod->state != MODULE_STATE_GOING)) + return; mutex_lock(&klp_mutex); - /* - * Each module has to know that the notifier has been called. - * We never know what module will get patched by a new patch. + * Each module has to know that klp_module_going() + * has been called. We never know what module will + * get patched by a new patch. */ - if (action == MODULE_STATE_COMING) - mod->klp_alive = true; - else /* MODULE_STATE_GOING */ - mod->klp_alive = false; + mod->klp_alive = false; 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; - if (action == MODULE_STATE_COMING) { - obj->mod = mod; - ret = klp_module_notify_coming(patch, obj); - if (ret) { - obj->mod = NULL; - pr_warn("patch '%s' is in an inconsistent state!\n", - patch->mod->name); - } - } else /* MODULE_STATE_GOING */ - klp_module_notify_going(patch, obj); + if (patch->state != KLP_DISABLED) { + pr_notice("reverting patch '%s' on unloading module '%s'\n", + patch->mod->name, obj->mod->name); + klp_disable_object(obj); + } + klp_free_object_loaded(obj); break; } } mutex_unlock(&klp_mutex); - - return 0; } -static struct notifier_block klp_module_nb = { - .notifier_call = klp_module_notify, - .priority = INT_MIN+1, /* called late but before ftrace notifier */ -}; - static int __init klp_init(void) { int ret; @@ -973,21 +976,11 @@ static int __init klp_init(void) return -EINVAL; } - ret = register_module_notifier(&klp_module_nb); - if (ret) - return ret; - klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj); - if (!klp_root_kobj) { - ret = -ENOMEM; - goto unregister; - } + if (!klp_root_kobj) + return -ENOMEM; return 0; - -unregister: - unregister_module_notifier(&klp_module_nb); - return ret; } module_init(klp_init); diff --git a/kernel/module.c b/kernel/module.c index 0bd0077..e22d020 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -53,6 +53,7 @@ #include <asm/sections.h> #include <linux/tracepoint.h> #include <linux/ftrace.h> +#include <linux/livepatch.h> #include <linux/async.h> #include <linux/percpu.h> #include <linux/kmemleak.h> @@ -984,6 +985,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, mod->exit(); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); ftrace_release_mod(mod); async_synchronize_full(); @@ -3315,6 +3317,7 @@ fail: module_put(mod); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); ftrace_release_mod(mod); free_module(mod); wake_up_all(&module_wq); @@ -3371,12 +3374,17 @@ out_unlocked: static int prepare_coming_module(struct module *mod) { + int err; ftrace_module_enable(mod); + err = klp_module_coming(mod); + if (err) + return err; blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); return 0; + } static int complete_formation(struct module *mod, struct load_info *info) @@ -3556,6 +3564,7 @@ static int load_module(struct load_info *info, const char __user *uargs, mod->state = MODULE_STATE_GOING; blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); bug_cleanup: /* module_bug_cleanup needs module_mutex protection */ -- 2.4.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-09 4:50 ` [PATCH v4 4/4] livepatch/module: remove livepatch module notifier Jessica Yu @ 2016-02-09 23:43 ` Rusty Russell 2016-02-10 10:18 ` Jiri Kosina 0 siblings, 1 reply; 16+ messages in thread From: Rusty Russell @ 2016-02-09 23:43 UTC (permalink / raw) To: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar Cc: live-patching, linux-kernel, Jessica Yu Jessica Yu <jeyu@redhat.com> writes: > 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. > > This fixes a notifier ordering issue in which the ftrace module notifier > (and hence ftrace_module_enable()) for coming modules was being called > after klp_module_notify(), which caused livepatch modules to initialize > incorrectly. Without a Fixes: line, it's not absolutely clear whether this needs CC:stable, needs to go to Linus now, or can wait for the next merge window. I *think* you want all four merged this merge window, and 3 and 4 are required to fix a regression introduced since 4.4... Sorry for being obtuse! Rusty. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-09 23:43 ` Rusty Russell @ 2016-02-10 10:18 ` Jiri Kosina 2016-02-14 22:59 ` Jiri Kosina 2016-02-29 0:30 ` [PATCH v4 4/4] " Rusty Russell 0 siblings, 2 replies; 16+ messages in thread From: Jiri Kosina @ 2016-02-10 10:18 UTC (permalink / raw) To: Rusty Russell Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Wed, 10 Feb 2016, Rusty Russell 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. > > > > This fixes a notifier ordering issue in which the ftrace module notifier > > (and hence ftrace_module_enable()) for coming modules was being called > > after klp_module_notify(), which caused livepatch modules to initialize > > incorrectly. > > Without a Fixes: line, it's not absolutely clear whether this needs > CC:stable, needs to go to Linus now, or can wait for the next merge > window. > > I *think* you want all four merged this merge window, and 3 and 4 are > required to fix a regression introduced since 4.4... Your understanding is correct; #3 and #4 are needed to fix a 4.4 regression. It makes sense for the whole lot go to together, but for #1 and #2 I absolutely need your Ack before I take it to my tree, as I don't want to be merging this behind your back. Once you Ack #1 and #2, I plan to take this to Linus immediately so that we avoid doing these changes as very last minute. Thanks! -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-10 10:18 ` Jiri Kosina @ 2016-02-14 22:59 ` Jiri Kosina 2016-02-15 23:27 ` Josh Poimboeuf 2016-02-29 0:30 ` [PATCH v4 4/4] " Rusty Russell 1 sibling, 1 reply; 16+ messages in thread From: Jiri Kosina @ 2016-02-14 22:59 UTC (permalink / raw) To: Rusty Russell Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Wed, 10 Feb 2016, Jiri Kosina 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. > > > > > > This fixes a notifier ordering issue in which the ftrace module notifier > > > (and hence ftrace_module_enable()) for coming modules was being called > > > after klp_module_notify(), which caused livepatch modules to initialize > > > incorrectly. > > > > Without a Fixes: line, it's not absolutely clear whether this needs > > CC:stable, needs to go to Linus now, or can wait for the next merge > > window. > > > > I *think* you want all four merged this merge window, and 3 and 4 are > > required to fix a regression introduced since 4.4... > > Your understanding is correct; #3 and #4 are needed to fix a 4.4 > regression. It makes sense for the whole lot go to together, but for #1 > and #2 I absolutely need your Ack before I take it to my tree, as I don't > want to be merging this behind your back. > > Once you Ack #1 and #2, I plan to take this to Linus immediately so that > we avoid doing these changes as very last minute. Rusty, friendly ping? :) I know that this is quite tight timing, but I'd like to have the 4.4 regression fixed, and we are quite late in the -rc phase already. Thanks in advance, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-14 22:59 ` Jiri Kosina @ 2016-02-15 23:27 ` Josh Poimboeuf 2016-02-15 23:42 ` Jiri Kosina 0 siblings, 1 reply; 16+ messages in thread From: Josh Poimboeuf @ 2016-02-15 23:27 UTC (permalink / raw) To: Jiri Kosina Cc: Rusty Russell, Jessica Yu, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Sun, Feb 14, 2016 at 11:59:00PM +0100, Jiri Kosina wrote: > On Wed, 10 Feb 2016, Jiri Kosina 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. > > > > > > > > This fixes a notifier ordering issue in which the ftrace module notifier > > > > (and hence ftrace_module_enable()) for coming modules was being called > > > > after klp_module_notify(), which caused livepatch modules to initialize > > > > incorrectly. > > > > > > Without a Fixes: line, it's not absolutely clear whether this needs > > > CC:stable, needs to go to Linus now, or can wait for the next merge > > > window. > > > > > > I *think* you want all four merged this merge window, and 3 and 4 are > > > required to fix a regression introduced since 4.4... > > > > Your understanding is correct; #3 and #4 are needed to fix a 4.4 > > regression. It makes sense for the whole lot go to together, but for #1 > > and #2 I absolutely need your Ack before I take it to my tree, as I don't > > want to be merging this behind your back. > > > > Once you Ack #1 and #2, I plan to take this to Linus immediately so that > > we avoid doing these changes as very last minute. > > Rusty, friendly ping? :) > > I know that this is quite tight timing, but I'd like to have the 4.4 > regression fixed, and we are quite late in the -rc phase already. So I think the commit causing the regression is 5156dca34a3e, which occurred in the 4.5 cycle, *not* in 4.4. Also it's my understanding that only the third patch ("remove ftrace module notifier") is needed to fix the regression, and the other patches are just general improvements. So if needed I think we can just rebase that patch (which already has Rusty's ack I believe) and send it to Linus now. -- Josh ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-15 23:27 ` Josh Poimboeuf @ 2016-02-15 23:42 ` Jiri Kosina 2016-02-16 0:48 ` Jessica Yu 0 siblings, 1 reply; 16+ messages in thread From: Jiri Kosina @ 2016-02-15 23:42 UTC (permalink / raw) To: Josh Poimboeuf Cc: Rusty Russell, Jessica Yu, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Mon, 15 Feb 2016, Josh Poimboeuf wrote: > So I think the commit causing the regression is 5156dca34a3e, which > occurred in the 4.5 cycle, *not* in 4.4. Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e. regression that will become real issue once 4.5 is released. > Also it's my understanding that only the third patch ("remove ftrace > module notifier") is needed to fix the regression, and the other patches > are just general improvements. So if needed I think we can just rebase > that patch (which already has Rusty's ack I believe) and send it to > Linus now. 3/4 and 4/4 are be sufficient, yes (although I'd like to have this confimed by Jessica, as she apparently already has a reliable testcase). To be honest: I was skiing (and being offline) thursday - sunday :), so my original plan was to get Ack from Rusty in the meantime, and then send pull request to Linus once I am back on sunday evening. This is not going to happen, so we have to start with plan B (which is pushing just 3/4 and 4/4). Jessica, could you please send me updated (and tested on your side) patchset with module.c cleanups omitted? Steven, I'd appreciate if you could tell me whether your Ack to "ftrace/module: remove ftrace module notifier" still holds even if module.c changes are not happening. Thanks, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-15 23:42 ` Jiri Kosina @ 2016-02-16 0:48 ` Jessica Yu 2016-02-16 8:41 ` Miroslav Benes 0 siblings, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-02-16 0:48 UTC (permalink / raw) To: Jiri Kosina Cc: Josh Poimboeuf, Rusty Russell, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel +++ Jiri Kosina [16/02/16 00:42 +0100]: >On Mon, 15 Feb 2016, Josh Poimboeuf wrote: > >> So I think the commit causing the regression is 5156dca34a3e, which >> occurred in the 4.5 cycle, *not* in 4.4. > >Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e. >regression that will become real issue once 4.5 is released. > >> Also it's my understanding that only the third patch ("remove ftrace >> module notifier") is needed to fix the regression, and the other patches >> are just general improvements. So if needed I think we can just rebase >> that patch (which already has Rusty's ack I believe) and send it to >> Linus now. > >3/4 and 4/4 are be sufficient, yes (although I'd like to have this >confimed by Jessica, as she apparently already has a reliable testcase). Yes, so Josh is right; technically only patch 3/4 "ftrace/module: remove ftrace module notifier" is sufficient enough to fix the bug, and patch 4/4 is just a natural extension of that change. Since I'm going to be sending out another patchset anyway without the module.c cleanups, I'll just keep them together. Jessica >To be honest: I was skiing (and being offline) thursday - sunday :), so my >original plan was to get Ack from Rusty in the meantime, and then send >pull request to Linus once I am back on sunday evening. > >This is not going to happen, so we have to start with plan B (which is >pushing just 3/4 and 4/4). Jessica, could you please send me updated (and >tested on your side) patchset with module.c cleanups omitted? > >Steven, I'd appreciate if you could tell me whether your Ack to >"ftrace/module: remove ftrace module notifier" still holds even if >module.c changes are not happening. > >Thanks, > >-- >Jiri Kosina >SUSE Labs > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-16 0:48 ` Jessica Yu @ 2016-02-16 8:41 ` Miroslav Benes 2016-02-16 19:51 ` Jessica Yu 0 siblings, 1 reply; 16+ messages in thread From: Miroslav Benes @ 2016-02-16 8:41 UTC (permalink / raw) To: Jessica Yu Cc: Jiri Kosina, Josh Poimboeuf, Rusty Russell, Seth Jennings, Vojtech Pavlik, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Mon, 15 Feb 2016, Jessica Yu wrote: > +++ Jiri Kosina [16/02/16 00:42 +0100]: > > On Mon, 15 Feb 2016, Josh Poimboeuf wrote: > > > > > So I think the commit causing the regression is 5156dca34a3e, which > > > occurred in the 4.5 cycle, *not* in 4.4. > > > > Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e. > > regression that will become real issue once 4.5 is released. > > > > > Also it's my understanding that only the third patch ("remove ftrace > > > module notifier") is needed to fix the regression, and the other patches > > > are just general improvements. So if needed I think we can just rebase > > > that patch (which already has Rusty's ack I believe) and send it to > > > Linus now. > > > > 3/4 and 4/4 are be sufficient, yes (although I'd like to have this > > confimed by Jessica, as she apparently already has a reliable testcase). > > Yes, so Josh is right; technically only patch 3/4 "ftrace/module: > remove ftrace module notifier" is sufficient enough to fix the bug, > and patch 4/4 is just a natural extension of that change. Since I'm > going to be sending out another patchset anyway without the module.c > cleanups, I'll just keep them together. Yes, 3/4 should be sufficient to fix the bug. However if you take 4/4 too, you need 1/4 as well. Otherwise we would introduce a bug in error handling as Petr pointed out. Cheers, Miroslav ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-16 8:41 ` Miroslav Benes @ 2016-02-16 19:51 ` Jessica Yu 0 siblings, 0 replies; 16+ messages in thread From: Jessica Yu @ 2016-02-16 19:51 UTC (permalink / raw) To: Miroslav Benes Cc: Jiri Kosina, Josh Poimboeuf, Rusty Russell, Seth Jennings, Vojtech Pavlik, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel +++ Miroslav Benes [16/02/16 09:41 +0100]: >On Mon, 15 Feb 2016, Jessica Yu wrote: > >> +++ Jiri Kosina [16/02/16 00:42 +0100]: >> > On Mon, 15 Feb 2016, Josh Poimboeuf wrote: >> > >> > > So I think the commit causing the regression is 5156dca34a3e, which >> > > occurred in the 4.5 cycle, *not* in 4.4. >> > >> > Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e. >> > regression that will become real issue once 4.5 is released. >> > >> > > Also it's my understanding that only the third patch ("remove ftrace >> > > module notifier") is needed to fix the regression, and the other patches >> > > are just general improvements. So if needed I think we can just rebase >> > > that patch (which already has Rusty's ack I believe) and send it to >> > > Linus now. >> > >> > 3/4 and 4/4 are be sufficient, yes (although I'd like to have this >> > confimed by Jessica, as she apparently already has a reliable testcase). >> >> Yes, so Josh is right; technically only patch 3/4 "ftrace/module: >> remove ftrace module notifier" is sufficient enough to fix the bug, >> and patch 4/4 is just a natural extension of that change. Since I'm >> going to be sending out another patchset anyway without the module.c >> cleanups, I'll just keep them together. > >Yes, 3/4 should be sufficient to fix the bug. However if you take 4/4 too, >you need 1/4 as well. Otherwise we would introduce a bug in error handling >as Petr pointed out. > Hm. I am just realizing that patch 4/4 will still need new ACK's for the error handling portion. What I'll do is, after testing, send out patch 3/4 ("ftrace/module: remove ftrace module notifier") as a standalone patch to be merged immediately, since it fixes an actual bug. The rest of this patchset will follow separately and can be reviewed at its own pace. Jessica ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier 2016-02-10 10:18 ` Jiri Kosina 2016-02-14 22:59 ` Jiri Kosina @ 2016-02-29 0:30 ` Rusty Russell 2016-03-01 3:00 ` Jessica Yu 1 sibling, 1 reply; 16+ messages in thread From: Rusty Russell @ 2016-02-29 0:30 UTC (permalink / raw) To: Jiri Kosina Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel Jiri Kosina <jikos@kernel.org> writes: > On Wed, 10 Feb 2016, Rusty Russell 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. >> > >> > This fixes a notifier ordering issue in which the ftrace module notifier >> > (and hence ftrace_module_enable()) for coming modules was being called >> > after klp_module_notify(), which caused livepatch modules to initialize >> > incorrectly. >> >> Without a Fixes: line, it's not absolutely clear whether this needs >> CC:stable, needs to go to Linus now, or can wait for the next merge >> window. >> >> I *think* you want all four merged this merge window, and 3 and 4 are >> required to fix a regression introduced since 4.4... > > Your understanding is correct; #3 and #4 are needed to fix a 4.4 > regression. It makes sense for the whole lot go to together, but for #1 > and #2 I absolutely need your Ack before I take it to my tree, as I don't > want to be merging this behind your back. > > Once you Ack #1 and #2, I plan to take this to Linus immediately so that > we avoid doing these changes as very last minute. Sorry Jiri, I am on paternity leave. Am happy with all these patches; please use your best judgement: Acked-by: Rusty Russell <rusty@rustcorp.com.au> Thanks, Rusty. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-29 0:30 ` [PATCH v4 4/4] " Rusty Russell @ 2016-03-01 3:00 ` Jessica Yu 0 siblings, 0 replies; 16+ messages in thread From: Jessica Yu @ 2016-03-01 3:00 UTC (permalink / raw) To: Rusty Russell Cc: Jiri Kosina, Josh Poimboeuf, Seth Jennings, Vojtech Pavlik, Miroslav Benes, Petr Mladek, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel +++ Rusty Russell [29/02/16 11:00 +1030]: >Jiri Kosina <jikos@kernel.org> writes: >> On Wed, 10 Feb 2016, Rusty Russell 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. >>> > >>> > This fixes a notifier ordering issue in which the ftrace module notifier >>> > (and hence ftrace_module_enable()) for coming modules was being called >>> > after klp_module_notify(), which caused livepatch modules to initialize >>> > incorrectly. >>> >>> Without a Fixes: line, it's not absolutely clear whether this needs >>> CC:stable, needs to go to Linus now, or can wait for the next merge >>> window. >>> >>> I *think* you want all four merged this merge window, and 3 and 4 are >>> required to fix a regression introduced since 4.4... >> >> Your understanding is correct; #3 and #4 are needed to fix a 4.4 >> regression. It makes sense for the whole lot go to together, but for #1 >> and #2 I absolutely need your Ack before I take it to my tree, as I don't >> want to be merging this behind your back. >> >> Once you Ack #1 and #2, I plan to take this to Linus immediately so that >> we avoid doing these changes as very last minute. > >Sorry Jiri, I am on paternity leave. Am happy with all these patches; >please use your best judgement: > >Acked-by: Rusty Russell <rusty@rustcorp.com.au> > Thanks Rusty. No worries, we got the regression out of the way already. :-) Jiri, I'll resend the remaining 3 patches in the series separately, since we've merged the ftrace patch already. Jessica ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload @ 2016-02-02 1:17 Jessica Yu 2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu 0 siblings, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-02-02 1:17 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar Cc: live-patching, linux-kernel, Jessica Yu As explained here [1], livepatch modules are failing to initialize properly because the ftrace coming module notifier (which calls ftrace_module_enable()) runs *after* the livepatch module notifier (which enables the patch(es)). Thus livepatch attempts to apply patches to modules before ftrace_module_enable() is even called for the corresponding module(s). As a result, patch modules break. Ftrace code must run before livepatch on module load, and the reverse is true on module unload. For ftrace and livepatch, order of initialization (plus exit/cleanup code) is important for loading and unloading modules, and using module notifiers to perform this work is not ideal since it is not always clear what gets called when. In this patchset, dependence on the module notifier call chain is removed in favor of hard coding the corresponding function calls in the module loader. This promotes better code visibility and ensures that ftrace and livepatch code get called in the correct order on patch module load and unload. Tested the changes with a test livepatch module that patches 9p and nilfs2, and verified that the issue described in [1] is fixed. Patches are based on linux-next. v1 can be found here - http://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com v2: - Instead of splitting the ftrace and livepatch notifiers into coming + going notifiers and adjusting their priorities, remove ftrace and livepatch notifiers completely and hard-code the necessary function calls in the module loader. [1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com Jessica Yu (2): ftrace/module: remove ftrace module notifier livepatch/module: remove livepatch module notifier include/linux/ftrace.h | 6 +- include/linux/livepatch.h | 9 +++ kernel/livepatch/core.c | 144 ++++++++++++++++++++++------------------------ kernel/module.c | 12 ++++ kernel/trace/ftrace.c | 36 +----------- 5 files changed, 95 insertions(+), 112 deletions(-) -- 2.4.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] livepatch/module: remove livepatch module notifier 2016-02-02 1:17 [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jessica Yu @ 2016-02-02 1:17 ` Jessica Yu 2016-02-04 14:39 ` Petr Mladek 0 siblings, 1 reply; 16+ messages in thread From: Jessica Yu @ 2016-02-02 1:17 UTC (permalink / raw) To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar Cc: live-patching, linux-kernel, Jessica Yu 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_enable() during module load, and that klp_module_disable() 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. This fixes a notifier ordering issue in which the ftrace module notifier (and hence ftrace_module_enable()) for coming modules was being called after klp_module_notify(), which caused livepatch modules to initialize incorrectly. Signed-off-by: Jessica Yu <jeyu@redhat.com> --- include/linux/livepatch.h | 9 +++ kernel/livepatch/core.c | 144 ++++++++++++++++++++++------------------------ kernel/module.c | 8 +++ 3 files changed, 86 insertions(+), 75 deletions(-) diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index a882865..fdd5f1c 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *); int klp_enable_patch(struct klp_patch *); int klp_disable_patch(struct klp_patch *); +/* Called from the module loader during module coming/going states */ +extern int klp_module_enable(struct module *mod); +extern void klp_module_disable(struct module *mod); + +#else /* !CONFIG_LIVEPATCH */ + +static inline int klp_module_enable(struct module *mod) { return 0; } +static inline void klp_module_disable(struct module *mod) { } + #endif /* CONFIG_LIVEPATCH */ #endif /* _LINUX_LIVEPATCH_H_ */ diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index bc2c85c..7aa975d 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj) */ mod = find_module(obj->name); /* - * Do not mess work of the module coming and going notifiers. + * Do not mess work of the klp module coming and going handlers. * Note that the patch might still be needed before the going handler * is called. Module functions can be called even in the GOING state * until mod->exit() finishes. This is especially important for @@ -866,103 +866,107 @@ 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) +/* Called when module state is MODULE_STATE_COMING */ +int klp_module_enable(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 (patch->state == KLP_DISABLED) + if (mod->state != MODULE_STATE_COMING) return 0; - pr_notice("applying patch '%s' to loading module '%s'\n", - pmod->name, mod->name); + mutex_lock(&klp_mutex); + /* + * Each module has to know that the coming handler has + * been called. We never know what module will get + * patched by a new patch. + */ + mod->klp_alive = true; - 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; -} + 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; -static void klp_module_notify_going(struct klp_patch *patch, - struct klp_object *obj) -{ - struct module *pmod = patch->mod; - struct module *mod = obj->mod; + obj->mod = mod; - if (patch->state == KLP_DISABLED) - goto disabled; + 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) + break; - pr_notice("reverting patch '%s' on unloading module '%s'\n", - pmod->name, mod->name); + pr_notice("applying patch '%s' to loading module '%s'\n", + patch->mod->name, obj->mod->name); - klp_disable_object(obj); + 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; + } + } + + mutex_unlock(&klp_mutex); -disabled: - klp_free_object_loaded(obj); + return 0; + +err: + /* + * If a patch is unsuccessfully applied, return + * error to the module loader. + */ + obj->mod = NULL; + pr_warn("patch '%s' is in an inconsistent state!\n", patch->mod->name); + mutex_unlock(&klp_mutex); + + return ret; } -static int klp_module_notify(struct notifier_block *nb, unsigned long action, - void *data) +/* Called when module state is MODULE_STATE_GOING */ +void klp_module_disable(struct module *mod) { - int ret; - struct module *mod = data; struct klp_patch *patch; struct klp_object *obj; - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING) - return 0; + if (mod->state != MODULE_STATE_GOING) + return; mutex_lock(&klp_mutex); - /* - * Each module has to know that the notifier has been called. - * We never know what module will get patched by a new patch. + * Each module has to know that the going handler + * has been called. We never know what module will + * get patched by a new patch. */ - if (action == MODULE_STATE_COMING) - mod->klp_alive = true; - else /* MODULE_STATE_GOING */ - mod->klp_alive = false; + mod->klp_alive = false; 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; - if (action == MODULE_STATE_COMING) { - obj->mod = mod; - ret = klp_module_notify_coming(patch, obj); - if (ret) { - obj->mod = NULL; - pr_warn("patch '%s' is in an inconsistent state!\n", - patch->mod->name); - } - } else /* MODULE_STATE_GOING */ - klp_module_notify_going(patch, obj); + if (patch->state != KLP_DISABLED) { + pr_notice("reverting patch '%s' on unloading module '%s'\n", + patch->mod->name, obj->mod->name); + klp_disable_object(obj); + } + klp_free_object_loaded(obj); break; } } mutex_unlock(&klp_mutex); - - return 0; } -static struct notifier_block klp_module_nb = { - .notifier_call = klp_module_notify, - .priority = INT_MIN+1, /* called late but before ftrace notifier */ -}; - static int __init klp_init(void) { int ret; @@ -973,21 +977,11 @@ static int __init klp_init(void) return -EINVAL; } - ret = register_module_notifier(&klp_module_nb); - if (ret) - return ret; - klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj); - if (!klp_root_kobj) { - ret = -ENOMEM; - goto unregister; - } + if (!klp_root_kobj) + return -ENOMEM; return 0; - -unregister: - unregister_module_notifier(&klp_module_nb); - return ret; } module_init(klp_init); diff --git a/kernel/module.c b/kernel/module.c index b05d466..71c77ed 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -53,6 +53,7 @@ #include <asm/sections.h> #include <linux/tracepoint.h> #include <linux/ftrace.h> +#include <linux/livepatch.h> #include <linux/async.h> #include <linux/percpu.h> #include <linux/kmemleak.h> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, mod->exit(); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_disable(mod); ftrace_release_mod(mod); async_synchronize_full(); @@ -3297,6 +3299,7 @@ fail: module_put(mod); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_disable(mod); ftrace_release_mod(mod); free_module(mod); wake_up_all(&module_wq); @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info) mutex_unlock(&module_mutex); ftrace_module_enable(mod); + err = klp_module_enable(mod); + if (err) + goto out; + blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); return 0; @@ -3531,6 +3538,7 @@ static int load_module(struct load_info *info, const char __user *uargs, blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_disable(mod); /* we can't deallocate the module until we clear memory protection */ module_disable_ro(mod); -- 2.4.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier 2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu @ 2016-02-04 14:39 ` Petr Mladek 2016-02-05 4:11 ` Jessica Yu 0 siblings, 1 reply; 16+ messages in thread From: Petr Mladek @ 2016-02-04 14:39 UTC (permalink / raw) To: Jessica Yu Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Mon 2016-02-01 20:17:36, 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_enable() during module load, and that klp_module_disable() 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. > > This fixes a notifier ordering issue in which the ftrace module notifier > (and hence ftrace_module_enable()) for coming modules was being called > after klp_module_notify(), which caused livepatch modules to initialize > incorrectly. > > Signed-off-by: Jessica Yu <jeyu@redhat.com> > --- > include/linux/livepatch.h | 9 +++ > kernel/livepatch/core.c | 144 ++++++++++++++++++++++------------------------ > kernel/module.c | 8 +++ > 3 files changed, 86 insertions(+), 75 deletions(-) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index a882865..fdd5f1c 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *); > int klp_enable_patch(struct klp_patch *); > int klp_disable_patch(struct klp_patch *); > > +/* Called from the module loader during module coming/going states */ > +extern int klp_module_enable(struct module *mod); > +extern void klp_module_disable(struct module *mod); > + > +#else /* !CONFIG_LIVEPATCH */ > + > +static inline int klp_module_enable(struct module *mod) { return 0; } > +static inline void klp_module_disable(struct module *mod) { } > + > #endif /* CONFIG_LIVEPATCH */ > > #endif /* _LINUX_LIVEPATCH_H_ */ > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index bc2c85c..7aa975d 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj) > */ > mod = find_module(obj->name); > /* > - * Do not mess work of the module coming and going notifiers. > + * Do not mess work of the klp module coming and going handlers. This is a bit confusing because you removed all functions called *coming* and *going*. I would say something like: * Do not mess work of klp_module_enable() and klp_module_disable(). > * Note that the patch might still be needed before the going handler > * is called. Module functions can be called even in the GOING state > * until mod->exit() finishes. This is especially important for > @@ -866,103 +866,107 @@ 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) > +/* Called when module state is MODULE_STATE_COMING */ > +int klp_module_enable(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 (patch->state == KLP_DISABLED) > + if (mod->state != MODULE_STATE_COMING) > return 0; The function is not longer called from another state. I would replace this by: if (WARN_ON(mod->state != MODULE_STATE_COMING)) return -EINVAL; > - pr_notice("applying patch '%s' to loading module '%s'\n", > - pmod->name, mod->name); > + mutex_lock(&klp_mutex); > + /* > + * Each module has to know that the coming handler has > + * been called. We never know what module will get > + * patched by a new patch. > + */ > + mod->klp_alive = true; > > - 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; > -} > + 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; > > -static void klp_module_notify_going(struct klp_patch *patch, > - struct klp_object *obj) > -{ > - struct module *pmod = patch->mod; > - struct module *mod = obj->mod; > + obj->mod = mod; > > - if (patch->state == KLP_DISABLED) > - goto disabled; > + 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) > + break; > > - pr_notice("reverting patch '%s' on unloading module '%s'\n", > - pmod->name, mod->name); > + pr_notice("applying patch '%s' to loading module '%s'\n", > + patch->mod->name, obj->mod->name); > > - klp_disable_object(obj); > + 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; > + } > + } > + > + mutex_unlock(&klp_mutex); > > -disabled: > - klp_free_object_loaded(obj); > + return 0; > + > +err: > + /* > + * If a patch is unsuccessfully applied, return > + * error to the module loader. > + */ > + obj->mod = NULL; > + pr_warn("patch '%s' is in an inconsistent state!\n", patch->mod->name); This message is not correct. The module will not get loaded when the patch is not applied. Instead, we need to revert all the operations that has already been done for this module. Note that the module stayed loaded before, so we did not need to release any memory or revert any ftrace call registration but we need to do so now! > + mutex_unlock(&klp_mutex); > + > + return ret; > } > > -static int klp_module_notify(struct notifier_block *nb, unsigned long action, > - void *data) > +/* Called when module state is MODULE_STATE_GOING */ > +void klp_module_disable(struct module *mod) > { > - int ret; > - struct module *mod = data; > struct klp_patch *patch; > struct klp_object *obj; > > - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING) > - return 0; > + if (mod->state != MODULE_STATE_GOING) > + return; > > mutex_lock(&klp_mutex); > - > /* > - * Each module has to know that the notifier has been called. > - * We never know what module will get patched by a new patch. > + * Each module has to know that the going handler > + * has been called. We never know what module will > + * get patched by a new patch. > */ > - if (action == MODULE_STATE_COMING) > - mod->klp_alive = true; > - else /* MODULE_STATE_GOING */ > - mod->klp_alive = false; > + mod->klp_alive = false; > > 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; > > - if (action == MODULE_STATE_COMING) { > - obj->mod = mod; > - ret = klp_module_notify_coming(patch, obj); > - if (ret) { > - obj->mod = NULL; > - pr_warn("patch '%s' is in an inconsistent state!\n", > - patch->mod->name); > - } > - } else /* MODULE_STATE_GOING */ > - klp_module_notify_going(patch, obj); > + if (patch->state != KLP_DISABLED) { > + pr_notice("reverting patch '%s' on unloading module '%s'\n", > + patch->mod->name, obj->mod->name); > + klp_disable_object(obj); > + } > > + klp_free_object_loaded(obj); > break; > } > } > > mutex_unlock(&klp_mutex); > - > - return 0; > } > > -static struct notifier_block klp_module_nb = { > - .notifier_call = klp_module_notify, > - .priority = INT_MIN+1, /* called late but before ftrace notifier */ > -}; > - > static int __init klp_init(void) > { > int ret; > @@ -973,21 +977,11 @@ static int __init klp_init(void) > return -EINVAL; > } > > - ret = register_module_notifier(&klp_module_nb); > - if (ret) > - return ret; > - > klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj); > - if (!klp_root_kobj) { > - ret = -ENOMEM; > - goto unregister; > - } > + if (!klp_root_kobj) > + return -ENOMEM; > > return 0; > - > -unregister: > - unregister_module_notifier(&klp_module_nb); > - return ret; > } > > module_init(klp_init); > diff --git a/kernel/module.c b/kernel/module.c > index b05d466..71c77ed 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -53,6 +53,7 @@ > #include <asm/sections.h> > #include <linux/tracepoint.h> > #include <linux/ftrace.h> > +#include <linux/livepatch.h> > #include <linux/async.h> > #include <linux/percpu.h> > #include <linux/kmemleak.h> > @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, > mod->exit(); > blocking_notifier_call_chain(&module_notify_list, > MODULE_STATE_GOING, mod); > + klp_module_disable(mod); > ftrace_release_mod(mod); > > async_synchronize_full(); > @@ -3297,6 +3299,7 @@ fail: > module_put(mod); > blocking_notifier_call_chain(&module_notify_list, > MODULE_STATE_GOING, mod); > + klp_module_disable(mod); > ftrace_release_mod(mod); > free_module(mod); > wake_up_all(&module_wq); > @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info) > mutex_unlock(&module_mutex); > > ftrace_module_enable(mod); > + err = klp_module_enable(mod); > + if (err) > + goto out; If you go out here, you need to revert some some operations that are normally done in the bug_cleanup: goto target in load_module(). In particular, you need to do: /* module_bug_cleanup needs module_mutex protection */ mutex_lock(&module_mutex); module_bug_cleanup(mod); mutex_unlock(&module_mutex); ftrace_release_mod(mod); /* we can't deallocate the module until we clear memory protection */ module_disable_ro(mod); module_disable_nx(mod); IMHO, it would make sense to somehow split the complete_formation() function and avoid a code duplication in the error paths. Best Regards, Petr ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-04 14:39 ` Petr Mladek @ 2016-02-05 4:11 ` Jessica Yu 2016-02-05 9:15 ` Miroslav Benes 2016-02-08 0:34 ` Rusty Russell 0 siblings, 2 replies; 16+ messages in thread From: Jessica Yu @ 2016-02-05 4:11 UTC (permalink / raw) To: Petr Mladek Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel +++ Petr Mladek [04/02/16 15:39 +0100]: >On Mon 2016-02-01 20:17:36, Jessica Yu wrote: [ snipped since email is getting long ] >> diff --git a/kernel/module.c b/kernel/module.c >> index b05d466..71c77ed 100644 >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -53,6 +53,7 @@ >> #include <asm/sections.h> >> #include <linux/tracepoint.h> >> #include <linux/ftrace.h> >> +#include <linux/livepatch.h> >> #include <linux/async.h> >> #include <linux/percpu.h> >> #include <linux/kmemleak.h> >> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, >> mod->exit(); >> blocking_notifier_call_chain(&module_notify_list, >> MODULE_STATE_GOING, mod); >> + klp_module_disable(mod); >> ftrace_release_mod(mod); >> >> async_synchronize_full(); >> @@ -3297,6 +3299,7 @@ fail: >> module_put(mod); >> blocking_notifier_call_chain(&module_notify_list, >> MODULE_STATE_GOING, mod); >> + klp_module_disable(mod); >> ftrace_release_mod(mod); >> free_module(mod); >> wake_up_all(&module_wq); >> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info) >> mutex_unlock(&module_mutex); >> >> ftrace_module_enable(mod); >> + err = klp_module_enable(mod); >> + if (err) >> + goto out; > >If you go out here, you need to revert some some operations >that are normally done in the bug_cleanup: goto target >in load_module(). In particular, you need to do: > > /* module_bug_cleanup needs module_mutex protection */ > mutex_lock(&module_mutex); > module_bug_cleanup(mod); > mutex_unlock(&module_mutex); > > ftrace_release_mod(mod); > > /* we can't deallocate the module until we clear memory protection */ > module_disable_ro(mod); > module_disable_nx(mod); > > >IMHO, it would make sense to somehow split the complete_formation() function >and avoid a code duplication in the error paths. Argh, thank you for catching that. I think we could split up complete_formation() into two functions in order to make the error handling work. We could probably take out the coming notifier calls, ftrace_module_enable(), and klp_module_enable() out of complete_formation(), and put them in another function, maybe called prepare_coming_module(), that would be called right after complete_formation(). It might look something like this: @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs, err = complete_formation(mod, info); if (err) goto ddebug_cleanup; + err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers + if (err) // means that klp_module_enable failed + goto bug_cleanup; /* Module is ready to execute: parsing args may do that. */ after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs, unknown_module_param_cb); if (IS_ERR(after_dashes)) { err = PTR_ERR(after_dashes); - goto bug_cleanup; + goto coming_cleanup; } else if (after_dashes) { pr_warn("%s: parameters '%s' after `--' ignored\n", mod->name, after_dashes); Now for the error conditions. If complete_formation() fails, goto ddebug_cleanup. If prepare_coming_module() fails (at that point, module_enable_{ro,nx} and module_bug_finalize() have already finished), goto bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable, ftrace_module_enable, and the coming notifiers have finished) goto coming_cleanup. ftrace_release_mod() gets called in the goto free_module label so we don't have to call it in coming_module. @@ -3649,16 +3659,16 @@ static int load_module(struct load_info *info, const char __user *uargs, + coming_cleanup: + blocking_notifier_call_chain(&module_notify_list, + MODULE_STATE_GOING, mod); + klp_module_disable(mod); bug_cleanup: /* module_bug_cleanup needs module_mutex protection */ mutex_lock(&module_mutex); module_bug_cleanup(mod); mutex_unlock(&module_mutex); - blocking_notifier_call_chain(&module_notify_list, - MODULE_STATE_GOING, mod); - klp_module_disable(mod); - /* we can't deallocate the module until we clear memory protection */ module_disable_ro(mod); module_disable_nx(mod); Does all this look ok? Also, one last thing, I noticed that module->state isn't set to MODULE_STATE_GOING anywhere before the going notifier chain is called in the bug_cleanup label (I think it is still COMING at that point), so the klp_module_disable call right afterwards would have bailed out because of that. To be consistent, shouldn't it be set before the going notifiers are called? Thanks, Jessica ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-05 4:11 ` Jessica Yu @ 2016-02-05 9:15 ` Miroslav Benes 2016-02-05 10:06 ` Petr Mladek 2016-02-08 0:34 ` Rusty Russell 1 sibling, 1 reply; 16+ messages in thread From: Miroslav Benes @ 2016-02-05 9:15 UTC (permalink / raw) To: Jessica Yu Cc: Petr Mladek, Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Thu, 4 Feb 2016, Jessica Yu wrote: > +++ Petr Mladek [04/02/16 15:39 +0100]: > > On Mon 2016-02-01 20:17:36, Jessica Yu wrote: > [ snipped since email is getting long ] > > > diff --git a/kernel/module.c b/kernel/module.c > > > index b05d466..71c77ed 100644 > > > --- a/kernel/module.c > > > +++ b/kernel/module.c > > > @@ -53,6 +53,7 @@ > > > #include <asm/sections.h> > > > #include <linux/tracepoint.h> > > > #include <linux/ftrace.h> > > > +#include <linux/livepatch.h> > > > #include <linux/async.h> > > > #include <linux/percpu.h> > > > #include <linux/kmemleak.h> > > > @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, > > > name_user, > > > mod->exit(); > > > blocking_notifier_call_chain(&module_notify_list, > > > MODULE_STATE_GOING, mod); > > > + klp_module_disable(mod); > > > ftrace_release_mod(mod); > > > > > > async_synchronize_full(); > > > @@ -3297,6 +3299,7 @@ fail: > > > module_put(mod); > > > blocking_notifier_call_chain(&module_notify_list, > > > MODULE_STATE_GOING, mod); > > > + klp_module_disable(mod); > > > ftrace_release_mod(mod); > > > free_module(mod); > > > wake_up_all(&module_wq); > > > @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, > > > struct load_info *info) > > > mutex_unlock(&module_mutex); > > > > > > ftrace_module_enable(mod); > > > + err = klp_module_enable(mod); > > > + if (err) > > > + goto out; > > > > If you go out here, you need to revert some some operations > > that are normally done in the bug_cleanup: goto target > > in load_module(). In particular, you need to do: > > > > /* module_bug_cleanup needs module_mutex protection */ > > mutex_lock(&module_mutex); > > module_bug_cleanup(mod); > > mutex_unlock(&module_mutex); > > > > ftrace_release_mod(mod); > > > > /* we can't deallocate the module until we clear memory protection */ > > module_disable_ro(mod); > > module_disable_nx(mod); > > > > > > IMHO, it would make sense to somehow split the complete_formation() function > > and avoid a code duplication in the error paths. > > Argh, thank you for catching that. I think we could split up > complete_formation() > into two functions in order to make the error handling work. > We could probably take out the coming notifier calls, ftrace_module_enable(), > and klp_module_enable() out of complete_formation(), and put them in another > function, maybe called prepare_coming_module(), that would be called right > after complete_formation(). It might look something like this: > > @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const > char __user *uargs, > err = complete_formation(mod, info); if (err) > goto ddebug_cleanup; > + err = prepare_coming_module(mod); // calls ftrace_module_enable(), > klp_module_enable(), then coming notifiers > + if (err) // means that klp_module_enable failed > + goto bug_cleanup; > > /* Module is ready to execute: parsing args may do that. */ > after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, > @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const > char __user *uargs, > unknown_module_param_cb); > if (IS_ERR(after_dashes)) { > err = PTR_ERR(after_dashes); > - goto bug_cleanup; > + goto coming_cleanup; > } else if (after_dashes) { > pr_warn("%s: parameters '%s' after `--' ignored\n", > mod->name, after_dashes); > > Now for the error conditions. If complete_formation() fails, goto > ddebug_cleanup. If prepare_coming_module() fails (at that point, > module_enable_{ro,nx} and module_bug_finalize() have already finished), goto > bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable, > ftrace_module_enable, and the coming notifiers have finished) goto > coming_cleanup. ftrace_release_mod() gets called in the goto free_module label > so we don't have to call it in coming_module. > > @@ -3649,16 +3659,16 @@ static int load_module(struct load_info *info, const > char __user *uargs, > > + coming_cleanup: > + blocking_notifier_call_chain(&module_notify_list, > + MODULE_STATE_GOING, mod); > + klp_module_disable(mod); > bug_cleanup: > /* module_bug_cleanup needs module_mutex protection */ > mutex_lock(&module_mutex); > module_bug_cleanup(mod); > mutex_unlock(&module_mutex); > > - blocking_notifier_call_chain(&module_notify_list, > - MODULE_STATE_GOING, mod); > - klp_module_disable(mod); > - > /* we can't deallocate the module until we clear memory protection */ > module_disable_ro(mod); > module_disable_nx(mod); > > Does all this look ok? Hm, there is an another option. We can cover the needed error handling in complete_formation(). So for the first error there (verify_export_symbols() fails) we need to only release module_mutex. For our second error we need more. We would call module_bug_cleanup() under module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct? It would be hidden in complete_formation() this way and cleaner in my opinion. There is some code duplication though. > Also, one last thing, I noticed that module->state > isn't > set to MODULE_STATE_GOING anywhere before the going notifier chain is called > in > the bug_cleanup label (I think it is still COMING at that point), so the > klp_module_disable call right afterwards would have bailed out because of > that. > To be consistent, shouldn't it be set before the going notifiers are called? This could break something or introduce a race somewhere. Git grep says there are several checks for MODULE_STATE_GOING through out the kernel which need to be checked. Regards, Miroslav ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-05 9:15 ` Miroslav Benes @ 2016-02-05 10:06 ` Petr Mladek 0 siblings, 0 replies; 16+ messages in thread From: Petr Mladek @ 2016-02-05 10:06 UTC (permalink / raw) To: Miroslav Benes Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel On Fri 2016-02-05 10:15:56, Miroslav Benes wrote: > On Thu, 4 Feb 2016, Jessica Yu wrote: > > Argh, thank you for catching that. I think we could split up > > complete_formation() > > into two functions in order to make the error handling work. > > > > Does all this look ok? > > Hm, there is an another option. We can cover the needed error handling in > complete_formation(). So for the first error there > (verify_export_symbols() fails) we need to only release module_mutex. For > our second error we need more. We would call module_bug_cleanup() under > module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct? > It would be hidden in complete_formation() this way and cleaner in my > opinion. There is some code duplication though. This sounds better to me. > > Also, one last thing, I noticed that module->state > > isn't > > set to MODULE_STATE_GOING anywhere before the going notifier chain is called > > in > > the bug_cleanup label (I think it is still COMING at that point), so the > > klp_module_disable call right afterwards would have bailed out because of > > that. > > To be consistent, shouldn't it be set before the going notifiers are called? > > This could break something or introduce a race somewhere. Git grep says > there are several checks for MODULE_STATE_GOING through out the kernel > which need to be checked. I think that we could relax the condition in the klp_module_going/disable() function and allow to call it also in MODULE_STATE_COMMING_STATE. It would deserve a comment. Best Regards, Petr ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: livepatch/module: remove livepatch module notifier 2016-02-05 4:11 ` Jessica Yu 2016-02-05 9:15 ` Miroslav Benes @ 2016-02-08 0:34 ` Rusty Russell 1 sibling, 0 replies; 16+ messages in thread From: Rusty Russell @ 2016-02-08 0:34 UTC (permalink / raw) To: Jessica Yu, Petr Mladek Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes, Steven Rostedt, Ingo Molnar, live-patching, linux-kernel Jessica Yu <jeyu@redhat.com> writes: > +++ Petr Mladek [04/02/16 15:39 +0100]: >>On Mon 2016-02-01 20:17:36, Jessica Yu wrote: > [ snipped since email is getting long ] >>> diff --git a/kernel/module.c b/kernel/module.c >>> index b05d466..71c77ed 100644 >>> --- a/kernel/module.c >>> +++ b/kernel/module.c >>> @@ -53,6 +53,7 @@ >>> #include <asm/sections.h> >>> #include <linux/tracepoint.h> >>> #include <linux/ftrace.h> >>> +#include <linux/livepatch.h> >>> #include <linux/async.h> >>> #include <linux/percpu.h> >>> #include <linux/kmemleak.h> >>> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, >>> mod->exit(); >>> blocking_notifier_call_chain(&module_notify_list, >>> MODULE_STATE_GOING, mod); >>> + klp_module_disable(mod); >>> ftrace_release_mod(mod); >>> >>> async_synchronize_full(); >>> @@ -3297,6 +3299,7 @@ fail: >>> module_put(mod); >>> blocking_notifier_call_chain(&module_notify_list, >>> MODULE_STATE_GOING, mod); >>> + klp_module_disable(mod); >>> ftrace_release_mod(mod); >>> free_module(mod); >>> wake_up_all(&module_wq); >>> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info) >>> mutex_unlock(&module_mutex); >>> >>> ftrace_module_enable(mod); >>> + err = klp_module_enable(mod); >>> + if (err) >>> + goto out; >> >>If you go out here, you need to revert some some operations >>that are normally done in the bug_cleanup: goto target >>in load_module(). In particular, you need to do: >> >> /* module_bug_cleanup needs module_mutex protection */ >> mutex_lock(&module_mutex); >> module_bug_cleanup(mod); >> mutex_unlock(&module_mutex); >> >> ftrace_release_mod(mod); >> >> /* we can't deallocate the module until we clear memory protection */ >> module_disable_ro(mod); >> module_disable_nx(mod); >> >> >>IMHO, it would make sense to somehow split the complete_formation() function >>and avoid a code duplication in the error paths. > > Argh, thank you for catching that. I think we could split up complete_formation() > into two functions in order to make the error handling work. > > We could probably take out the coming notifier calls, ftrace_module_enable(), > and klp_module_enable() out of complete_formation(), and put them in another > function, maybe called prepare_coming_module(), that would be called right > after complete_formation(). It might look something like this: > > @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs, > err = complete_formation(mod, info); > if (err) > goto ddebug_cleanup; > + err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers > + if (err) // means that klp_module_enable failed > + goto bug_cleanup; > > /* Module is ready to execute: parsing args may do that. */ > after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, > @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs, > unknown_module_param_cb); > if (IS_ERR(after_dashes)) { > err = PTR_ERR(after_dashes); > - goto bug_cleanup; > + goto coming_cleanup; > } else if (after_dashes) { > pr_warn("%s: parameters '%s' after `--' ignored\n", > mod->name, after_dashes); > > Now for the error conditions. If complete_formation() fails, goto > ddebug_cleanup. If prepare_coming_module() fails (at that point, > module_enable_{ro,nx} and module_bug_finalize() have already finished), goto > bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable, > ftrace_module_enable, and the coming notifiers have finished) goto > coming_cleanup. ftrace_release_mod() gets called in the goto free_module label > so we don't have to call it in coming_module. Sounds good. > Also, one last thing, I noticed that module->state isn't > set to MODULE_STATE_GOING anywhere before the going notifier chain is called in > the bug_cleanup label (I think it is still COMING at that point), so the > klp_module_disable call right afterwards would have bailed out because of that. > To be consistent, shouldn't it be set before the going notifiers are called? Good spotting. You could argue that there's a difference between the notifier argument (what we're doing) and the module state (how far it got), but we do set 'mod->state = MODULE_STATE_GOING;' in the initcall fail case, so this should be the same. Patch welcome :) Thanks, Rusty. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-03-15 9:10 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-11 20:03 [PATCH v2 0/2] Livepatch module notifier cleanup Jessica Yu 2016-03-11 20:03 ` [PATCH v2 1/2] modules: split part of complete_formation() into prepare_coming_module() Jessica Yu 2016-03-14 15:12 ` Petr Mladek 2016-03-11 20:03 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu 2016-03-14 15:06 ` Petr Mladek 2016-03-14 17:50 ` Jessica Yu 2016-03-15 9:10 ` Petr Mladek 2016-03-14 20:01 ` [PATCH v2 2/2] " Josh Poimboeuf -- strict thread matches above, loose matches on Subject: below -- 2016-02-09 4:50 [PATCH v4 0/4] Fix ordering of ftrace/livepatch calls on module load and unload Jessica Yu 2016-02-09 4:50 ` [PATCH v4 4/4] livepatch/module: remove livepatch module notifier Jessica Yu 2016-02-09 23:43 ` Rusty Russell 2016-02-10 10:18 ` Jiri Kosina 2016-02-14 22:59 ` Jiri Kosina 2016-02-15 23:27 ` Josh Poimboeuf 2016-02-15 23:42 ` Jiri Kosina 2016-02-16 0:48 ` Jessica Yu 2016-02-16 8:41 ` Miroslav Benes 2016-02-16 19:51 ` Jessica Yu 2016-02-29 0:30 ` [PATCH v4 4/4] " Rusty Russell 2016-03-01 3:00 ` Jessica Yu 2016-02-02 1:17 [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jessica Yu 2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu 2016-02-04 14:39 ` Petr Mladek 2016-02-05 4:11 ` Jessica Yu 2016-02-05 9:15 ` Miroslav Benes 2016-02-05 10:06 ` Petr Mladek 2016-02-08 0:34 ` Rusty Russell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).