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

[PATCH v2 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 -
http://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com

v2:
- Instead of splitting the ftrace and livepatch notifiers into coming + going
  notifiers and adjusting their priorities, remove ftrace and livepatch notifiers
  completely and hard-code the necessary function calls in the module loader.

[1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com


Jessica Yu (2):
  ftrace/module: remove ftrace module notifier
  livepatch/module: remove livepatch module notifier

 include/linux/ftrace.h    |   6 +-
 include/linux/livepatch.h |   9 +++
 kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
 kernel/module.c           |  12 ++++
 kernel/trace/ftrace.c     |  36 +-----------
 5 files changed, 95 insertions(+), 112 deletions(-)

-- 
2.4.3

[PATCH v2 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 8358f46..b05d466 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -981,6 +981,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		mod->exit();
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	ftrace_release_mod(mod);
+
 	async_synchronize_full();
 
 	/* Store the name of the last unloaded module for diagnostic purposes */
@@ -3295,6 +3297,7 @@ fail:
 	module_put(mod);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	ftrace_release_mod(mod);
 	free_module(mod);
 	wake_up_all(&module_wq);
 	return ret;
@@ -3371,6 +3374,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mod->state = MODULE_STATE_COMING;
 	mutex_unlock(&module_mutex);
 
+	ftrace_module_enable(mod);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
 	return 0;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index eca592f..57a6eea 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4961,7 +4961,7 @@ void ftrace_release_mod(struct module *mod)
 	mutex_unlock(&ftrace_lock);
 }
 
-static void ftrace_module_enable(struct module *mod)
+void ftrace_module_enable(struct module *mod)
 {
 	struct dyn_ftrace *rec;
 	struct ftrace_page *pg;
@@ -5038,38 +5038,8 @@ void ftrace_module_init(struct module *mod)
 	ftrace_process_locs(mod, mod->ftrace_callsites,
 			    mod->ftrace_callsites + mod->num_ftrace_callsites);
 }
-
-static int ftrace_module_notify(struct notifier_block *self,
-				unsigned long val, void *data)
-{
-	struct module *mod = data;
-
-	switch (val) {
-	case MODULE_STATE_COMING:
-		ftrace_module_enable(mod);
-		break;
-	case MODULE_STATE_GOING:
-		ftrace_release_mod(mod);
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-#else
-static int ftrace_module_notify(struct notifier_block *self,
-				unsigned long val, void *data)
-{
-	return 0;
-}
 #endif /* CONFIG_MODULES */
 
-struct notifier_block ftrace_module_nb = {
-	.notifier_call = ftrace_module_notify,
-	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
-};
-
 void __init ftrace_init(void)
 {
 	extern unsigned long __start_mcount_loc[];
@@ -5098,10 +5068,6 @@ void __init ftrace_init(void)
 				  __start_mcount_loc,
 				  __stop_mcount_loc);
 
-	ret = register_module_notifier(&ftrace_module_nb);
-	if (ret)
-		pr_warning("Failed to register trace ftrace module exit notifier\n");
-
 	set_ftrace_early_filters();
 
 	return;
-- 
2.4.3

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

[Petr Mladek]

On Mon 2016-02-01 20:17:35, Jessica Yu wrote:
> Remove the ftrace module notifier in favor of directly calling
> ftrace_module_enable() and ftrace_release_mod() in the module loader.
> Hard-coding the function calls directly in the module loader removes
> dependence on the module notifier call chain and provides better
> visibility and control over what gets called when, which is important
> to kernel utilities such as livepatch.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/ftrace.h |  6 ++++--
>  kernel/module.c        |  4 ++++
>  kernel/trace/ftrace.c  | 36 +-----------------------------------
>  3 files changed, 9 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 81de712..c2b340e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -603,6 +603,7 @@ extern int ftrace_arch_read_dyn_info(char *buf, int size);
>  
>  extern int skip_trace(unsigned long ip);
>  extern void ftrace_module_init(struct module *mod);
> +extern void ftrace_module_enable(struct module *mod);
>  extern void ftrace_release_mod(struct module *mod);
>  
>  extern void ftrace_disable_daemon(void);
> @@ -612,8 +613,9 @@ static inline int skip_trace(unsigned long ip) { return 0; }
>  static inline int ftrace_force_update(void) { return 0; }
>  static inline void ftrace_disable_daemon(void) { }
>  static inline void ftrace_enable_daemon(void) { }
> -static inline void ftrace_release_mod(struct module *mod) {}
> -static inline void ftrace_module_init(struct module *mod) {}
> +static inline void ftrace_module_init(struct module *mod) { }
> +static inline void ftrace_module_enable(struct module *mod) { }
> +static inline void ftrace_release_mod(struct module *mod) { }
>  static inline __init int register_ftrace_command(struct ftrace_func_command *cmd)
>  {
>  	return -EINVAL;
> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f46..b05d466 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -981,6 +981,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		mod->exit();
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	ftrace_release_mod(mod);
> +
>  	async_synchronize_full();
>  
>  	/* Store the name of the last unloaded module for diagnostic purposes */
> @@ -3295,6 +3297,7 @@ fail:
>  	module_put(mod);
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	ftrace_release_mod(mod);
>  	free_module(mod);
>  	wake_up_all(&module_wq);
>  	return ret;
> @@ -3371,6 +3374,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mod->state = MODULE_STATE_COMING;
>  	mutex_unlock(&module_mutex);
>  
> +	ftrace_module_enable(mod);
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_COMING, mod);
>  	return 0;

Also we need to call ftrace_release_mod() in bug_cleanup:
goto target in load_module(). Otherwise, it will stay
enabled when, e.g. parse_args() fails.

Best Regards,
Petr

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

[Steven Rostedt]

On Thu, 4 Feb 2016 14:27:51 +0100
Petr Mladek <pmladek@suse.com> wrote:


> > +	ftrace_module_enable(mod);
> >  	blocking_notifier_call_chain(&module_notify_list,
> >  				     MODULE_STATE_COMING, mod);
> >  	return 0;  
> 
> Also we need to call ftrace_release_mod() in bug_cleanup:
> goto target in load_module(). Otherwise, it will stay
> enabled when, e.g. parse_args() fails.

Look farther down (after free_module:), it's already there.

-- Steve

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

[Petr Mladek]

On Thu 2016-02-04 09:18:01, Steven Rostedt wrote:
> On Thu, 4 Feb 2016 14:27:51 +0100
> Petr Mladek <pmladek@suse.com> wrote:
> 
> 
> > > +	ftrace_module_enable(mod);
> > >  	blocking_notifier_call_chain(&module_notify_list,
> > >  				     MODULE_STATE_COMING, mod);
> > >  	return 0;  
> > 
> > Also we need to call ftrace_release_mod() in bug_cleanup:
> > goto target in load_module(). Otherwise, it will stay
> > enabled when, e.g. parse_args() fails.
> 
> Look farther down (after free_module:), it's already there.

Ah, I see. ftrace_release_mod() is called there on an unexpected
location. Error paths typically do actions in the reverse order
in compare with the normal paths :-)

Thanks for the pointer,
Petr

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

[Steven Rostedt]

On Thu, 4 Feb 2016 16:21:20 +0100
Petr Mladek <pmladek@suse.com> wrote:

> Ah, I see. ftrace_release_mod() is called there on an unexpected
> location. Error paths typically do actions in the reverse order
> in compare with the normal paths :-)

And that's why there's a comment above it describing why it's not in
the normal "reverse" location.

-- Steve

[PATCH v2 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_enable() during module load, and that klp_module_disable() is
run before ftrace_release_mod() during module unload. This way, ftrace
and livepatch code is run in the correct order during the module
load/unload sequence without dependence on the module notifier call chain.

This fixes a notifier ordering issue in which the ftrace module notifier
(and hence ftrace_module_enable()) for coming modules was being called
after klp_module_notify(), which caused livepatch modules to initialize
incorrectly.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/linux/livepatch.h |   9 +++
 kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
 kernel/module.c           |   8 +++
 3 files changed, 86 insertions(+), 75 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a882865..fdd5f1c 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
+/* Called from the module loader during module coming/going states */
+extern int klp_module_enable(struct module *mod);
+extern void klp_module_disable(struct module *mod);
+
+#else /* !CONFIG_LIVEPATCH */
+
+static inline int klp_module_enable(struct module *mod) { return 0; }
+static inline void klp_module_disable(struct module *mod) { }
+
 #endif /* CONFIG_LIVEPATCH */
 
 #endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc2c85c..7aa975d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj)
 	 */
 	mod = find_module(obj->name);
 	/*
-	 * Do not mess work of the module coming and going notifiers.
+	 * Do not mess work of the klp module coming and going handlers.
 	 * Note that the patch might still be needed before the going handler
 	 * is called. Module functions can be called even in the GOING state
 	 * until mod->exit() finishes. This is especially important for
@@ -866,103 +866,107 @@ int klp_register_patch(struct klp_patch *patch)
 }
 EXPORT_SYMBOL_GPL(klp_register_patch);
 
-static int klp_module_notify_coming(struct klp_patch *patch,
-				     struct klp_object *obj)
+/* Called when module state is MODULE_STATE_COMING */
+int klp_module_enable(struct module *mod)
 {
-	struct module *pmod = patch->mod;
-	struct module *mod = obj->mod;
 	int ret;
+	struct klp_patch *patch;
+	struct klp_object *obj;
 
-	ret = klp_init_object_loaded(patch, obj);
-	if (ret) {
-		pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
-			pmod->name, mod->name, ret);
-		return ret;
-	}
-
-	if (patch->state == KLP_DISABLED)
+	if (mod->state != MODULE_STATE_COMING)
 		return 0;
 
-	pr_notice("applying patch '%s' to loading module '%s'\n",
-		  pmod->name, mod->name);
+	mutex_lock(&klp_mutex);
+	/*
+	 * Each module has to know that the coming handler has
+	 * been called. We never know what module will get
+	 * patched by a new patch.
+	 */
+	mod->klp_alive = true;
 
-	ret = klp_enable_object(obj);
-	if (ret)
-		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
-			pmod->name, mod->name, ret);
-	return ret;
-}
+	list_for_each_entry(patch, &klp_patches, list) {
+		klp_for_each_object(patch, obj) {
+			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
+				continue;
 
-static void klp_module_notify_going(struct klp_patch *patch,
-				    struct klp_object *obj)
-{
-	struct module *pmod = patch->mod;
-	struct module *mod = obj->mod;
+			obj->mod = mod;
 
-	if (patch->state == KLP_DISABLED)
-		goto disabled;
+			ret = klp_init_object_loaded(patch, obj);
+			if (ret) {
+				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
+					patch->mod->name, obj->mod->name, ret);
+				goto err;
+			}
+
+			if (patch->state == KLP_DISABLED)
+				break;
 
-	pr_notice("reverting patch '%s' on unloading module '%s'\n",
-		  pmod->name, mod->name);
+			pr_notice("applying patch '%s' to loading module '%s'\n",
+				  patch->mod->name, obj->mod->name);
 
-	klp_disable_object(obj);
+			ret = klp_enable_object(obj);
+			if (ret) {
+				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
+					patch->mod->name, obj->mod->name, ret);
+				goto err;
+			}
+
+			break;
+		}
+	}
+
+	mutex_unlock(&klp_mutex);
 
