From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Jessica Yu <jeyu@redhat.com>
Cc: Seth Jennings <sjenning@redhat.com>,
Jiri Kosina <jikos@kernel.org>, Vojtech Pavlik <vojtech@suse.com>,
Miroslav Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>,
Rusty Russell <rusty@rustcorp.com.au>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier
Date: Tue, 9 Feb 2016 09:45:38 -0600 [thread overview]
Message-ID: <20160209154538.GD10270@treble.redhat.com> (raw)
In-Reply-To: <1454993424-31031-5-git-send-email-jeyu@redhat.com>
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@redhat.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> 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
next prev parent reply other threads:[~2016-02-09 15:45 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
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 1/4] modules: split part of complete_formation() into prepare_coming_module() Jessica Yu
2016-02-09 15:41 ` Josh Poimboeuf
2016-02-10 10:13 ` Miroslav Benes
2016-02-10 12:16 ` Petr Mladek
2016-02-09 4:50 ` [PATCH v4 2/4] modules: set mod->state to MODULE_STATE_GOING before going notifiers are called Jessica Yu
2016-02-09 15:45 ` Josh Poimboeuf
2016-02-10 10:20 ` Miroslav Benes
2016-02-10 12:37 ` Petr Mladek
2016-02-09 4:50 ` [PATCH v4 3/4] ftrace/module: remove ftrace module notifier Jessica Yu
2016-02-09 15:45 ` Josh Poimboeuf
2016-02-10 10:27 ` Miroslav Benes
2016-02-09 4:50 ` [PATCH v4 4/4] livepatch/module: remove livepatch " Jessica Yu
2016-02-09 15:45 ` Josh Poimboeuf [this message]
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-03-29 2:03 ` [PATCH v4 4/4] " Steven Rostedt
2016-02-29 0:30 ` Rusty Russell
2016-03-01 3:00 ` Jessica Yu
2016-02-10 10:34 ` [PATCH v4 4/4] " Miroslav Benes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160209154538.GD10270@treble.redhat.com \
--to=jpoimboe@redhat.com \
--cc=jeyu@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=mingo@redhat.com \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=rusty@rustcorp.com.au \
--cc=sjenning@redhat.com \
--cc=vojtech@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).