[PATCH v4 0/4] Fix ordering of ftrace/livepatch calls on module load and unload

[PATCH v4 0/4] Fix ordering of ftrace/livepatch calls on module load and unload

[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:
https://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com
v2:
https://lkml.kernel.org/g/1454375856-27757-1-git-send-email-jeyu@redhat.com
v3:
https://lkml.kernel.org/g/1454728097-7106-1-git-send-email-jeyu@redhat.com

v4:
- Split part of complete_formation() into prepare_coming_module() to make
  error handling a bit easier
- Minor tweak: change mod->state to going before calling going notifiers in
  the load_module error path
- Swapped out obj->mod = NULL assignment in klp_module_coming() for a
  call to klp_free_object_loaded()

v3:
- Fix incorrect comments
- Rename klp_module_{enable,disable} to klp_module_{coming,going}
- Remove externs from livepatch.h
- Fix error handling in kernel/module.c

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 (4):
  modules: split part of complete_formation() into
    prepare_coming_module()
  modules: set mod->state to MODULE_STATE_GOING before going notifiers
    are called
  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   | 145 ++++++++++++++++++++++------------------------
 kernel/module.c           |  40 ++++++++++---
 kernel/trace/ftrace.c     |  36 +-----------
 5 files changed, 116 insertions(+), 120 deletions(-)

-- 
2.4.3

[PATCH v4 1/4] modules: split part of complete_formation() into prepare_coming_module()

[Jessica Yu]

Put all actions that are performed after module->state is set to
MODULE_STATE_COMING in complete_formation() into a separate function
prepare_coming_module(). This prepares for the removal of ftrace and
livepatch module coming notifiers and instead the corresponding work will
be done in prepare_coming_module(). This split will also allow for
appropriate error handling.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 kernel/module.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 9537da3..a174335 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3366,6 +3366,14 @@ out_unlocked:
 	return err;
 }
 
+static int prepare_coming_module(struct module *mod)
+{
+
+	blocking_notifier_call_chain(&module_notify_list,
+				     MODULE_STATE_COMING, mod);
+	return 0;
+}
+
 static int complete_formation(struct module *mod, struct load_info *info)
 {
 	int err;
@@ -3389,8 +3397,6 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mod->state = MODULE_STATE_COMING;
 	mutex_unlock(&module_mutex);
 
-	blocking_notifier_call_chain(&module_notify_list,
-				     MODULE_STATE_COMING, mod);
 	return 0;
 
 out:
@@ -3512,13 +3518,17 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto ddebug_cleanup;
 
+	err = prepare_coming_module(mod);
+	if (err)
+		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,
 				  -32768, 32767, mod,
 				  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);
@@ -3527,7 +3537,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Link in to syfs. */
 	err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
 	if (err < 0)
-		goto bug_cleanup;
+		goto coming_cleanup;
 
 	/* Get rid of temporary copy. */
 	free_copy(info);
@@ -3537,15 +3547,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
+ coming_cleanup:
+	blocking_notifier_call_chain(&module_notify_list,
+				     MODULE_STATE_GOING, 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);
-
 	/* we can't deallocate the module until we clear memory protection */
 	module_disable_ro(mod);
 	module_disable_nx(mod);
-- 
2.4.3

[PATCH v4 2/4] modules: set mod->state to MODULE_STATE_GOING before going notifiers are called

[Jessica Yu]

In load_module(), the going notifiers are called during error handling when
an error occurs after the coming notifiers have already been called.
However, a module's state is still MODULE_STATE_COMING when the going
notifiers are called in the error path. To be consistent, also set
mod->state to MODULE_STATE_GOING before calling the going notifiers.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/module.c b/kernel/module.c
index a174335..77f6791 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3548,6 +3548,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	return do_init_module(mod);
 
  coming_cleanup:
+	mod->state = MODULE_STATE_GOING;
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 
-- 
2.4.3

[PATCH v4 3/4] ftrace/module: remove ftrace module notifier

