On Mon, Feb 08, 2016 at 11:50:24PM -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.
>
> 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@xxxxxxxxxx>
> Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
> Acked-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
Acked-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
> 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
>
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
