Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323691 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu <jeyu@redhat.com> - 2016-02-02 02:20 +0100
    Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Petr Mladek <pmladek@suse.com> - 2016-02-04 15:40 +0100
      Re: [PATCH v2 2/2] livepatch/module: remove livepatch module  notifier Steven Rostedt <rostedt@goodmis.org> - 2016-02-04 16:00 +0100
      Re: [PATCH v2 2/2] livepatch/module: remove livepatch module  notifier Miroslav Benes <mbenes@suse.cz> - 2016-02-04 17:50 +0100
      Re: livepatch/module: remove livepatch module notifier Jessica Yu <jeyu@redhat.com> - 2016-02-05 05:20 +0100
        Re: livepatch/module: remove livepatch module notifier Miroslav Benes <mbenes@suse.cz> - 2016-02-05 10:20 +0100
          Re: livepatch/module: remove livepatch module notifier Petr Mladek <pmladek@suse.com> - 2016-02-05 11:10 +0100
        Re: livepatch/module: remove livepatch module notifier Rusty Russell <rusty@rustcorp.com.au> - 2016-02-08 03:00 +0100
    Re: [PATCH v2 2/2] livepatch/module: remove livepatch module  notifier Miroslav Benes <mbenes@suse.cz> - 2016-02-04 18:40 +0100
    Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-04 22:00 +0100
      Re: [PATCH v2 2/2] livepatch/module: remove livepatch module  notifier Miroslav Benes <mbenes@suse.cz> - 2016-02-05 10:00 +0100

#1323691 — [PATCH v2 2/2] livepatch/module: remove livepatch module notifier

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

[toc] | [next] | [standalone]

#1326879

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1327472 — Re: livepatch/module: remove livepatch module notifier

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

[toc] | [prev] | [next] | [standalone]

#1327551 — Re: livepatch/module: remove livepatch module notifier

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

[toc] | [prev] | [next] | [standalone]

#1327589 — Re: livepatch/module: remove livepatch module notifier

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

[toc] | [prev] | [next] | [standalone]

#1328694 — Re: livepatch/module: remove livepatch module notifier

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1327185

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web