* [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload
@ 2016-02-02 1:17 Jessica Yu
2016-02-02 1:17 ` [PATCH v2 1/2] ftrace/module: remove ftrace module notifier Jessica Yu
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Jessica Yu @ 2016-02-02 1:17 UTC (permalink / raw)
To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar
Cc: live-patching, linux-kernel, Jessica Yu
As explained here [1], livepatch modules are failing to initialize properly
because the ftrace coming module notifier (which calls
ftrace_module_enable()) runs *after* the livepatch module notifier (which
enables the patch(es)). Thus livepatch attempts to apply patches to
modules before ftrace_module_enable() is even called for the corresponding
module(s). As a result, patch modules break. Ftrace code must run before
livepatch on module load, and the reverse is true on module unload.
For ftrace and livepatch, order of initialization (plus exit/cleanup code)
is important for loading and unloading modules, and using module notifiers
to perform this work is not ideal since it is not always clear what gets
called when. In this patchset, dependence on the module notifier call chain
is removed in favor of hard coding the corresponding function calls in the
module loader. This promotes better code visibility and ensures that ftrace
and livepatch code get called in the correct order on patch module load and
unload.
Tested the changes with a test livepatch module that patches 9p and nilfs2,
and verified that the issue described in [1] is fixed.
Patches are based on linux-next.
v1 can be found here -
http://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com
v2:
- Instead of splitting the ftrace and livepatch notifiers into coming + going
notifiers and adjusting their priorities, remove ftrace and livepatch notifiers
completely and hard-code the necessary function calls in the module loader.
[1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com
Jessica Yu (2):
ftrace/module: remove ftrace module notifier
livepatch/module: remove livepatch module notifier
include/linux/ftrace.h | 6 +-
include/linux/livepatch.h | 9 +++
kernel/livepatch/core.c | 144 ++++++++++++++++++++++------------------------
kernel/module.c | 12 ++++
kernel/trace/ftrace.c | 36 +-----------
5 files changed, 95 insertions(+), 112 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] ftrace/module: remove ftrace module notifier
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 ` Jessica Yu
2016-02-04 13:27 ` Petr Mladek
2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
2016-02-04 10:43 ` [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jiri Kosina
2 siblings, 1 reply; 20+ messages in thread
From: Jessica Yu @ 2016-02-02 1:17 UTC (permalink / raw)
To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar
Cc: live-patching, linux-kernel, Jessica Yu
Remove the ftrace module notifier in favor of directly calling
ftrace_module_enable() and ftrace_release_mod() in the module loader.
Hard-coding the function calls directly in the module loader removes
dependence on the module notifier call chain and provides better
visibility and control over what gets called when, which is important
to kernel utilities such as livepatch.
Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
include/linux/ftrace.h | 6 ++++--
kernel/module.c | 4 ++++
kernel/trace/ftrace.c | 36 +-----------------------------------
3 files changed, 9 insertions(+), 37 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 81de712..c2b340e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -603,6 +603,7 @@ extern int ftrace_arch_read_dyn_info(char *buf, int size);
extern int skip_trace(unsigned long ip);
extern void ftrace_module_init(struct module *mod);
+extern void ftrace_module_enable(struct module *mod);
extern void ftrace_release_mod(struct module *mod);
extern void ftrace_disable_daemon(void);
@@ -612,8 +613,9 @@ static inline int skip_trace(unsigned long ip) { return 0; }
static inline int ftrace_force_update(void) { return 0; }
static inline void ftrace_disable_daemon(void) { }
static inline void ftrace_enable_daemon(void) { }
-static inline void ftrace_release_mod(struct module *mod) {}
-static inline void ftrace_module_init(struct module *mod) {}
+static inline void ftrace_module_init(struct module *mod) { }
+static inline void ftrace_module_enable(struct module *mod) { }
+static inline void ftrace_release_mod(struct module *mod) { }
static inline __init int register_ftrace_command(struct ftrace_func_command *cmd)
{
return -EINVAL;
diff --git a/kernel/module.c b/kernel/module.c
index 8358f46..b05d466 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -981,6 +981,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
mod->exit();
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_GOING, mod);
+ ftrace_release_mod(mod);
+
async_synchronize_full();
/* Store the name of the last unloaded module for diagnostic purposes */
@@ -3295,6 +3297,7 @@ fail:
module_put(mod);
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_GOING, mod);
+ ftrace_release_mod(mod);
free_module(mod);
wake_up_all(&module_wq);
return ret;
@@ -3371,6 +3374,7 @@ 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;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index eca592f..57a6eea 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4961,7 +4961,7 @@ void ftrace_release_mod(struct module *mod)
mutex_unlock(&ftrace_lock);
}
-static void ftrace_module_enable(struct module *mod)
+void ftrace_module_enable(struct module *mod)
{
struct dyn_ftrace *rec;
struct ftrace_page *pg;
@@ -5038,38 +5038,8 @@ void ftrace_module_init(struct module *mod)
ftrace_process_locs(mod, mod->ftrace_callsites,
mod->ftrace_callsites + mod->num_ftrace_callsites);
}
-
-static int ftrace_module_notify(struct notifier_block *self,
- unsigned long val, void *data)
-{
- struct module *mod = data;
-
- switch (val) {
- case MODULE_STATE_COMING:
- ftrace_module_enable(mod);
- break;
- case MODULE_STATE_GOING:
- ftrace_release_mod(mod);
- break;
- default:
- break;
- }
-
- return 0;
-}
-#else
-static int ftrace_module_notify(struct notifier_block *self,
- unsigned long val, void *data)
-{
- return 0;
-}
#endif /* CONFIG_MODULES */
-struct notifier_block ftrace_module_nb = {
- .notifier_call = ftrace_module_notify,
- .priority = INT_MIN, /* Run after anything that can remove kprobes */
-};
-
void __init ftrace_init(void)
{
extern unsigned long __start_mcount_loc[];
@@ -5098,10 +5068,6 @@ void __init ftrace_init(void)
__start_mcount_loc,
__stop_mcount_loc);
- ret = register_module_notifier(&ftrace_module_nb);
- if (ret)
- pr_warning("Failed to register trace ftrace module exit notifier\n");
-
set_ftrace_early_filters();
return;
--
2.4.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
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-02 1:17 ` Jessica Yu
2016-02-04 14:39 ` Petr Mladek
` (2 more replies)
2016-02-04 10:43 ` [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jiri Kosina
2 siblings, 3 replies; 20+ messages in thread
From: Jessica Yu @ 2016-02-02 1:17 UTC (permalink / raw)
To: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar
Cc: 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_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.
* 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;
- 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);
+ 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;
+
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_COMING, mod);
return 0;
@@ -3531,6 +3538,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_GOING, mod);
+ klp_module_disable(mod);
/* we can't deallocate the module until we clear memory protection */
module_disable_ro(mod);
--
2.4.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload
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-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
@ 2016-02-04 10:43 ` Jiri Kosina
2016-02-04 13:29 ` Steven Rostedt
2 siblings, 1 reply; 20+ messages in thread
From: Jiri Kosina @ 2016-02-04 10:43 UTC (permalink / raw)
To: Jessica Yu
Cc: Josh Poimboeuf, Seth Jennings, Vojtech Pavlik, Miroslav Benes,
Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching,
linux-kernel
On Mon, 1 Feb 2016, Jessica Yu wrote:
> As explained here [1], livepatch modules are failing to initialize properly
> because the ftrace coming module notifier (which calls
> ftrace_module_enable()) runs *after* the livepatch module notifier (which
> enables the patch(es)). Thus livepatch attempts to apply patches to
> modules before ftrace_module_enable() is even called for the corresponding
> module(s). As a result, patch modules break. Ftrace code must run before
> livepatch on module load, and the reverse is true on module unload.
>
> For ftrace and livepatch, order of initialization (plus exit/cleanup code)
> is important for loading and unloading modules, and using module notifiers
> to perform this work is not ideal since it is not always clear what gets
> called when. In this patchset, dependence on the module notifier call chain
> is removed in favor of hard coding the corresponding function calls in the
> module loader. This promotes better code visibility and ensures that ftrace
> and livepatch code get called in the correct order on patch module load and
> unload.
>
> Tested the changes with a test livepatch module that patches 9p and nilfs2,
> and verified that the issue described in [1] is fixed.
>
> Patches are based on linux-next.
>
> v1 can be found here -
> http://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com
>
> v2:
> - Instead of splitting the ftrace and livepatch notifiers into coming + going
> notifiers and adjusting their priorities, remove ftrace and livepatch notifiers
> completely and hard-code the necessary function calls in the module loader.
>
> [1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com
>
>
> Jessica Yu (2):
> ftrace/module: remove ftrace module notifier
> livepatch/module: remove livepatch module notifier
>
> include/linux/ftrace.h | 6 +-
> include/linux/livepatch.h | 9 +++
> kernel/livepatch/core.c | 144 ++++++++++++++++++++++------------------------
> kernel/module.c | 12 ++++
> kernel/trace/ftrace.c | 36 +-----------
Steven, Rusty, what is your word on this please?
These two patches should be merged together, and I'd like to have the
module patching issue Jessica discovered fixed for 4.5 still. IOW, if you
Ack the parts relevant to you (ftrace and module), I'd be willing to take
it to Linus through my tree.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] ftrace/module: remove ftrace module notifier
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
0 siblings, 1 reply; 20+ messages in thread
From: Petr Mladek @ 2016-02-04 13:27 UTC (permalink / raw)
To: Jessica Yu
Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar,
live-patching, linux-kernel
On Mon 2016-02-01 20:17:35, Jessica Yu wrote:
> Remove the ftrace module notifier in favor of directly calling
> ftrace_module_enable() and ftrace_release_mod() in the module loader.
> Hard-coding the function calls directly in the module loader removes
> dependence on the module notifier call chain and provides better
> visibility and control over what gets called when, which is important
> to kernel utilities such as livepatch.
>
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
> include/linux/ftrace.h | 6 ++++--
> kernel/module.c | 4 ++++
> kernel/trace/ftrace.c | 36 +-----------------------------------
> 3 files changed, 9 insertions(+), 37 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 81de712..c2b340e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -603,6 +603,7 @@ extern int ftrace_arch_read_dyn_info(char *buf, int size);
>
> extern int skip_trace(unsigned long ip);
> extern void ftrace_module_init(struct module *mod);
> +extern void ftrace_module_enable(struct module *mod);
> extern void ftrace_release_mod(struct module *mod);
>
> extern void ftrace_disable_daemon(void);
> @@ -612,8 +613,9 @@ static inline int skip_trace(unsigned long ip) { return 0; }
> static inline int ftrace_force_update(void) { return 0; }
> static inline void ftrace_disable_daemon(void) { }
> static inline void ftrace_enable_daemon(void) { }
> -static inline void ftrace_release_mod(struct module *mod) {}
> -static inline void ftrace_module_init(struct module *mod) {}
> +static inline void ftrace_module_init(struct module *mod) { }
> +static inline void ftrace_module_enable(struct module *mod) { }
> +static inline void ftrace_release_mod(struct module *mod) { }
> static inline __init int register_ftrace_command(struct ftrace_func_command *cmd)
> {
> return -EINVAL;
> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f46..b05d466 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -981,6 +981,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> mod->exit();
> blocking_notifier_call_chain(&module_notify_list,
> MODULE_STATE_GOING, mod);
> + ftrace_release_mod(mod);
> +
> async_synchronize_full();
>
> /* Store the name of the last unloaded module for diagnostic purposes */
> @@ -3295,6 +3297,7 @@ fail:
> module_put(mod);
> blocking_notifier_call_chain(&module_notify_list,
> MODULE_STATE_GOING, mod);
> + ftrace_release_mod(mod);
> free_module(mod);
> wake_up_all(&module_wq);
> return ret;
> @@ -3371,6 +3374,7 @@ 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;
Also we need to call ftrace_release_mod() in bug_cleanup:
goto target in load_module(). Otherwise, it will stay
enabled when, e.g. parse_args() fails.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload
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
0 siblings, 1 reply; 20+ messages in thread
From: Steven Rostedt @ 2016-02-04 13:29 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Ingo Molnar, live-patching,
linux-kernel
On Thu, 4 Feb 2016 11:43:57 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:
>
> Steven, Rusty, what is your word on this please?
I'm fine with the first patch, but you still need an Ack from Rusty on
both. Unfortunately, Rusty is more part-time on kernel development
these days. Hopefully he's still paying attention.
>
> These two patches should be merged together, and I'd like to have the
> module patching issue Jessica discovered fixed for 4.5 still. IOW, if you
> Ack the parts relevant to you (ftrace and module), I'd be willing to take
> it to Linus through my tree.
If you get Rusty's acks, you can add my:
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
on the first patch and take it through your tree.
-- Steve
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] ftrace/module: remove ftrace module notifier
2016-02-04 13:27 ` Petr Mladek
@ 2016-02-04 14:18 ` Steven Rostedt
2016-02-04 15:21 ` Petr Mladek
0 siblings, 1 reply; 20+ messages in thread
From: Steven Rostedt @ 2016-02-04 14:18 UTC (permalink / raw)
To: Petr Mladek
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Miroslav Benes, Rusty Russell, Ingo Molnar,
live-patching, linux-kernel
On Thu, 4 Feb 2016 14:27:51 +0100
Petr Mladek <pmladek@suse.com> wrote:
> > + ftrace_module_enable(mod);
> > blocking_notifier_call_chain(&module_notify_list,
> > MODULE_STATE_COMING, mod);
> > return 0;
>
> Also we need to call ftrace_release_mod() in bug_cleanup:
> goto target in load_module(). Otherwise, it will stay
> enabled when, e.g. parse_args() fails.
Look farther down (after free_module:), it's already there.
-- Steve
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
@ 2016-02-04 14:39 ` Petr Mladek
2016-02-04 14:56 ` Steven Rostedt
` (2 more replies)
2016-02-04 17:29 ` [PATCH v2 2/2] " Miroslav Benes
2016-02-04 20:55 ` Josh Poimboeuf
2 siblings, 3 replies; 20+ messages in thread
From: Petr Mladek @ 2016-02-04 14:39 UTC (permalink / raw)
To: Jessica Yu
Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar,
live-patching, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
2016-02-04 14:39 ` Petr Mladek
@ 2016-02-04 14:56 ` Steven Rostedt
2016-02-04 16:47 ` Miroslav Benes
2016-02-05 4:11 ` Jessica Yu
2 siblings, 0 replies; 20+ messages in thread
From: Steven Rostedt @ 2016-02-04 14:56 UTC (permalink / raw)
To: Petr Mladek
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Miroslav Benes, Rusty Russell, Ingo Molnar,
live-patching, linux-kernel
On Thu, 4 Feb 2016 15:39:35 +0100
Petr Mladek <pmladek@suse.com> wrote:
> > @@ -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.
If complete_formation() fails, load_module will do a goto
ddebug_cleanup, which will eventually call ftrace_release_mod(). No
need to do it here.
-- Steve
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] ftrace/module: remove ftrace module notifier
2016-02-04 14:18 ` Steven Rostedt
@ 2016-02-04 15:21 ` Petr Mladek
2016-02-04 15:33 ` Steven Rostedt
0 siblings, 1 reply; 20+ messages in thread
From: Petr Mladek @ 2016-02-04 15:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Miroslav Benes, Rusty Russell, Ingo Molnar,
live-patching, linux-kernel
On Thu 2016-02-04 09:18:01, Steven Rostedt wrote:
> On Thu, 4 Feb 2016 14:27:51 +0100
> Petr Mladek <pmladek@suse.com> wrote:
>
>
> > > + ftrace_module_enable(mod);
> > > blocking_notifier_call_chain(&module_notify_list,
> > > MODULE_STATE_COMING, mod);
> > > return 0;
> >
> > Also we need to call ftrace_release_mod() in bug_cleanup:
> > goto target in load_module(). Otherwise, it will stay
> > enabled when, e.g. parse_args() fails.
>
> Look farther down (after free_module:), it's already there.
Ah, I see. ftrace_release_mod() is called there on an unexpected
location. Error paths typically do actions in the reverse order
in compare with the normal paths :-)
Thanks for the pointer,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] ftrace/module: remove ftrace module notifier
2016-02-04 15:21 ` Petr Mladek
@ 2016-02-04 15:33 ` Steven Rostedt
0 siblings, 0 replies; 20+ messages in thread
From: Steven Rostedt @ 2016-02-04 15:33 UTC (permalink / raw)
To: Petr Mladek
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Miroslav Benes, Rusty Russell, Ingo Molnar,
live-patching, linux-kernel
On Thu, 4 Feb 2016 16:21:20 +0100
Petr Mladek <pmladek@suse.com> wrote:
> Ah, I see. ftrace_release_mod() is called there on an unexpected
> location. Error paths typically do actions in the reverse order
> in compare with the normal paths :-)
And that's why there's a comment above it describing why it's not in
the normal "reverse" location.
-- Steve
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
2016-02-04 14:39 ` Petr Mladek
2016-02-04 14:56 ` Steven Rostedt
@ 2016-02-04 16:47 ` Miroslav Benes
2016-02-05 4:11 ` Jessica Yu
2 siblings, 0 replies; 20+ messages in thread
From: Miroslav Benes @ 2016-02-04 16:47 UTC (permalink / raw)
To: Petr Mladek
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Rusty Russell, Steven Rostedt, Ingo Molnar,
live-patching, linux-kernel
On Thu, 4 Feb 2016, Petr Mladek wrote:
> On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> >
> >
> > - 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.
Yes, because we are in a better situation with this patch. We actually
return an error and refuse to load the module. Message should take that
into account.
> 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!
Actually, I think the code is correct. If klp_init_object_loaded() there
is no problem because we only write relocations there (which are written
to the module being loaded) and resolve symbols via kallsyms. Nothing to
revert there and it could be done again.
If klp_enable_object() fails, all the relevant error handling was already
done there. See the call to klp_disable_object() if klp_enable_function()
fails there.
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
2016-02-04 14:39 ` Petr Mladek
@ 2016-02-04 17:29 ` Miroslav Benes
2016-02-04 20:55 ` Josh Poimboeuf
2 siblings, 0 replies; 20+ messages in thread
From: Miroslav Benes @ 2016-02-04 17:29 UTC (permalink / raw)
To: Jessica Yu
Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching,
linux-kernel
On Mon, 1 Feb 2016, Jessica Yu wrote:
> +/* 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);
We do not use 'extern' keyword in header files. It is redundant.
Unfortunately, the situation differs among header files and it is hard to
be consistent.
> + /*
> + * 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;
This comment should fixed too.
Note: we still need klp_alive, because the race is still there even
without notifiers.
> +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;
This is similar to what Petr proposed. We must be in MODULE_STATE_GOING
here. We could WARN here and return.
> diff --git a/kernel/module.c b/kernel/module.c
> index b05d466..71c77ed 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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 (err)
return err;
module_mutex is already unlocked so no need to jump to out.
Apart from these minor things and Petr's remarks it looks ok.
Thanks for fixing this.
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
2016-02-04 14:39 ` Petr Mladek
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
2 siblings, 1 reply; 20+ messages in thread
From: Josh Poimboeuf @ 2016-02-04 20:55 UTC (permalink / raw)
To: Jessica Yu
Cc: Seth Jennings, Jiri Kosina, Vojtech Pavlik, Miroslav Benes,
Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching,
linux-kernel
On Mon, Feb 01, 2016 at 08:17:36PM -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_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.
> * 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)
I think this function name was originally my idea. But now I'm thinking
it could cause some confusion with the similarly named
klp_enable_object().
How about naming it klp_module_coming()? That more accurately describes
its purpose IMO and it would also make the comment above it no longer
necessary.
And similarly we could rename klp_module_disable() ->
klp_module_going().
--
Josh
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload
2016-02-04 13:29 ` Steven Rostedt
@ 2016-02-05 1:17 ` Rusty Russell
0 siblings, 0 replies; 20+ messages in thread
From: Rusty Russell @ 2016-02-05 1:17 UTC (permalink / raw)
To: Steven Rostedt, Jiri Kosina
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Vojtech Pavlik,
Miroslav Benes, Ingo Molnar, live-patching, linux-kernel
Steven Rostedt <rostedt@goodmis.org> writes:
>> These two patches should be merged together, and I'd like to have the
>> module patching issue Jessica discovered fixed for 4.5 still. IOW, if you
>> Ack the parts relevant to you (ftrace and module), I'd be willing to take
>> it to Linus through my tree.
>
> If you get Rusty's acks, you can add my:
>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
>
> on the first patch and take it through your tree.
Agreed.
Acked-by: Rusty Russell <rusty@rustcorp.com.au> (module.c parts)
Cheers,
Rusty.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: livepatch/module: remove livepatch module notifier
2016-02-04 14:39 ` Petr Mladek
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-08 0:34 ` Rusty Russell
2 siblings, 2 replies; 20+ messages in thread
From: Jessica Yu @ 2016-02-05 4:11 UTC (permalink / raw)
To: Petr Mladek
Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Rusty Russell, Steven Rostedt, Ingo Molnar,
live-patching, linux-kernel
+++ Petr Mladek [04/02/16 15:39 +0100]:
>On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
[ snipped since email is getting long ]
>> 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.
Argh, thank you for catching that. I think we could split up complete_formation()
into two functions in order to make the error handling work.
We could probably take out the coming notifier calls, ftrace_module_enable(),
and klp_module_enable() out of complete_formation(), and put them in another
function, maybe called prepare_coming_module(), that would be called right
after complete_formation(). It might look something like this:
@@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
err = complete_formation(mod, info);
if (err)
goto ddebug_cleanup;
+ err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers
+ if (err) // means that klp_module_enable failed
+ 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,
@@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
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);
Now for the error conditions. If complete_formation() fails, goto
ddebug_cleanup. If prepare_coming_module() fails (at that point,
module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
ftrace_module_enable, and the coming notifiers have finished) goto
coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
so we don't have to call it in coming_module.
@@ -3649,16 +3659,16 @@ 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_disable(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);
- klp_module_disable(mod);
-
/* we can't deallocate the module until we clear memory protection */
module_disable_ro(mod);
module_disable_nx(mod);
Does all this look ok? Also, one last thing, I noticed that module->state isn't
set to MODULE_STATE_GOING anywhere before the going notifier chain is called in
the bug_cleanup label (I think it is still COMING at that point), so the
klp_module_disable call right afterwards would have bailed out because of that.
To be consistent, shouldn't it be set before the going notifiers are called?
Thanks,
Jessica
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
2016-02-04 20:55 ` Josh Poimboeuf
@ 2016-02-05 8:59 ` Miroslav Benes
0 siblings, 0 replies; 20+ messages in thread
From: Miroslav Benes @ 2016-02-05 8:59 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Jessica Yu, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Rusty Russell, Steven Rostedt, Ingo Molnar, live-patching,
linux-kernel
On Thu, 4 Feb 2016, Josh Poimboeuf wrote:
> On Mon, Feb 01, 2016 at 08:17:36PM -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_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.
> > * 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)
>
> I think this function name was originally my idea. But now I'm thinking
> it could cause some confusion with the similarly named
> klp_enable_object().
>
> How about naming it klp_module_coming()? That more accurately describes
> its purpose IMO and it would also make the comment above it no longer
> necessary.
>
> And similarly we could rename klp_module_disable() ->
> klp_module_going().
I agree. klp_module_{coming,going} is better.
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: livepatch/module: remove livepatch module notifier
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
1 sibling, 1 reply; 20+ messages in thread
From: Miroslav Benes @ 2016-02-05 9:15 UTC (permalink / raw)
To: Jessica Yu
Cc: Petr Mladek, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Rusty Russell, Steven Rostedt, Ingo Molnar,
live-patching, linux-kernel
On Thu, 4 Feb 2016, Jessica Yu wrote:
> +++ Petr Mladek [04/02/16 15:39 +0100]:
> > On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> [ snipped since email is getting long ]
> > > 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.
>
> Argh, thank you for catching that. I think we could split up
> complete_formation()
> into two functions in order to make the error handling work.
> We could probably take out the coming notifier calls, ftrace_module_enable(),
> and klp_module_enable() out of complete_formation(), and put them in another
> function, maybe called prepare_coming_module(), that would be called right
> after complete_formation(). It might look something like this:
>
> @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const
> char __user *uargs,
> err = complete_formation(mod, info); if (err)
> goto ddebug_cleanup;
> + err = prepare_coming_module(mod); // calls ftrace_module_enable(),
> klp_module_enable(), then coming notifiers
> + if (err) // means that klp_module_enable failed
> + 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,
> @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const
> char __user *uargs,
> 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);
>
> Now for the error conditions. If complete_formation() fails, goto
> ddebug_cleanup. If prepare_coming_module() fails (at that point,
> module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
> bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
> ftrace_module_enable, and the coming notifiers have finished) goto
> coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
> so we don't have to call it in coming_module.
>
> @@ -3649,16 +3659,16 @@ 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_disable(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);
> - klp_module_disable(mod);
> -
> /* we can't deallocate the module until we clear memory protection */
> module_disable_ro(mod);
> module_disable_nx(mod);
>
> Does all this look ok?
Hm, there is an another option. We can cover the needed error handling in
complete_formation(). So for the first error there
(verify_export_symbols() fails) we need to only release module_mutex. For
our second error we need more. We would call module_bug_cleanup() under
module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct?
It would be hidden in complete_formation() this way and cleaner in my
opinion. There is some code duplication though.
> Also, one last thing, I noticed that module->state
> isn't
> set to MODULE_STATE_GOING anywhere before the going notifier chain is called
> in
> the bug_cleanup label (I think it is still COMING at that point), so the
> klp_module_disable call right afterwards would have bailed out because of
> that.
> To be consistent, shouldn't it be set before the going notifiers are called?
This could break something or introduce a race somewhere. Git grep says
there are several checks for MODULE_STATE_GOING through out the kernel
which need to be checked.
Regards,
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: livepatch/module: remove livepatch module notifier
2016-02-05 9:15 ` Miroslav Benes
@ 2016-02-05 10:06 ` Petr Mladek
0 siblings, 0 replies; 20+ messages in thread
From: Petr Mladek @ 2016-02-05 10:06 UTC (permalink / raw)
To: Miroslav Benes
Cc: Jessica Yu, Josh Poimboeuf, Seth Jennings, Jiri Kosina,
Vojtech Pavlik, Rusty Russell, Steven Rostedt, Ingo Molnar,
live-patching, linux-kernel
On Fri 2016-02-05 10:15:56, Miroslav Benes wrote:
> On Thu, 4 Feb 2016, Jessica Yu wrote:
> > Argh, thank you for catching that. I think we could split up
> > complete_formation()
> > into two functions in order to make the error handling work.
> >
> > Does all this look ok?
>
> Hm, there is an another option. We can cover the needed error handling in
> complete_formation(). So for the first error there
> (verify_export_symbols() fails) we need to only release module_mutex. For
> our second error we need more. We would call module_bug_cleanup() under
> module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct?
> It would be hidden in complete_formation() this way and cleaner in my
> opinion. There is some code duplication though.
This sounds better to me.
> > Also, one last thing, I noticed that module->state
> > isn't
> > set to MODULE_STATE_GOING anywhere before the going notifier chain is called
> > in
> > the bug_cleanup label (I think it is still COMING at that point), so the
> > klp_module_disable call right afterwards would have bailed out because of
> > that.
> > To be consistent, shouldn't it be set before the going notifiers are called?
>
> This could break something or introduce a race somewhere. Git grep says
> there are several checks for MODULE_STATE_GOING through out the kernel
> which need to be checked.
I think that we could relax the condition in the
klp_module_going/disable() function and allow to call it
also in MODULE_STATE_COMMING_STATE. It would deserve
a comment.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: livepatch/module: remove livepatch module notifier
2016-02-05 4:11 ` Jessica Yu
2016-02-05 9:15 ` Miroslav Benes
@ 2016-02-08 0:34 ` Rusty Russell
1 sibling, 0 replies; 20+ messages in thread
From: Rusty Russell @ 2016-02-08 0:34 UTC (permalink / raw)
To: Jessica Yu, Petr Mladek
Cc: Josh Poimboeuf, Seth Jennings, Jiri Kosina, Vojtech Pavlik,
Miroslav Benes, Steven Rostedt, Ingo Molnar, live-patching,
linux-kernel
Jessica Yu <jeyu@redhat.com> writes:
> +++ Petr Mladek [04/02/16 15:39 +0100]:
>>On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> [ snipped since email is getting long ]
>>> 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.
>
> Argh, thank you for catching that. I think we could split up complete_formation()
> into two functions in order to make the error handling work.
>
> We could probably take out the coming notifier calls, ftrace_module_enable(),
> and klp_module_enable() out of complete_formation(), and put them in another
> function, maybe called prepare_coming_module(), that would be called right
> after complete_formation(). It might look something like this:
>
> @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
> err = complete_formation(mod, info);
> if (err)
> goto ddebug_cleanup;
> + err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers
> + if (err) // means that klp_module_enable failed
> + 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,
> @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> 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);
>
> Now for the error conditions. If complete_formation() fails, goto
> ddebug_cleanup. If prepare_coming_module() fails (at that point,
> module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
> bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
> ftrace_module_enable, and the coming notifiers have finished) goto
> coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
> so we don't have to call it in coming_module.
Sounds good.
> Also, one last thing, I noticed that module->state isn't
> set to MODULE_STATE_GOING anywhere before the going notifier chain is called in
> the bug_cleanup label (I think it is still COMING at that point), so the
> klp_module_disable call right afterwards would have bailed out because of that.
> To be consistent, shouldn't it be set before the going notifiers are called?
Good spotting. You could argue that there's a difference between the
notifier argument (what we're doing) and the module state (how far it
got), but we do set 'mod->state = MODULE_STATE_GOING;' in the initcall
fail case, so this should be the same.
Patch welcome :)
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-02-08 1:52 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).