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

[PATCH v3 0/2] 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 can be found here -
https://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com
v2 can be found here -
https://lkml.kernel.org/g/1454375856-27757-1-git-send-email-jeyu@redhat.com

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 (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   | 153 +++++++++++++++++++++++-----------------------
 kernel/module.c           |  24 +++++++-
 kernel/trace/ftrace.c     |  36 +----------
 5 files changed, 112 insertions(+), 116 deletions(-)

-- 
2.4.3

[PATCH v3 1/2] 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.

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 9537da3..794ebe8 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;
@@ -3389,6 +3392,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

Re: [PATCH v3 1/2] ftrace/module: remove ftrace module notifier

[Petr Mladek]

On Fri 2016-02-05 22:08:16, 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>

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

Best Regards,
Petr

Re: [PATCH v3 1/2] ftrace/module: remove ftrace module notifier

[Josh Poimboeuf]

On Fri, Feb 05, 2016 at 10:08:16PM -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.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

This is also fixing the bug described in the cover letter, so it would
be good to add a description of that here and a corresponding "Fixes:"
tag.

-- 
Josh

[PATCH v3 2/2] 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>
---
 include/linux/livepatch.h |   9 +++
 kernel/livepatch/core.c   | 153 +++++++++++++++++++++++-----------------------
 kernel/module.c           |  20 +++++-
 3 files changed, 103 insertions(+), 79 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..1d47f96 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,112 @@ 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("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;
+			}
 
-disabled:
-	klp_free_object_loaded(obj);
+			break;
+		}
+	}
+
+	mutex_unlock(&klp_mutex);
+
+	return 0;
+
+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);
+	obj->mod = NULL;
+	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;
+	/*
+	 * Module state is normally GOING when this is called, but it could
+	 * still be COMING in cases where we are cleaning up after a module
+	 * that failed to load.
+	 */
+	if (WARN_ON(mod->state != MODULE_STATE_GOING &&
+		    mod->state != MODULE_STATE_COMING))
+		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 +982,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 794ebe8..0fec949 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);
@@ -3377,8 +3380,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
 
 	/* Find duplicate symbols (must be called under lock). */
 	err = verify_export_symbols(mod);
-	if (err < 0)
-		goto out;
+	if (err < 0) {
+		mutex_unlock(&module_mutex);
+		return err;
+	}
 
 	/* This relies on module_mutex for list integrity. */
 	module_bug_finalize(info->hdr, info->sechdrs, mod);
@@ -3393,12 +3398,22 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mutex_unlock(&module_mutex);
 
 	ftrace_module_enable(mod);
+	err = klp_module_coming(mod);
+	if (err)
+		goto out;
+
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
 	return 0;
 
 out:
+	mutex_lock(&module_mutex);
+	module_bug_cleanup(mod);
 	mutex_unlock(&module_mutex);
+
+	module_disable_ro(mod);
+	module_disable_nx(mod);
+
 	return err;
 }
 
@@ -3549,6 +3564,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_going(mod);
 
 	/* we can't deallocate the module until we clear memory protection */
 	module_disable_ro(mod);
-- 
2.4.3

Re: [PATCH v3 2/2] livepatch/module: remove livepatch module notifier

[Petr Mladek]

On Fri 2016-02-05 22:08:17, 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>
> ---
>  include/linux/livepatch.h |   9 +++
>  kernel/livepatch/core.c   | 153 +++++++++++++++++++++++-----------------------
>  kernel/module.c           |  20 +++++-
>  3 files changed, 103 insertions(+), 79 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..1d47f96 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> +int klp_module_coming(struct module *mod)
[...]
> +			obj->mod = 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;
> +			}
[...]
> +			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;
> +			}
[...]
> +
> +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);
> +	obj->mod = NULL;

To be on the safe side, we should replace this assignment with:

	klp_free_object_loaded(mod);

It clears obj->mod and also all func->old_addr for this module.
By other words, it leaves the structures in the same state as
klp_module_going(). I am sorry that I have missed this before.

Otherwise, it looks fine. So, with the above suggested change:

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

Best Regards,
Petr

Re: [PATCH v3 0/2] Fix ordering of ftrace/livepatch calls on module load and unload

[Josh Poimboeuf]

On Fri, Feb 05, 2016 at 10:08:15PM -0500, 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.

Since Rusty agreed to your suggested changes for splitting up
complete_formation() and for setting mod->state = MODULE_STATE_GOING
before calling the going notifiers in the error path, can you do a v4
with those changes?  They should probably be split up like:

1. split up complete_formation()
2. set MODULE_STATE_GOING before calling going notifiers in error path
3. remove ftrace module notifier
4. remove livepatch module notifier

-- 
Josh

Re: Fix ordering of ftrace/livepatch calls on module load and unload

[Jessica Yu]

+++ Josh Poimboeuf [08/02/16 11:48 -0600]:
>On Fri, Feb 05, 2016 at 10:08:15PM -0500, 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.
>
>Since Rusty agreed to your suggested changes for splitting up
>complete_formation() and for setting mod->state = MODULE_STATE_GOING
>before calling the going notifiers in the error path, can you do a v4
>with those changes?  They should probably be split up like:
>
>1. split up complete_formation()
>2. set MODULE_STATE_GOING before calling going notifiers in error path
>3. remove ftrace module notifier
>4. remove livepatch module notifier

Sure, I'll do that.

Jessica

Re: Fix ordering of ftrace/livepatch calls on module load and unload

[Jiri Kosina]

On Mon, 8 Feb 2016, Jessica Yu wrote:

> > 1. split up complete_formation()
> > 2. set MODULE_STATE_GOING before calling going notifiers in error path
> > 3. remove ftrace module notifier
> > 4. remove livepatch module notifier
> 
> Sure, I'll do that.

Also, please when sending out v4, don't forget to include all the 
Acked-by/Reviewed-by's received so far (for 1/2 you have the substantial 
ones already :) ).

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs

[PATCH v3 0/2] Livepatch module notifier cleanup

[Jessica Yu]

These are the remaining 2 patches that came from the original ftrace/livepatch
module notifier patchset found here: https://lkml.org/lkml/2016/2/8/1180

Basically, the patchset does a bit of module.c cleanup (patch 1) in
preparation for the klp_module_{coming,going} calls (patch 2). We decided
to stop relying on the module notifier callchain in favor of hard-coding
the appropriate livepatch function calls that handle coming and going
modules. Hard-coding these calls will guarantee that ftrace and livepatch
exit/initialization routines are called in the correct order without
relying on module notifiers.

The patches should be nearly exactly the same as those from the previous
discussion, except in patch 2 I've added back the #if IS_ENABLED(CONFIG_LIVEPATCH)
guard in livepatch.h.

Patches based on linux-next.

v2 discussion found here:
https://lkml.kernel.org/g/1457726628-9171-1-git-send-email-jeyu@redhat.com 

v3:
 - Remove confusing comment above klp_module_going()
 - Place prepare_coming_module() after complete_formation()
 - Fix small race in klp_module_coming() by setting mod->klp_alive
   to false in the error path

v2:
  - we don't need to change mod->state to GOING when cleaning up during
    failed module load
  - modify mod->state check to allow klp_module_going() to be called in
    both COMING and GOING states

Jessica Yu (2):
  modules: split part of complete_formation() into
    prepare_coming_module()
  livepatch/module: remove livepatch module notifier

 include/linux/livepatch.h |  13 ++++
 kernel/livepatch/core.c   | 147 ++++++++++++++++++++++------------------------
 kernel/module.c           |  36 +++++++++---
 3 files changed, 112 insertions(+), 84 deletions(-)

-- 
2.4.3

[PATCH v3 2/2] 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.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
Reviewed-by: Petr Mladek <pmladek@suse.cz> 
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/livepatch.h |  13 ++++
 kernel/livepatch/core.c   | 147 ++++++++++++++++++++++------------------------
 kernel/module.c           |  10 ++++
 3 files changed, 94 insertions(+), 76 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index c056797..bd830d5 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -24,6 +24,8 @@
 #include <linux/module.h>
 #include <linux/ftrace.h>
 
+#if IS_ENABLED(CONFIG_LIVEPATCH)
+
 #include <asm/livepatch.h>
 
 enum klp_state {
@@ -132,4 +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 780f00c..d68fbf6 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,108 @@ 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);
+	mod->klp_alive = false;
 	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 &&
+		    mod->state != MODULE_STATE_COMING))
+		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 +978,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 a5e0089..041200c 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();
@@ -3258,6 +3260,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);
@@ -3344,7 +3347,13 @@ out:
 
 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;
@@ -3496,6 +3505,7 @@ 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_going(mod);
 
  bug_cleanup:
 	/* module_bug_cleanup needs module_mutex protection */
-- 
2.4.3