[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.

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. This patch removes dependence on the module notifier call
chain in favor of hard coding the corresponding function calls in the
module loader. This ensures that ftrace and livepatch code get called in
the correct order on patch module load and unload.

Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
Signed-off-by: Jessica Yu <jeyu@redhat.com>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Petr Mladek <pmladek@suse.cz>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/ftrace.h |  6 ++++--
 kernel/module.c        |  5 +++++
 kernel/trace/ftrace.c  | 36 +-----------------------------------
 3 files changed, 10 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 77f6791..0bd0077 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -984,6 +984,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 */
@@ -3313,6 +3315,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;
@@ -3369,6 +3372,8 @@ out_unlocked:
 static int prepare_coming_module(struct module *mod)
 {
 
+	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

[PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[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_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>
---
 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

Re: [PATCH v4 1/4] modules: split part of complete_formation() into prepare_coming_module()

[Josh Poimboeuf]

On Mon, Feb 08, 2016 at 11:50:21PM -0500, Jessica Yu wrote:
> Put all actions that are performed after module->state is set to
> MODULE_STATE_COMING in complete_formation() into a separate function
> prepare_coming_module(). This prepares for the removal of ftrace and
> livepatch module coming notifiers and instead the corresponding work will
> be done in prepare_coming_module(). This split will also allow for
> appropriate error handling.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

> ---
>  kernel/module.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 9537da3..a174335 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3366,6 +3366,14 @@ out_unlocked:
>  	return err;
>  }
>  
> +static int prepare_coming_module(struct module *mod)
> +{
> +
> +	blocking_notifier_call_chain(&module_notify_list,
> +				     MODULE_STATE_COMING, mod);
> +	return 0;
> +}
> +
>  static int complete_formation(struct module *mod, struct load_info *info)
>  {
>  	int err;
> @@ -3389,8 +3397,6 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mod->state = MODULE_STATE_COMING;
>  	mutex_unlock(&module_mutex);
>  
> -	blocking_notifier_call_chain(&module_notify_list,
> -				     MODULE_STATE_COMING, mod);
>  	return 0;
>  
>  out:
> @@ -3512,13 +3518,17 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err)
>  		goto ddebug_cleanup;
>  
> +	err = prepare_coming_module(mod);
> +	if (err)
> +		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,
>  				  -32768, 32767, mod,
>  				  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);
> @@ -3527,7 +3537,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	/* Link in to syfs. */
>  	err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
>  	if (err < 0)
> -		goto bug_cleanup;
> +		goto coming_cleanup;
>  
>  	/* Get rid of temporary copy. */
>  	free_copy(info);
> @@ -3537,15 +3547,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  
>  	return do_init_module(mod);
>  
> + coming_cleanup:
> +	blocking_notifier_call_chain(&module_notify_list,
> +				     MODULE_STATE_GOING, 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);
> -
>  	/* we can't deallocate the module until we clear memory protection */
>  	module_disable_ro(mod);
>  	module_disable_nx(mod);
> -- 
> 2.4.3
> 

-- 
Josh

Re: [PATCH v4 2/4] modules: set mod->state to MODULE_STATE_GOING before going notifiers are called

[Josh Poimboeuf]

On Mon, Feb 08, 2016 at 11:50:22PM -0500, Jessica Yu wrote:
> In load_module(), the going notifiers are called during error handling when
> an error occurs after the coming notifiers have already been called.
> However, a module's state is still MODULE_STATE_COMING when the going
> notifiers are called in the error path. To be consistent, also set
> mod->state to MODULE_STATE_GOING before calling the going notifiers.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index a174335..77f6791 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3548,6 +3548,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	return do_init_module(mod);
>  
>   coming_cleanup:
> +	mod->state = MODULE_STATE_GOING;
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  
> -- 
> 2.4.3
> 

-- 
Josh

Re: [PATCH v4 3/4] ftrace/module: remove ftrace module notifier

[Josh Poimboeuf]

On Mon, Feb 08, 2016 at 11:50:23PM -0500, 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.
> 
> 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. This patch removes dependence on the module notifier call
> chain in favor of hard coding the corresponding function calls in the
> module loader. This ensures that ftrace and livepatch code get called in
> the correct order on patch module load and unload.
> 
> Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> Reviewed-by: Petr Mladek <pmladek@suse.cz>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

> ---
>  include/linux/ftrace.h |  6 ++++--
>  kernel/module.c        |  5 +++++
>  kernel/trace/ftrace.c  | 36 +-----------------------------------
>  3 files changed, 10 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 77f6791..0bd0077 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -984,6 +984,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 */
> @@ -3313,6 +3315,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;
> @@ -3369,6 +3372,8 @@ out_unlocked:
>  static int prepare_coming_module(struct module *mod)
>  {
>  
> +	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
> 

-- 
Josh

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Josh Poimboeuf]

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

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Rusty Russell]

Jessica Yu <jeyu@redhat.com> writes:
> 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.

Without a Fixes: line, it's not absolutely clear whether this needs
CC:stable, needs to go to Linus now, or can wait for the next merge
window.

I *think* you want all four merged this merge window, and 3 and 4 are
required to fix a regression introduced since 4.4...

Sorry for being obtuse!
Rusty.

Re: [PATCH v4 1/4] modules: split part of complete_formation() into prepare_coming_module()

[Miroslav Benes]

On Mon, 8 Feb 2016, Jessica Yu wrote:

> Put all actions that are performed after module->state is set to
> MODULE_STATE_COMING in complete_formation() into a separate function
> prepare_coming_module(). This prepares for the removal of ftrace and
> livepatch module coming notifiers and instead the corresponding work will
> be done in prepare_coming_module(). This split will also allow for
> appropriate error handling.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Jiri Kosina]