-disabled:
-	klp_free_object_loaded(obj);
+	return 0;
+
+err:
+	/*
+	 * If a patch is unsuccessfully applied, return
+	 * error to the module loader.
+	 */
+	obj->mod = NULL;
+	pr_warn("patch '%s' is in an inconsistent state!\n", patch->mod->name);
+	mutex_unlock(&klp_mutex);
+
+	return ret;
 }
 
-static int klp_module_notify(struct notifier_block *nb, unsigned long action,
-			     void *data)
+/* Called when module state is MODULE_STATE_GOING */
+void klp_module_disable(struct module *mod)
 {
-	int ret;
-	struct module *mod = data;
 	struct klp_patch *patch;
 	struct klp_object *obj;
 
-	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
-		return 0;
+	if (mod->state != MODULE_STATE_GOING)
+		return;
 
 	mutex_lock(&klp_mutex);
-
 	/*
-	 * Each module has to know that the notifier has been called.
-	 * We never know what module will get patched by a new patch.
+	 * Each module has to know that the going handler
+	 * has been called. We never know what module will
+	 * get patched by a new patch.
 	 */
-	if (action == MODULE_STATE_COMING)
-		mod->klp_alive = true;
-	else /* MODULE_STATE_GOING */
-		mod->klp_alive = false;
+	mod->klp_alive = false;
 
 	list_for_each_entry(patch, &klp_patches, list) {
 		klp_for_each_object(patch, obj) {
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
 
-			if (action == MODULE_STATE_COMING) {
-				obj->mod = mod;
-				ret = klp_module_notify_coming(patch, obj);
-				if (ret) {
-					obj->mod = NULL;
-					pr_warn("patch '%s' is in an inconsistent state!\n",
-						patch->mod->name);
-				}
-			} else /* MODULE_STATE_GOING */
-				klp_module_notify_going(patch, obj);
+			if (patch->state != KLP_DISABLED) {
+				pr_notice("reverting patch '%s' on unloading module '%s'\n",
+					  patch->mod->name, obj->mod->name);
+				klp_disable_object(obj);
+			}
 
+			klp_free_object_loaded(obj);
 			break;
 		}
 	}
 
 	mutex_unlock(&klp_mutex);
-
-	return 0;
 }
 
