On Wed, Mar 09, 2016 at 05:13:57PM -0500, 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 <[email protected]> > --- > include/linux/livepatch.h | 13 +++++ > kernel/livepatch/core.c | 145 > ++++++++++++++++++++++------------------------ > kernel/module.c | 10 ++++ > 3 files changed, 92 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 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;
If we're removing patch 2/3, then mod->state isn't necessarily GOING. I think the check can probably just be removed altogether. With that fixed, Acked-by: Josh Poimboeuf <[email protected]> -- Josh
