* [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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ messages in thread
end of thread, other threads:[~2016-03-15 9:10 UTC | newest] Thread overview: 8+ 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
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).