-static struct notifier_block klp_module_nb = {
-	.notifier_call = klp_module_notify,
-	.priority = INT_MIN+1, /* called late but before ftrace notifier */
-};
-
 static int __init klp_init(void)
 {
 	int ret;
@@ -973,21 +977,11 @@ static int __init klp_init(void)
 		return -EINVAL;
 	}
 
-	ret = register_module_notifier(&klp_module_nb);
-	if (ret)
-		return ret;
-
 	klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj);
-	if (!klp_root_kobj) {
-		ret = -ENOMEM;
-		goto unregister;
-	}
+	if (!klp_root_kobj)
+		return -ENOMEM;
 
 	return 0;
-
-unregister:
-	unregister_module_notifier(&klp_module_nb);
-	return ret;
 }
 
 module_init(klp_init);
diff --git a/kernel/module.c b/kernel/module.c
index b05d466..71c77ed 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -53,6 +53,7 @@
 #include <asm/sections.h>
 #include <linux/tracepoint.h>
 #include <linux/ftrace.h>
+#include <linux/livepatch.h>
 #include <linux/async.h>
 #include <linux/percpu.h>
 #include <linux/kmemleak.h>
@@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		mod->exit();
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_disable(mod);
 	ftrace_release_mod(mod);
 
 	async_synchronize_full();
@@ -3297,6 +3299,7 @@ fail:
 	module_put(mod);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_disable(mod);
 	ftrace_release_mod(mod);
 	free_module(mod);
 	wake_up_all(&module_wq);
@@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mutex_unlock(&module_mutex);
 
 	ftrace_module_enable(mod);
+	err = klp_module_enable(mod);
+	if (err)
+		goto out;
+
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
 	return 0;
@@ -3531,6 +3538,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_disable(mod);
 
 	/* we can't deallocate the module until we clear memory protection */
 	module_disable_ro(mod);
-- 
2.4.3

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

[Petr Mladek]

On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> Remove the livepatch module notifier in favor of directly enabling and
> disabling patches to modules in the module loader. Hard-coding the
> function calls ensures that ftrace_module_enable() is run before
> klp_module_enable() during module load, and that klp_module_disable() is
> run before ftrace_release_mod() during module unload. This way, ftrace
> and livepatch code is run in the correct order during the module
> load/unload sequence without dependence on the module notifier call chain.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/livepatch.h |   9 +++
>  kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
>  kernel/module.c           |   8 +++
>  3 files changed, 86 insertions(+), 75 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a882865..fdd5f1c 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
>  int klp_disable_patch(struct klp_patch *);
>  
> +/* Called from the module loader during module coming/going states */
> +extern int klp_module_enable(struct module *mod);
> +extern void klp_module_disable(struct module *mod);
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline int klp_module_enable(struct module *mod) { return 0; }
> +static inline void klp_module_disable(struct module *mod) { }
> +
>  #endif /* CONFIG_LIVEPATCH */
>  
>  #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..7aa975d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj)
>  	 */
>  	mod = find_module(obj->name);
>  	/*
> -	 * Do not mess work of the module coming and going notifiers.
> +	 * Do not mess work of the klp module coming and going handlers.

This is a bit confusing because you removed all functions called
*coming* and *going*. I would say something like:

	* Do not mess work of klp_module_enable() and klp_module_disable().


>  	 * Note that the patch might still be needed before the going handler
>  	 * is called. Module functions can be called even in the GOING state
>  	 * until mod->exit() finishes. This is especially important for
> @@ -866,103 +866,107 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static int klp_module_notify_coming(struct klp_patch *patch,
> -				     struct klp_object *obj)
> +/* Called when module state is MODULE_STATE_COMING */
> +int klp_module_enable(struct module *mod)
>  {
> -	struct module *pmod = patch->mod;
> -	struct module *mod = obj->mod;
>  	int ret;
> +	struct klp_patch *patch;
> +	struct klp_object *obj;
>  
> -	ret = klp_init_object_loaded(patch, obj);
> -	if (ret) {
> -		pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> -			pmod->name, mod->name, ret);
> -		return ret;
> -	}
> -
> -	if (patch->state == KLP_DISABLED)
> +	if (mod->state != MODULE_STATE_COMING)
>  		return 0;

The function is not longer called from another state. I would replace
this by:

	if (WARN_ON(mod->state != MODULE_STATE_COMING))
		return -EINVAL;