On Wed, 10 Feb 2016, Rusty Russell 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.
> 
> Without a Fixes: line, it's not absolutely clear whether this needs
> CC:stable, needs to go to Linus now, or can wait for the next merge
> window.
> 
> I *think* you want all four merged this merge window, and 3 and 4 are
> required to fix a regression introduced since 4.4...

Your understanding is correct; #3 and #4 are needed to fix a 4.4 
regression. It makes sense for the whole lot go to together, but for #1 
and #2 I absolutely need your Ack before I take it to my tree, as I don't 
want to be merging this behind your back.

Once you Ack #1 and #2, I plan to take this to Linus immediately so that 
we avoid doing these changes as very last minute.

Thanks!

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 2/4] modules: set mod->state to MODULE_STATE_GOING before going notifiers are called

[Miroslav Benes]

On Mon, 8 Feb 2016, Jessica Yu wrote:

> In load_module(), the going notifiers are called during error handling when
> an error occurs after the coming notifiers have already been called.
> However, a module's state is still MODULE_STATE_COMING when the going
> notifiers are called in the error path. To be consistent, also set
> mod->state to MODULE_STATE_GOING before calling the going notifiers.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH v4 3/4] ftrace/module: remove ftrace module notifier

[Miroslav Benes]

On Mon, 8 Feb 2016, 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.
> 
> 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. This patch removes dependence on the module notifier call
> chain in favor of hard coding the corresponding function calls in the
> module loader. This ensures that ftrace and livepatch code get called in
> the correct order on patch module load and unload.
> 
> Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> Reviewed-by: Petr Mladek <pmladek@suse.cz>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Miroslav Benes]

On Mon, 8 Feb 2016, 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>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH v4 1/4] modules: split part of complete_formation() into prepare_coming_module()

[Petr Mladek]

On Mon 2016-02-08 23:50:21, Jessica Yu wrote:
> Put all actions that are performed after module->state is set to
> MODULE_STATE_COMING in complete_formation() into a separate function
> prepare_coming_module(). This prepares for the removal of ftrace and
> livepatch module coming notifiers and instead the corresponding work will
> be done in prepare_coming_module(). This split will also allow for
> appropriate error handling.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v4 2/4] modules: set mod->state to MODULE_STATE_GOING before going notifiers are called

[Petr Mladek]

On Mon 2016-02-08 23:50:22, Jessica Yu wrote:
> In load_module(), the going notifiers are called during error handling when
> an error occurs after the coming notifiers have already been called.
> However, a module's state is still MODULE_STATE_COMING when the going
> notifiers are called in the error path. To be consistent, also set
> mod->state to MODULE_STATE_GOING before calling the going notifiers.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

