From: Petr Mladek <pmladek@suse.com>
To: Jessica Yu <jeyu@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
Seth Jennings <sjenning@redhat.com>,
Jiri Kosina <jikos@kernel.org>, Vojtech Pavlik <vojtech@suse.com>,
Miroslav Benes <mbenes@suse.cz>,
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 v2 2/2] livepatch/module: remove livepatch module notifier
Date: Thu, 4 Feb 2016 15:39:35 +0100 [thread overview]
Message-ID: <20160204143934.GZ3305@pathway.suse.cz> (raw)
In-Reply-To: <1454375856-27757-3-git-send-email-jeyu@redhat.com>
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
next prev parent reply other threads:[~2016-02-04 14:39 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
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 1/2] ftrace/module: remove ftrace module notifier Jessica Yu
2016-02-04 13:27 ` Petr Mladek
2016-02-04 14:18 ` Steven Rostedt
2016-02-04 15:21 ` Petr Mladek
2016-02-04 15:33 ` Steven Rostedt
2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
2016-02-04 14:39 ` Petr Mladek [this message]
2016-02-04 14:56 ` Steven Rostedt
2016-02-04 16:47 ` Miroslav Benes
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
2016-02-04 17:29 ` [PATCH v2 2/2] " Miroslav Benes
2016-02-04 20:55 ` Josh Poimboeuf
2016-02-05 8:59 ` Miroslav Benes
2016-02-04 10:43 ` [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jiri Kosina
2016-02-04 13:29 ` Steven Rostedt
2016-02-05 1:17 ` Rusty Russell
-- strict thread matches above, loose matches on Subject: below --
2016-03-11 20:03 [PATCH v2 0/2] Livepatch module notifier cleanup Jessica Yu
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
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=20160204143934.GZ3305@pathway.suse.cz \
--to=pmladek@suse.com \
--cc=jeyu@redhat.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=mingo@redhat.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).