> -	pr_notice("applying patch '%s' to loading module '%s'\n",
> -		  pmod->name, mod->name);
> +	mutex_lock(&klp_mutex);
> +	/*
> +	 * Each module has to know that the coming handler has
> +	 * been called. We never know what module will get
> +	 * patched by a new patch.
> +	 */
> +	mod->klp_alive = true;
>  
> -	ret = klp_enable_object(obj);
> -	if (ret)
> -		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> -			pmod->name, mod->name, ret);
> -	return ret;
> -}
> +	list_for_each_entry(patch, &klp_patches, list) {
> +		klp_for_each_object(patch, obj) {
> +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +				continue;
>  
> -static void klp_module_notify_going(struct klp_patch *patch,
> -				    struct klp_object *obj)
> -{
> -	struct module *pmod = patch->mod;
> -	struct module *mod = obj->mod;
> +			obj->mod = mod;
>  
> -	if (patch->state == KLP_DISABLED)
> -		goto disabled;
> +			ret = klp_init_object_loaded(patch, obj);
> +			if (ret) {
> +				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> +					patch->mod->name, obj->mod->name, ret);
> +				goto err;
> +			}
> +
> +			if (patch->state == KLP_DISABLED)
> +				break;
>  
> -	pr_notice("reverting patch '%s' on unloading module '%s'\n",
> -		  pmod->name, mod->name);
> +			pr_notice("applying patch '%s' to loading module '%s'\n",
> +				  patch->mod->name, obj->mod->name);
>  
> -	klp_disable_object(obj);
> +			ret = klp_enable_object(obj);
> +			if (ret) {
> +				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> +					patch->mod->name, obj->mod->name, ret);
> +				goto err;
> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&klp_mutex);
>  
> -disabled:
> -	klp_free_object_loaded(obj);
> +	return 0;
> +
> +err:
> +	/*
> +	 * If a patch is unsuccessfully applied, return
> +	 * error to the module loader.
> +	 */
> +	obj->mod = NULL;
> +	pr_warn("patch '%s' is in an inconsistent state!\n", patch->mod->name);

This message is not correct. The module will not get loaded
when the patch is not applied.

Instead, we need to revert all the operations that has already
been done for this module. Note that the module stayed loaded
before, so we did not need to release any memory or revert
any ftrace call registration but we need to do so now!


> +	mutex_unlock(&klp_mutex);
> +
> +	return ret;
>  }
>  
> -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> -			     void *data)
> +/* Called when module state is MODULE_STATE_GOING */
> +void klp_module_disable(struct module *mod)
>  {
> -	int ret;
> -	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
>  
> -	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> -		return 0;
> +	if (mod->state != MODULE_STATE_GOING)
> +		return;
>  
>  	mutex_lock(&klp_mutex);
> -
>  	/*
> -	 * Each module has to know that the notifier has been called.
> -	 * We never know what module will get patched by a new patch.
> +	 * Each module has to know that the going handler
> +	 * has been called. We never know what module will
> +	 * get patched by a new patch.
>  	 */
> -	if (action == MODULE_STATE_COMING)
> -		mod->klp_alive = true;
> -	else /* MODULE_STATE_GOING */
> -		mod->klp_alive = false;
> +	mod->klp_alive = false;
>  
>  	list_for_each_entry(patch, &klp_patches, list) {
>  		klp_for_each_object(patch, obj) {
>  			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
>  				continue;
>  
> -			if (action == MODULE_STATE_COMING) {
> -				obj->mod = mod;
> -				ret = klp_module_notify_coming(patch, obj);
> -				if (ret) {
> -					obj->mod = NULL;
> -					pr_warn("patch '%s' is in an inconsistent state!\n",
> -						patch->mod->name);
> -				}
> -			} else /* MODULE_STATE_GOING */
> -				klp_module_notify_going(patch, obj);
> +			if (patch->state != KLP_DISABLED) {
> +				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> +					  patch->mod->name, obj->mod->name);
> +				klp_disable_object(obj);
> +			}
>  
> +			klp_free_object_loaded(obj);
>  			break;
>  		}
>  	}
>  
>  	mutex_unlock(&klp_mutex);
> -
> -	return 0;
>  }
>  
> -static struct notifier_block klp_module_nb = {
> -	.notifier_call = klp_module_notify,
> -	.priority = INT_MIN+1, /* called late but before ftrace notifier */
> -};
> -
>  static int __init klp_init(void)
>  {
>  	int ret;
> @@ -973,21 +977,11 @@ static int __init klp_init(void)
>  		return -EINVAL;
>  	}
>  
> -	ret = register_module_notifier(&klp_module_nb);
> -	if (ret)
> -		return ret;
> -
>  	klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj);
> -	if (!klp_root_kobj) {
> -		ret = -ENOMEM;
> -		goto unregister;
> -	}
> +	if (!klp_root_kobj)
> +		return -ENOMEM;
>  
>  	return 0;
> -
> -unregister:
> -	unregister_module_notifier(&klp_module_nb);
> -	return ret;
>  }
>  
>  module_init(klp_init);
> diff --git a/kernel/module.c b/kernel/module.c
> index b05d466..71c77ed 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -53,6 +53,7 @@
>  #include <asm/sections.h>
>  #include <linux/tracepoint.h>
>  #include <linux/ftrace.h>
> +#include <linux/livepatch.h>
>  #include <linux/async.h>
>  #include <linux/percpu.h>
>  #include <linux/kmemleak.h>
> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		mod->exit();
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	klp_module_disable(mod);
>  	ftrace_release_mod(mod);
>  
>  	async_synchronize_full();
> @@ -3297,6 +3299,7 @@ fail:
>  	module_put(mod);
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	klp_module_disable(mod);
>  	ftrace_release_mod(mod);
>  	free_module(mod);
>  	wake_up_all(&module_wq);
> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mutex_unlock(&module_mutex);
>  
>  	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod);
> +	if (err)
> +		goto out;

If you go out here, you need to revert some some operations
that are normally done in the bug_cleanup: goto target
in load_module(). In particular, you need to do:

	/* module_bug_cleanup needs module_mutex protection */
	mutex_lock(&module_mutex);
	module_bug_cleanup(mod);
	mutex_unlock(&module_mutex);

	ftrace_release_mod(mod);

	/* we can't deallocate the module until we clear memory protection */
	module_disable_ro(mod);
	module_disable_nx(mod);


IMHO, it would make sense to somehow split the complete_formation() function
and avoid a code duplication in the error paths.

Best Regards,
Petr

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

[Steven Rostedt]

On Thu, 4 Feb 2016 15:39:35 +0100
Petr Mladek <pmladek@suse.com> wrote:


> > @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
> >  	mutex_unlock(&module_mutex);
> >  
> >  	ftrace_module_enable(mod);
> > +	err = klp_module_enable(mod);
> > +	if (err)
> > +		goto out;  
> 
> If you go out here, you need to revert some some operations
> that are normally done in the bug_cleanup: goto target
> in load_module(). In particular, you need to do:
> 
> 	/* module_bug_cleanup needs module_mutex protection */
> 	mutex_lock(&module_mutex);
> 	module_bug_cleanup(mod);
> 	mutex_unlock(&module_mutex);
> 
> 	ftrace_release_mod(mod);
> 
> 	/* we can't deallocate the module until we clear memory protection */
> 	module_disable_ro(mod);
> 	module_disable_nx(mod);
> 
> 
> IMHO, it would make sense to somehow split the complete_formation() function
> and avoid a code duplication in the error paths.

If complete_formation() fails, load_module will do a goto
ddebug_cleanup, which will eventually call ftrace_release_mod(). No
need to do it here.

-- Steve

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

[Miroslav Benes]

On Thu, 4 Feb 2016, Petr Mladek wrote:

> On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> >
> >  
> > -	if (patch->state == KLP_DISABLED)
> > -		goto disabled;
> > +			ret = klp_init_object_loaded(patch, obj);
> > +			if (ret) {
> > +				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> > +					patch->mod->name, obj->mod->name, ret);
> > +				goto err;
> > +			}
> > +
> > +			if (patch->state == KLP_DISABLED)
> > +				break;
> >  
> > -	pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > -		  pmod->name, mod->name);
> > +			pr_notice("applying patch '%s' to loading module '%s'\n",
> > +				  patch->mod->name, obj->mod->name);
> >  
> > -	klp_disable_object(obj);
> > +			ret = klp_enable_object(obj);
> > +			if (ret) {
> > +				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > +					patch->mod->name, obj->mod->name, ret);
> > +				goto err;
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&klp_mutex);
> >  
> > -disabled:
> > -	klp_free_object_loaded(obj);
> > +	return 0;
> > +
> > +err:
> > +	/*
> > +	 * If a patch is unsuccessfully applied, return
> > +	 * error to the module loader.
> > +	 */
> > +	obj->mod = NULL;
> > +	pr_warn("patch '%s' is in an inconsistent state!\n", patch->mod->name);
> 
> This message is not correct. The module will not get loaded
> when the patch is not applied.