It makes perfect sense to set the state this way. But note that it is
only partial win. We still stay in the COMING state when using the
other goto targets for the error handling, e.g. bug_cleanup,
ddebug_cleanup.

I was a bit nervous by such a change at this stage of 4.5 release.
I spent quite some time on checking various scenarios and I did
not find any problem with it.

Best Regards,
Petr

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Jiri Kosina]

On Wed, 10 Feb 2016, Jiri Kosina 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.
> > 
> > Without a Fixes: line, it's not absolutely clear whether this needs
> > CC:stable, needs to go to Linus now, or can wait for the next merge
> > window.
> > 
> > I *think* you want all four merged this merge window, and 3 and 4 are
> > required to fix a regression introduced since 4.4...
> 
> Your understanding is correct; #3 and #4 are needed to fix a 4.4 
> regression. It makes sense for the whole lot go to together, but for #1 
> and #2 I absolutely need your Ack before I take it to my tree, as I don't 
> want to be merging this behind your back.
> 
> Once you Ack #1 and #2, I plan to take this to Linus immediately so that 
> we avoid doing these changes as very last minute.

Rusty, friendly ping? :)

I know that this is quite tight timing, but I'd like to have the 4.4 
regression fixed, and we are quite late in the -rc phase already.

Thanks in advance,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Josh Poimboeuf]

On Sun, Feb 14, 2016 at 11:59:00PM +0100, Jiri Kosina wrote:
> On Wed, 10 Feb 2016, Jiri Kosina 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.
> > > 
> > > Without a Fixes: line, it's not absolutely clear whether this needs
> > > CC:stable, needs to go to Linus now, or can wait for the next merge
> > > window.
> > > 
> > > I *think* you want all four merged this merge window, and 3 and 4 are
> > > required to fix a regression introduced since 4.4...
> > 
> > Your understanding is correct; #3 and #4 are needed to fix a 4.4 
> > regression. It makes sense for the whole lot go to together, but for #1 
> > and #2 I absolutely need your Ack before I take it to my tree, as I don't 
> > want to be merging this behind your back.
> > 
> > Once you Ack #1 and #2, I plan to take this to Linus immediately so that 
> > we avoid doing these changes as very last minute.
> 
> Rusty, friendly ping? :)
> 
> I know that this is quite tight timing, but I'd like to have the 4.4 
> regression fixed, and we are quite late in the -rc phase already.

So I think the commit causing the regression is 5156dca34a3e, which
occurred in the 4.5 cycle, *not* in 4.4.

Also it's my understanding that only the third patch ("remove ftrace
module notifier") is needed to fix the regression, and the other patches
are just general improvements.  So if needed I think we can just rebase
that patch (which already has Rusty's ack I believe) and send it to
Linus now.

-- 
Josh

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Jiri Kosina]

On Mon, 15 Feb 2016, Josh Poimboeuf wrote:

> So I think the commit causing the regression is 5156dca34a3e, which
> occurred in the 4.5 cycle, *not* in 4.4.

Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e. 
regression that will become real issue once 4.5 is released.

> Also it's my understanding that only the third patch ("remove ftrace 
> module notifier") is needed to fix the regression, and the other patches 
> are just general improvements.  So if needed I think we can just rebase 
> that patch (which already has Rusty's ack I believe) and send it to 
> Linus now.

3/4 and 4/4 are be sufficient, yes (although I'd like to have this 
confimed by Jessica, as she apparently already has a reliable testcase).

To be honest: I was skiing (and being offline) thursday - sunday :), so my 
original plan was to get Ack from Rusty in the meantime, and then send 
pull request to Linus once I am back on sunday evening.

This is not going to happen, so we have to start with plan B (which is 
pushing just 3/4 and 4/4). Jessica, could you please send me updated (and 
tested on your side) patchset with module.c cleanups omitted?