Yes, because we are in a better situation with this patch. We actually 
return an error and refuse to load the module. Message should take that 
into account.

> Instead, we need to revert all the operations that has already
> been done for this module. Note that the module stayed loaded
> before, so we did not need to release any memory or revert
> any ftrace call registration but we need to do so now!

Actually, I think the code is correct. If klp_init_object_loaded() there 
is no problem because we only write relocations there (which are written 
to the module being loaded) and resolve symbols via kallsyms. Nothing to 
revert there and it could be done again.

If klp_enable_object() fails, all the relevant error handling was already 
done there. See the call to klp_disable_object() if klp_enable_function() 
fails there.

Miroslav

Re: livepatch/module: remove livepatch module notifier

[Jessica Yu]

+++ Petr Mladek [04/02/16 15:39 +0100]:
>On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
[ snipped since email is getting long ]
>> diff --git a/kernel/module.c b/kernel/module.c
>> index b05d466..71c77ed 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -53,6 +53,7 @@
>>  #include <asm/sections.h>
>>  #include <linux/tracepoint.h>
>>  #include <linux/ftrace.h>
>> +#include <linux/livepatch.h>
>>  #include <linux/async.h>
>>  #include <linux/percpu.h>
>>  #include <linux/kmemleak.h>
>> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>>  		mod->exit();
>>  	blocking_notifier_call_chain(&module_notify_list,
>>  				     MODULE_STATE_GOING, mod);
>> +	klp_module_disable(mod);
>>  	ftrace_release_mod(mod);
>>
>>  	async_synchronize_full();
>> @@ -3297,6 +3299,7 @@ fail:
>>  	module_put(mod);
>>  	blocking_notifier_call_chain(&module_notify_list,
>>  				     MODULE_STATE_GOING, mod);
>> +	klp_module_disable(mod);
>>  	ftrace_release_mod(mod);
>>  	free_module(mod);
>>  	wake_up_all(&module_wq);
>> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>  	mutex_unlock(&module_mutex);
>>
>>  	ftrace_module_enable(mod);
>> +	err = klp_module_enable(mod);
>> +	if (err)
>> +		goto out;
>
>If you go out here, you need to revert some some operations
>that are normally done in the bug_cleanup: goto target
>in load_module(). In particular, you need to do:
>
>	/* module_bug_cleanup needs module_mutex protection */
>	mutex_lock(&module_mutex);
>	module_bug_cleanup(mod);
>	mutex_unlock(&module_mutex);
>
>	ftrace_release_mod(mod);
>
>	/* we can't deallocate the module until we clear memory protection */
>	module_disable_ro(mod);
>	module_disable_nx(mod);
>
>
>IMHO, it would make sense to somehow split the complete_formation() function
>and avoid a code duplication in the error paths.

Argh, thank you for catching that. I think we could split up complete_formation()
into two functions in order to make the error handling work. 

We could probably take out the coming notifier calls, ftrace_module_enable(),
and klp_module_enable() out of complete_formation(), and put them in another
function, maybe called prepare_coming_module(), that would be called right
after complete_formation(). It might look something like this:

@@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
        err = complete_formation(mod, info); 
        if (err)
                goto ddebug_cleanup;
+       err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers
+       if (err) // means that klp_module_enable failed
+               goto bug_cleanup;
 
        /* Module is ready to execute: parsing args may do that. */
        after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
@@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
                                  unknown_module_param_cb);
        if (IS_ERR(after_dashes)) {
                err = PTR_ERR(after_dashes);
-               goto bug_cleanup;
+               goto coming_cleanup;
        } else if (after_dashes) {
                pr_warn("%s: parameters '%s' after `--' ignored\n",
                       mod->name, after_dashes);

Now for the error conditions. If complete_formation() fails, goto
ddebug_cleanup. If prepare_coming_module() fails (at that point,
module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
ftrace_module_enable, and the coming notifiers have finished) goto
coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
so we don't have to call it in coming_module.

@@ -3649,16 +3659,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
+ coming_cleanup:
+       blocking_notifier_call_chain(&module_notify_list,
+                                    MODULE_STATE_GOING, mod);
+       klp_module_disable(mod);
  bug_cleanup:
        /* module_bug_cleanup needs module_mutex protection */
        mutex_lock(&module_mutex);
        module_bug_cleanup(mod);
        mutex_unlock(&module_mutex);
 
-       blocking_notifier_call_chain(&module_notify_list,
-                                    MODULE_STATE_GOING, mod);
-       klp_module_disable(mod);
-
        /* we can't deallocate the module until we clear memory protection */
        module_disable_ro(mod);
        module_disable_nx(mod);

Does all this look ok? Also, one last thing, I noticed that module->state isn't
set to MODULE_STATE_GOING anywhere before the going notifier chain is called in
the bug_cleanup label (I think it is still COMING at that point), so the
klp_module_disable call right afterwards would have bailed out because of that.
To be consistent, shouldn't it be set before the going notifiers are called?

Thanks,
Jessica

Re: livepatch/module: remove livepatch module notifier

[Miroslav Benes]

On Thu, 4 Feb 2016, Jessica Yu wrote:

> +++ Petr Mladek [04/02/16 15:39 +0100]:
> > On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> [ snipped since email is getting long ]
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index b05d466..71c77ed 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -53,6 +53,7 @@
> > >  #include <asm/sections.h>
> > >  #include <linux/tracepoint.h>
> > >  #include <linux/ftrace.h>
> > > +#include <linux/livepatch.h>
> > >  #include <linux/async.h>
> > >  #include <linux/percpu.h>
> > >  #include <linux/kmemleak.h>
> > > @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *,
> > > name_user,
> > >  		mod->exit();
> > >  	blocking_notifier_call_chain(&module_notify_list,
> > >  				     MODULE_STATE_GOING, mod);
> > > +	klp_module_disable(mod);
> > >  	ftrace_release_mod(mod);
> > > 
> > >  	async_synchronize_full();
> > > @@ -3297,6 +3299,7 @@ fail:
> > >  	module_put(mod);
> > >  	blocking_notifier_call_chain(&module_notify_list,
> > >  				     MODULE_STATE_GOING, mod);
> > > +	klp_module_disable(mod);
> > >  	ftrace_release_mod(mod);
> > >  	free_module(mod);
> > >  	wake_up_all(&module_wq);
> > > @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod,
> > > struct load_info *info)
> > >  	mutex_unlock(&module_mutex);
> > > 
> > >  	ftrace_module_enable(mod);
> > > +	err = klp_module_enable(mod);
> > > +	if (err)
> > > +		goto out;
> > 
> > If you go out here, you need to revert some some operations
> > that are normally done in the bug_cleanup: goto target
> > in load_module(). In particular, you need to do:
> > 
> > 	/* module_bug_cleanup needs module_mutex protection */
> > 	mutex_lock(&module_mutex);
> > 	module_bug_cleanup(mod);
> > 	mutex_unlock(&module_mutex);
> > 
> > 	ftrace_release_mod(mod);
> > 
> > 	/* we can't deallocate the module until we clear memory protection */
> > 	module_disable_ro(mod);
> > 	module_disable_nx(mod);
> > 
> > 
> > IMHO, it would make sense to somehow split the complete_formation() function
> > and avoid a code duplication in the error paths.
> 
> Argh, thank you for catching that. I think we could split up
> complete_formation()
> into two functions in order to make the error handling work. 
> We could probably take out the coming notifier calls, ftrace_module_enable(),
> and klp_module_enable() out of complete_formation(), and put them in another
> function, maybe called prepare_coming_module(), that would be called right
> after complete_formation(). It might look something like this:
> 
> @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const
> char __user *uargs,
>        err = complete_formation(mod, info);        if (err)
>                goto ddebug_cleanup;
> +       err = prepare_coming_module(mod); // calls ftrace_module_enable(),
> klp_module_enable(), then coming notifiers
> +       if (err) // means that klp_module_enable failed
> +               goto bug_cleanup;
> 
>        /* Module is ready to execute: parsing args may do that. */
>        after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const
> char __user *uargs,
>                                  unknown_module_param_cb);
>        if (IS_ERR(after_dashes)) {
>                err = PTR_ERR(after_dashes);
> -               goto bug_cleanup;
> +               goto coming_cleanup;
>        } else if (after_dashes) {
>                pr_warn("%s: parameters '%s' after `--' ignored\n",
>                       mod->name, after_dashes);
> 
> Now for the error conditions. If complete_formation() fails, goto
> ddebug_cleanup. If prepare_coming_module() fails (at that point,
> module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
> bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
> ftrace_module_enable, and the coming notifiers have finished) goto
> coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
> so we don't have to call it in coming_module.
> 
> @@ -3649,16 +3659,16 @@ static int load_module(struct load_info *info, const
> char __user *uargs,
> 
> + coming_cleanup:
> +       blocking_notifier_call_chain(&module_notify_list,
> +                                    MODULE_STATE_GOING, mod);
> +       klp_module_disable(mod);
>  bug_cleanup:
>        /* module_bug_cleanup needs module_mutex protection */
>        mutex_lock(&module_mutex);
>        module_bug_cleanup(mod);
>        mutex_unlock(&module_mutex);
> 
> -       blocking_notifier_call_chain(&module_notify_list,
> -                                    MODULE_STATE_GOING, mod);
> -       klp_module_disable(mod);
> -
>        /* we can't deallocate the module until we clear memory protection */
>        module_disable_ro(mod);
>        module_disable_nx(mod);
> 
> Does all this look ok? 

Hm, there is an another option. We can cover the needed error handling in 
complete_formation(). So for the first error there 
(verify_export_symbols() fails) we need to only release module_mutex. For 
our second error we need more. We would call module_bug_cleanup() under 
module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct? 
It would be hidden in complete_formation() this way and cleaner in my 
opinion. There is some code duplication though.

> Also, one last thing, I noticed that module->state
> isn't
> set to MODULE_STATE_GOING anywhere before the going notifier chain is called
> in
> the bug_cleanup label (I think it is still COMING at that point), so the
> klp_module_disable call right afterwards would have bailed out because of
> that.
> To be consistent, shouldn't it be set before the going notifiers are called?

This could break something or introduce a race somewhere. Git grep says 
there are several checks for MODULE_STATE_GOING through out the kernel 
which need to be checked.

Regards,
Miroslav

Re: livepatch/module: remove livepatch module notifier

[Petr Mladek]

On Fri 2016-02-05 10:15:56, Miroslav Benes wrote:
> On Thu, 4 Feb 2016, Jessica Yu wrote:
> > Argh, thank you for catching that. I think we could split up
> > complete_formation()
> > into two functions in order to make the error handling work. 
> > 
> > Does all this look ok? 
> 
> Hm, there is an another option. We can cover the needed error handling in 
> complete_formation(). So for the first error there 
> (verify_export_symbols() fails) we need to only release module_mutex. For 
> our second error we need more. We would call module_bug_cleanup() under 
> module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct? 
> It would be hidden in complete_formation() this way and cleaner in my 
> opinion. There is some code duplication though.

This sounds better to me.

> > Also, one last thing, I noticed that module->state
> > isn't
> > set to MODULE_STATE_GOING anywhere before the going notifier chain is called
> > in
> > the bug_cleanup label (I think it is still COMING at that point), so the
> > klp_module_disable call right afterwards would have bailed out because of
> > that.
> > To be consistent, shouldn't it be set before the going notifiers are called?
> 
> This could break something or introduce a race somewhere. Git grep says 
> there are several checks for MODULE_STATE_GOING through out the kernel 
> which need to be checked.

I think that we could relax the condition in the
klp_module_going/disable() function and allow to call it
also in MODULE_STATE_COMMING_STATE. It would deserve
a comment.

Best Regards,
Petr

Re: livepatch/module: remove livepatch module notifier

[Rusty Russell]

Jessica Yu <jeyu@redhat.com> writes:
> +++ Petr Mladek [04/02/16 15:39 +0100]:
>>On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> [ snipped since email is getting long ]
>>> diff --git a/kernel/module.c b/kernel/module.c
>>> index b05d466..71c77ed 100644
>>> --- a/kernel/module.c
>>> +++ b/kernel/module.c
>>> @@ -53,6 +53,7 @@
>>>  #include <asm/sections.h>
>>>  #include <linux/tracepoint.h>
>>>  #include <linux/ftrace.h>
>>> +#include <linux/livepatch.h>
>>>  #include <linux/async.h>
>>>  #include <linux/percpu.h>
>>>  #include <linux/kmemleak.h>
>>> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>>>  		mod->exit();
>>>  	blocking_notifier_call_chain(&module_notify_list,
>>>  				     MODULE_STATE_GOING, mod);
>>> +	klp_module_disable(mod);
>>>  	ftrace_release_mod(mod);
>>>
>>>  	async_synchronize_full();
>>> @@ -3297,6 +3299,7 @@ fail:
>>>  	module_put(mod);
>>>  	blocking_notifier_call_chain(&module_notify_list,
>>>  				     MODULE_STATE_GOING, mod);
>>> +	klp_module_disable(mod);
>>>  	ftrace_release_mod(mod);
>>>  	free_module(mod);
>>>  	wake_up_all(&module_wq);
>>> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>>  	mutex_unlock(&module_mutex);
>>>
>>>  	ftrace_module_enable(mod);
>>> +	err = klp_module_enable(mod);
>>> +	if (err)
>>> +		goto out;
>>
>>If you go out here, you need to revert some some operations
>>that are normally done in the bug_cleanup: goto target
>>in load_module(). In particular, you need to do:
>>
>>	/* module_bug_cleanup needs module_mutex protection */
>>	mutex_lock(&module_mutex);
>>	module_bug_cleanup(mod);
>>	mutex_unlock(&module_mutex);
>>
>>	ftrace_release_mod(mod);
>>
>>	/* we can't deallocate the module until we clear memory protection */
>>	module_disable_ro(mod);
>>	module_disable_nx(mod);
>>
>>
>>IMHO, it would make sense to somehow split the complete_formation() function
>>and avoid a code duplication in the error paths.
>
> Argh, thank you for catching that. I think we could split up complete_formation()
> into two functions in order to make the error handling work. 
>
> We could probably take out the coming notifier calls, ftrace_module_enable(),
> and klp_module_enable() out of complete_formation(), and put them in another
> function, maybe called prepare_coming_module(), that would be called right
> after complete_formation(). It might look something like this:
>
> @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
>         err = complete_formation(mod, info); 
>         if (err)
>                 goto ddebug_cleanup;
> +       err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers
> +       if (err) // means that klp_module_enable failed
> +               goto bug_cleanup;
>  
>         /* Module is ready to execute: parsing args may do that. */
>         after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>                                   unknown_module_param_cb);
>         if (IS_ERR(after_dashes)) {
>                 err = PTR_ERR(after_dashes);
> -               goto bug_cleanup;
> +               goto coming_cleanup;
>         } else if (after_dashes) {
>                 pr_warn("%s: parameters '%s' after `--' ignored\n",
>                        mod->name, after_dashes);
>
> Now for the error conditions. If complete_formation() fails, goto
> ddebug_cleanup. If prepare_coming_module() fails (at that point,
> module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
> bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
> ftrace_module_enable, and the coming notifiers have finished) goto
> coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
> so we don't have to call it in coming_module.

Sounds good.

> Also, one last thing, I noticed that module->state isn't
> set to MODULE_STATE_GOING anywhere before the going notifier chain is called in
> the bug_cleanup label (I think it is still COMING at that point), so the
> klp_module_disable call right afterwards would have bailed out because of that.
> To be consistent, shouldn't it be set before the going notifiers are called?

Good spotting.  You could argue that there's a difference between the
notifier argument (what we're doing) and the module state (how far it
got), but we do set 'mod->state = MODULE_STATE_GOING;' in the initcall
fail case, so this should be the same.

Patch welcome :)

Thanks,
Rusty.

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

[Miroslav Benes]

On Mon, 1 Feb 2016, Jessica Yu wrote:

> +/* Called from the module loader during module coming/going states */
> +extern int klp_module_enable(struct module *mod);
> +extern void klp_module_disable(struct module *mod);

We do not use 'extern' keyword in header files. It is redundant. 
Unfortunately, the situation differs among header files and it is hard to 
be consistent.

> +	/*
> +	 * Each module has to know that the coming handler has
> +	 * been called. We never know what module will get
> +	 * patched by a new patch.
> +	 */
> +	mod->klp_alive = true;

This comment should fixed too.

Note: we still need klp_alive, because the race is still there even 
without notifiers.

> +void klp_module_disable(struct module *mod)
>  {
> -	int ret;
> -	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
>  
> -	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> -		return 0;
> +	if (mod->state != MODULE_STATE_GOING)
> +		return;

This is similar to what Petr proposed. We must be in MODULE_STATE_GOING 
here. We could WARN here and return.

> diff --git a/kernel/module.c b/kernel/module.c
> index b05d466..71c77ed 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mutex_unlock(&module_mutex);
>  
>  	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod);
> +	if (err)
> +		goto out;
> +

if (err)
	return err;

module_mutex is already unlocked so no need to jump to out.

Apart from these minor things and Petr's remarks it looks ok.

Thanks for fixing this.
Miroslav

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

[Josh Poimboeuf]

On Mon, Feb 01, 2016 at 08:17:36PM -0500, Jessica Yu wrote:
> Remove the livepatch module notifier in favor of directly enabling and
> disabling patches to modules in the module loader. Hard-coding the
> function calls ensures that ftrace_module_enable() is run before
> klp_module_enable() during module load, and that klp_module_disable() is
> run before ftrace_release_mod() during module unload. This way, ftrace
> and livepatch code is run in the correct order during the module
> load/unload sequence without dependence on the module notifier call chain.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/livepatch.h |   9 +++
>  kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
>  kernel/module.c           |   8 +++
>  3 files changed, 86 insertions(+), 75 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a882865..fdd5f1c 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
>  int klp_disable_patch(struct klp_patch *);
>  
> +/* Called from the module loader during module coming/going states */
> +extern int klp_module_enable(struct module *mod);
> +extern void klp_module_disable(struct module *mod);
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline int klp_module_enable(struct module *mod) { return 0; }
> +static inline void klp_module_disable(struct module *mod) { }
> +
>  #endif /* CONFIG_LIVEPATCH */
>  
>  #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..7aa975d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj)
>  	 */
>  	mod = find_module(obj->name);
>  	/*
> -	 * Do not mess work of the module coming and going notifiers.
> +	 * Do not mess work of the klp module coming and going handlers.
>  	 * Note that the patch might still be needed before the going handler
>  	 * is called. Module functions can be called even in the GOING state
>  	 * until mod->exit() finishes. This is especially important for
> @@ -866,103 +866,107 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static int klp_module_notify_coming(struct klp_patch *patch,
> -				     struct klp_object *obj)
> +/* Called when module state is MODULE_STATE_COMING */
> +int klp_module_enable(struct module *mod)

I think this function name was originally my idea.  But now I'm thinking
it could cause some confusion with the similarly named
klp_enable_object().

How about naming it klp_module_coming()?  That more accurately describes
its purpose IMO and it would also make the comment above it no longer
necessary.

And similarly we could rename klp_module_disable() ->
klp_module_going().


-- 
Josh

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

[Miroslav Benes]

On Thu, 4 Feb 2016, Josh Poimboeuf wrote:

> On Mon, Feb 01, 2016 at 08:17:36PM -0500, Jessica Yu wrote:
> > Remove the livepatch module notifier in favor of directly enabling and
> > disabling patches to modules in the module loader. Hard-coding the
> > function calls ensures that ftrace_module_enable() is run before
> > klp_module_enable() during module load, and that klp_module_disable() is
> > run before ftrace_release_mod() during module unload. This way, ftrace
> > and livepatch code is run in the correct order during the module
> > load/unload sequence without dependence on the module notifier call chain.
> > 
> > This fixes a notifier ordering issue in which the ftrace module notifier
> > (and hence ftrace_module_enable()) for coming modules was being called
> > after klp_module_notify(), which caused livepatch modules to initialize
> > incorrectly.
> > 
> > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > ---
> >  include/linux/livepatch.h |   9 +++
> >  kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
> >  kernel/module.c           |   8 +++
> >  3 files changed, 86 insertions(+), 75 deletions(-)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index a882865..fdd5f1c 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *);
> >  int klp_enable_patch(struct klp_patch *);
> >  int klp_disable_patch(struct klp_patch *);
> >  
> > +/* Called from the module loader during module coming/going states */
> > +extern int klp_module_enable(struct module *mod);
> > +extern void klp_module_disable(struct module *mod);
> > +
> > +#else /* !CONFIG_LIVEPATCH */
> > +
> > +static inline int klp_module_enable(struct module *mod) { return 0; }
> > +static inline void klp_module_disable(struct module *mod) { }
> > +
> >  #endif /* CONFIG_LIVEPATCH */
> >  
> >  #endif /* _LINUX_LIVEPATCH_H_ */
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index bc2c85c..7aa975d 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj)
> >  	 */
> >  	mod = find_module(obj->name);
> >  	/*
> > -	 * Do not mess work of the module coming and going notifiers.
> > +	 * Do not mess work of the klp module coming and going handlers.
> >  	 * Note that the patch might still be needed before the going handler
> >  	 * is called. Module functions can be called even in the GOING state
> >  	 * until mod->exit() finishes. This is especially important for
> > @@ -866,103 +866,107 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > -static int klp_module_notify_coming(struct klp_patch *patch,
> > -				     struct klp_object *obj)
> > +/* Called when module state is MODULE_STATE_COMING */
> > +int klp_module_enable(struct module *mod)
> 
> I think this function name was originally my idea.  But now I'm thinking
> it could cause some confusion with the similarly named
> klp_enable_object().
> 
> How about naming it klp_module_coming()?  That more accurately describes
> its purpose IMO and it would also make the comment above it no longer
> necessary.
> 
> And similarly we could rename klp_module_disable() ->
> klp_module_going().