Steven, I'd appreciate if you could tell me whether your Ack to 
"ftrace/module: remove ftrace module notifier" still holds even if 
module.c changes are not happening.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: livepatch/module: remove livepatch module notifier

[Jessica Yu]

+++ Jiri Kosina [16/02/16 00:42 +0100]:
>On Mon, 15 Feb 2016, Josh Poimboeuf wrote:
>
>> So I think the commit causing the regression is 5156dca34a3e, which
>> occurred in the 4.5 cycle, *not* in 4.4.
>
>Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e.
>regression that will become real issue once 4.5 is released.
>
>> Also it's my understanding that only the third patch ("remove ftrace
>> module notifier") is needed to fix the regression, and the other patches
>> are just general improvements.  So if needed I think we can just rebase
>> that patch (which already has Rusty's ack I believe) and send it to
>> Linus now.
>
>3/4 and 4/4 are be sufficient, yes (although I'd like to have this
>confimed by Jessica, as she apparently already has a reliable testcase).

Yes, so Josh is right; technically only patch 3/4 "ftrace/module:
remove ftrace module notifier" is sufficient enough to fix the bug,
and patch 4/4 is just a natural extension of that change. Since I'm
going to be sending out another patchset anyway without the module.c
cleanups, I'll just keep them together.

Jessica

>To be honest: I was skiing (and being offline) thursday - sunday :), so my
>original plan was to get Ack from Rusty in the meantime, and then send
>pull request to Linus once I am back on sunday evening.
>
>This is not going to happen, so we have to start with plan B (which is
>pushing just 3/4 and 4/4). Jessica, could you please send me updated (and
>tested on your side) patchset with module.c cleanups omitted?
>
>Steven, I'd appreciate if you could tell me whether your Ack to
>"ftrace/module: remove ftrace module notifier" still holds even if
>module.c changes are not happening.
>
>Thanks,
>
>-- 
>Jiri Kosina
>SUSE Labs
>

Re: livepatch/module: remove livepatch module notifier

[Miroslav Benes]

On Mon, 15 Feb 2016, Jessica Yu wrote:

> +++ Jiri Kosina [16/02/16 00:42 +0100]:
> > On Mon, 15 Feb 2016, Josh Poimboeuf wrote:
> > 
> > > So I think the commit causing the regression is 5156dca34a3e, which
> > > occurred in the 4.5 cycle, *not* in 4.4.
> > 
> > Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e.
> > regression that will become real issue once 4.5 is released.
> > 
> > > Also it's my understanding that only the third patch ("remove ftrace
> > > module notifier") is needed to fix the regression, and the other patches
> > > are just general improvements.  So if needed I think we can just rebase
> > > that patch (which already has Rusty's ack I believe) and send it to
> > > Linus now.
> > 
> > 3/4 and 4/4 are be sufficient, yes (although I'd like to have this
> > confimed by Jessica, as she apparently already has a reliable testcase).
> 
> Yes, so Josh is right; technically only patch 3/4 "ftrace/module:
> remove ftrace module notifier" is sufficient enough to fix the bug,
> and patch 4/4 is just a natural extension of that change. Since I'm
> going to be sending out another patchset anyway without the module.c
> cleanups, I'll just keep them together.

Yes, 3/4 should be sufficient to fix the bug. However if you take 4/4 too, 
you need 1/4 as well. Otherwise we would introduce a bug in error handling 
as Petr pointed out.

Cheers,
Miroslav

Re: livepatch/module: remove livepatch module notifier

[Jessica Yu]

+++ Miroslav Benes [16/02/16 09:41 +0100]:
>On Mon, 15 Feb 2016, Jessica Yu wrote:
>
>> +++ Jiri Kosina [16/02/16 00:42 +0100]:
>> > On Mon, 15 Feb 2016, Josh Poimboeuf wrote:
>> >
>> > > So I think the commit causing the regression is 5156dca34a3e, which
>> > > occurred in the 4.5 cycle, *not* in 4.4.
>> >
>> > Agreed, by "4.4 regresion" I mean "regression compared to 4.4"; i.e.
>> > regression that will become real issue once 4.5 is released.
>> >
>> > > Also it's my understanding that only the third patch ("remove ftrace
>> > > module notifier") is needed to fix the regression, and the other patches
>> > > are just general improvements.  So if needed I think we can just rebase
>> > > that patch (which already has Rusty's ack I believe) and send it to
>> > > Linus now.
>> >
>> > 3/4 and 4/4 are be sufficient, yes (although I'd like to have this
>> > confimed by Jessica, as she apparently already has a reliable testcase).
>>
>> Yes, so Josh is right; technically only patch 3/4 "ftrace/module:
>> remove ftrace module notifier" is sufficient enough to fix the bug,
>> and patch 4/4 is just a natural extension of that change. Since I'm
>> going to be sending out another patchset anyway without the module.c
>> cleanups, I'll just keep them together.
>
>Yes, 3/4 should be sufficient to fix the bug. However if you take 4/4 too,
>you need 1/4 as well. Otherwise we would introduce a bug in error handling
>as Petr pointed out.
>

Hm. I am just realizing that patch 4/4 will still need new ACK's for
the error handling portion. What I'll do is, after testing, send out
patch 3/4 ("ftrace/module: remove ftrace module notifier") as a
standalone patch to be merged immediately, since it fixes an actual
bug. The rest of this patchset will follow separately and can be
reviewed at its own pace.

Jessica

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Rusty Russell]

Jiri Kosina <jikos@kernel.org> writes:
> On Wed, 10 Feb 2016, Rusty Russell 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.
>> 
>> Without a Fixes: line, it's not absolutely clear whether this needs
>> CC:stable, needs to go to Linus now, or can wait for the next merge
>> window.
>> 
>> I *think* you want all four merged this merge window, and 3 and 4 are
>> required to fix a regression introduced since 4.4...
>
> Your understanding is correct; #3 and #4 are needed to fix a 4.4 
> regression. It makes sense for the whole lot go to together, but for #1 
> and #2 I absolutely need your Ack before I take it to my tree, as I don't 
> want to be merging this behind your back.
>
> Once you Ack #1 and #2, I plan to take this to Linus immediately so that 
> we avoid doing these changes as very last minute.

Sorry Jiri, I am on paternity leave.  Am happy with all these patches;
please use your best judgement:

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

Re: livepatch/module: remove livepatch module notifier

[Jessica Yu]

+++ Rusty Russell [29/02/16 11:00 +1030]:
>Jiri Kosina <jikos@kernel.org> writes:
>> On Wed, 10 Feb 2016, Rusty Russell 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.
>>>
>>> Without a Fixes: line, it's not absolutely clear whether this needs
>>> CC:stable, needs to go to Linus now, or can wait for the next merge
>>> window.
>>>
>>> I *think* you want all four merged this merge window, and 3 and 4 are
>>> required to fix a regression introduced since 4.4...
>>
>> Your understanding is correct; #3 and #4 are needed to fix a 4.4
>> regression. It makes sense for the whole lot go to together, but for #1
>> and #2 I absolutely need your Ack before I take it to my tree, as I don't
>> want to be merging this behind your back.
>>
>> Once you Ack #1 and #2, I plan to take this to Linus immediately so that
>> we avoid doing these changes as very last minute.
>
>Sorry Jiri, I am on paternity leave.  Am happy with all these patches;
>please use your best judgement:
>
>Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>

Thanks Rusty. No worries, we got the regression out of the way already. :-)

Jiri, I'll resend the remaining 3 patches in the series separately,
since we've merged the ftrace patch already.

Jessica

Re: [PATCH v4 4/4] livepatch/module: remove livepatch module notifier

[Steven Rostedt]

On Tue, 16 Feb 2016 00:42:25 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:


> Steven, I'd appreciate if you could tell me whether your Ack to 
> "ftrace/module: remove ftrace module notifier" still holds even if 
> module.c changes are not happening.

When threads get this busy, and I'm busy on other things, I don't
always read the entire email. You may want to send me a separate email
when you want to get my attention in deep threads like this.

Anyway, I guess this all got pulled into Linus's tree anyway.

-- Steve