I agree. klp_module_{coming,going} is better.

Miroslav

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

[Jiri Kosina]

On Mon, 1 Feb 2016, Jessica Yu wrote:

> As explained here [1], livepatch modules are failing to initialize properly
> because the ftrace coming module notifier (which calls
> ftrace_module_enable()) runs *after* the livepatch module notifier (which
> enables the patch(es)). Thus livepatch attempts to apply patches to
> modules before ftrace_module_enable() is even called for the corresponding
> module(s). As a result, patch modules break. Ftrace code must run before
> livepatch on module load, and the reverse is true on module unload.
> 
> For ftrace and livepatch, order of initialization (plus exit/cleanup code)
> is important for loading and unloading modules, and using module notifiers
> to perform this work is not ideal since it is not always clear what gets
> called when. In this patchset, dependence on the module notifier call chain
> is removed in favor of hard coding the corresponding function calls in the
> module loader. This promotes better code visibility and ensures that ftrace
> and livepatch code get called in the correct order on patch module load and
> unload.
> 
> Tested the changes with a test livepatch module that patches 9p and nilfs2,
> and verified that the issue described in [1] is fixed.
> 
> Patches are based on linux-next.
> 
> v1 can be found here -
> http://lkml.kernel.org/g/1454049827-3726-1-git-send-email-jeyu@redhat.com
> 
> v2:
> - Instead of splitting the ftrace and livepatch notifiers into coming + going
>   notifiers and adjusting their priorities, remove ftrace and livepatch notifiers
>   completely and hard-code the necessary function calls in the module loader.
> 
> [1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com
> 
> 
> Jessica Yu (2):
>   ftrace/module: remove ftrace module notifier
>   livepatch/module: remove livepatch module notifier
> 
>  include/linux/ftrace.h    |   6 +-
>  include/linux/livepatch.h |   9 +++
>  kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
>  kernel/module.c           |  12 ++++
>  kernel/trace/ftrace.c     |  36 +-----------

Steven, Rusty, what is your word on this please?

These two patches should be merged together, and I'd like to have the 
module patching issue Jessica discovered fixed for 4.5 still. IOW, if you 
Ack the parts relevant to you (ftrace and module), I'd be willing to take 
it to Linus through my tree.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Steven Rostedt]

On Thu, 4 Feb 2016 11:43:57 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:

>
> Steven, Rusty, what is your word on this please?

I'm fine with the first patch, but you still need an Ack from Rusty on
both. Unfortunately, Rusty is more part-time on kernel development
these days. Hopefully he's still paying attention.

> 
> These two patches should be merged together, and I'd like to have the 
> module patching issue Jessica discovered fixed for 4.5 still. IOW, if you 
> Ack the parts relevant to you (ftrace and module), I'd be willing to take 
> it to Linus through my tree.

If you get Rusty's acks, you can add my:

 Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

on the first patch and take it through your tree.

-- Steve

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

[Rusty Russell]

Steven Rostedt <rostedt@goodmis.org> writes:
>> These two patches should be merged together, and I'd like to have the 
>> module patching issue Jessica discovered fixed for 4.5 still. IOW, if you 
>> Ack the parts relevant to you (ftrace and module), I'd be willing to take 
>> it to Linus through my tree.
>
> If you get Rusty's acks, you can add my:
>
>  Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
>
> on the first patch and take it through your tree.

Agreed.

Acked-by: Rusty Russell <rusty@rustcorp.com.au> (module.c parts)

Cheers,
Rusty.

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

Previous discussion (v4) found here: https://lkml.org/lkml/2016/2/8/1180

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 v2 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>
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..4fafbae 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);
 	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)
+/* Module can be either COMING or GOING */
+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 1981ae0..932fb35 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);
@@ -3314,7 +3317,13 @@ 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;
@@ -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

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

[Petr Mladek]

On Fri 2016-03-11 15:03:48, 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.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> 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/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 780f00c..4fafbae 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -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);

One more thing. We should add here:

	mod->klp_alive = true;

Otherwise, there is a small race window when a new patch will try to
patch the module.

>  	klp_free_object_loaded(obj);
> +	mutex_unlock(&klp_mutex);
> +
> +	return ret;
>  }

With the above fix:

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


Best Regards,
Petr

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

[Josh Poimboeuf]

On Fri, Mar 11, 2016 at 03:03:48PM -0500, Jessica Yu wrote:
> +/* Module can be either COMING or GOING */

IMO this comment doesn't really add anything: the below WARN_ON already
says as much.  Also the location of the comment right above the function
is confusing: someone not familiar with the code might wonder why the
function is called klp_module_going() if the module can be either coming
or going.  So I think the comment can just be removed.

> +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;

-- 
Josh