[PATCH v2 0/2] livepatch/module: Avoid races between modules and live patches

[PATCH v2 0/2] livepatch/module: Avoid races between modules and live patches

[Petr Mladek]

There is a notifier that handles live patches for coming and going modules.
It takes klp_mutex lock to avoid races with coming and going patches.

Unfortunately, there are some possible races in the current implementation.
The problem is that we do not keep the klp_mutex lock all the time when
the module is being added or removed.

All the problems should get fixed by the two patches.

Some of the problems will be visible only after we add a more complex
consistency model and start supporting semantics changes in patched
functions. But I would like to fix it already now. We will need it
anyway. IMHO, the current solution is more elegant than any temporary
hacks. The patchset with consistency model will be complex enough.
Let's solve some problems even before.


Thanks a lot Josh for pointing out that module_ftrace_init() is called
in MODULE_STALE_UNFORMED. It inspired me for the other solution
of coming modules.


Changes in v2:

+ split fix for coming and going modules
+ call klp_module_init() directly instead of using a handler
+ check if mod is not NULL when checking the module state
+ use the boolean flag only for going handler


Petr Mladek (2):
  livepatch/module: Apply patch when loaded module is unformed
  livepatch/module: Correctly handle going modules

 include/linux/livepatch.h |  10 ++++
 include/linux/module.h    |   4 ++
 kernel/livepatch/core.c   | 124 +++++++++++++++++++++++++++++++++++-----------
 kernel/module.c           |   9 ++++
 4 files changed, 119 insertions(+), 28 deletions(-)

-- 
1.8.5.6

[PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Petr Mladek]

Existing live patches are applied to loaded modules using a notify handler.
There are two problems with this approach.

First, errors from module notifiers are ignored and could not stop the module
from being loaded. But we will need to refuse the module when there are
semantics dependencies between functions and there are some problems
to apply the patch to the module. Otherwise, the system might become
into an inconsistent state.

Second, the module notifiers are called when the module is in
STATE_MODULE_COMING. It means that it is visible by find_module()
and can be detected by klp_find_object_module() when a new patch is
registered.

Now, the timing is important. If the new patch is registered after the module
notifier has been called, it has to initialize the module object for the new
patch. Note that, in this case, the new patch has to see the module as loaded
even when it is still in the COMING state.

But when the new patch is registered before the module notifier, it _should_
not initialize the module object, see below for detailed explanation.

This patch solves both problems by calling klp_module_init() directly in
load_module(). We could handle the error there. Also it is called in
MODULE_STATE_UNFORMED and therefore before the module is visible via
find_module().

The implementation creates three functions for module init and three
functions for going modules. We need to revert already initialized
patches when something fails and thus need to be able to call
the code for going modules without leaving klp_mutex.

Detailed explanation of the last problem:

Why should not we initialize the module object for a new patch when
the related module coming notifier has not been called yet?

Note that the notifier could _not_ _simply_ ignore already initialized module
objects. The notifier initializes the module object for all existing patches.
If the new patch is registered and enabled before, it would crate wrong
order of patches in fops->func_stack.

For example, let's have three patches (P1, P2, P3) for the functions a()
and b() where a() is from vmcore and b() is from a module M. Something
like:

	a()	b()
P1	a1()	b1()
P2	a2()	b2()
P3	a3()	b3(3)

If you load the module M after all patches are registered and enabled.
The ftrace ops for function a() and b() has listed the functions in this
order

	ops_a->func_stack -> list(a3,a2,a1)
	ops_b->func_stack -> list(b3,b2,b1)

, so the pointer to b3() is the first and will be used.

Then you might have the following scenario. Let's start with state
when patches P1 and P2 are registered and enabled but the module M
is not loaded. Then ftrace ops for b() does not exist. Then we
get into the following race:

CPU0					CPU1

load_module(M)

  complete_formation()

  mod->state = MODULE_STATE_COMING;
  mutex_unlock(&module_mutex);

					klp_register_patch(P3);
					klp_enable_patch(P3);

					# STATE 1

  klp_module_notify(M)
    klp_module_notify_coming(P1);
    klp_module_notify_coming(P2);
    klp_module_notify_coming(P3);

					# STATE 2

The ftrace ops for a() and b() then looks:

  STATE1:

	ops_a->func_stack -> list(a3,a2,a1);
	ops_b->func_stack -> list(b3);

  STATE2:
	ops_a->func_stack -> list(a3,a2,a1);
	ops_b->func_stack -> list(b2,b1,b3);

therefore, b2() is used for the module but a3() is used for vmcore
because they were the last added.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 include/linux/livepatch.h | 10 +++++
 kernel/livepatch/core.c   | 94 +++++++++++++++++++++++++++++++++++------------
 kernel/module.c           |  9 +++++
 3 files changed, 89 insertions(+), 24 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index ee6dbb39a809..78ac10546160 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -128,6 +128,16 @@ int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
+int klp_module_init(struct module *mod);
+
+#else /* CONFIG_LIVEPATCH */
+
+inline int klp_module_init(struct module *mod)
+{
+	return 0;
+}
+
 #endif /* CONFIG_LIVEPATCH */
 
+
 #endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 7e0c83dc7215..198f7733604b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -869,8 +869,8 @@ int klp_register_patch(struct klp_patch *patch)
 }
 EXPORT_SYMBOL_GPL(klp_register_patch);
 
-static void klp_module_notify_coming(struct klp_patch *patch,
-				     struct klp_object *obj)
+static int klp_module_coming_update_patch(struct klp_patch *patch,
+					  struct klp_object *obj)
 {
 	struct module *pmod = patch->mod;
 	struct module *mod = obj->mod;
@@ -881,22 +881,62 @@ static void klp_module_notify_coming(struct klp_patch *patch,
 		goto err;
 
 	if (patch->state == KLP_DISABLED)
-		return;
+		return 0;
 
 	pr_notice("applying patch '%s' to loading module '%s'\n",
 		  pmod->name, mod->name);
 
 	ret = klp_enable_object(obj);
 	if (!ret)
-		return;
+		return 0;
 
 err:
 	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
 		pmod->name, mod->name, ret);
+	return ret;
 }
 
-static void klp_module_notify_going(struct klp_patch *patch,
-				    struct klp_object *obj)
+static void klp_module_going(struct module *mod);
+
+int klp_module_coming(struct module *mod)
+{
+	struct klp_patch *patch;
+	struct klp_object *obj;
+	int ret = 0;
+
+	list_for_each_entry(patch, &klp_patches, list) {
+		for (obj = patch->objs; obj->funcs; obj++) {
+			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
+				continue;
+
+			obj->mod = mod;
+			ret = klp_module_coming_update_patch(patch, obj);
+			if (ret)
+				goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	klp_module_going(mod);
+	return ret;
+}
+
+
+int klp_module_init(struct module *mod)
+{
+	int ret = 0;
+
+	mutex_lock(&klp_mutex);
+	ret = klp_module_coming(mod);
+	mutex_unlock(&klp_mutex);
+
+	return ret;
+}
+
+static void klp_module_going_update_patch(struct klp_patch *patch,
+					  struct klp_object *obj)
 {
 	struct module *pmod = patch->mod;
 	struct module *mod = obj->mod;
@@ -913,40 +953,46 @@ disabled:
 	klp_free_object_loaded(obj);
 }
 
-static int klp_module_notify(struct notifier_block *nb, unsigned long action,
-			     void *data)
+static void klp_module_going(struct module *mod)
 {
-	struct module *mod = data;
 	struct klp_patch *patch;
 	struct klp_object *obj;
 
-	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
-		return 0;
-
-	mutex_lock(&klp_mutex);
-
 	list_for_each_entry(patch, &klp_patches, list) {
 		for (obj = patch->objs; obj->funcs; obj++) {
-			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
+			/*
+			 * Handle only loaded (initialized) modules.
+			 * This is needed when used in an error path.
+			 */
+			if (!klp_is_object_loaded(obj) ||
+			    strcmp(obj->name, mod->name))
 				continue;
 
-			if (action == MODULE_STATE_COMING) {
-				obj->mod = mod;
-				klp_module_notify_coming(patch, obj);
-			} else /* MODULE_STATE_GOING */
-				klp_module_notify_going(patch, obj);
-
-			break;
+			klp_module_going_update_patch(patch, obj);
 		}
 	}
 
-	mutex_unlock(&klp_mutex);
+	return;
+}
+
+static int klp_module_notify_going(struct notifier_block *nb,
+				   unsigned long action,
+				   void *data)
+{
+	struct module *mod = data;
+
+	if (action != MODULE_STATE_GOING)
+		return 0;
+
+	mutex_lock(&klp_mutex);
+	klp_module_going(mod);
+	mutex_lock(&klp_mutex);
 
 	return 0;
 }
 
 static struct notifier_block klp_module_nb = {
-	.notifier_call = klp_module_notify,
+	.notifier_call = klp_module_notify_going,
 	.priority = INT_MIN+1, /* called late but before ftrace notifier */
 };
 
diff --git a/kernel/module.c b/kernel/module.c
index d856e96a3cce..f744a639460d 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>
@@ -3321,6 +3322,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
 	ftrace_module_init(mod);
 
+	/*
+	 * LivePatch init must be called in the MODULE_STATE_UNFORMED state
+	 * and it might reject the module to avoid a system inconsistency.
+	 */
+	err = klp_module_init(mod);
+	if (err)
+		goto ddebug_cleanup;
+
 	/* Finally it's fully formed, ready to start executing. */
 	err = complete_formation(mod, info);
 	if (err)
-- 
1.8.5.6

Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Josh Poimboeuf]

On Thu, Mar 05, 2015 at 04:45:13PM +0100, Petr Mladek wrote:
> Existing live patches are applied to loaded modules using a notify handler.
> There are two problems with this approach.
> 
> First, errors from module notifiers are ignored and could not stop the module
> from being loaded. But we will need to refuse the module when there are
> semantics dependencies between functions and there are some problems
> to apply the patch to the module. Otherwise, the system might become
> into an inconsistent state.
> 
> Second, the module notifiers are called when the module is in
> STATE_MODULE_COMING. It means that it is visible by find_module()
> and can be detected by klp_find_object_module() when a new patch is
> registered.
> 
> Now, the timing is important. If the new patch is registered after the module
> notifier has been called, it has to initialize the module object for the new
> patch. Note that, in this case, the new patch has to see the module as loaded
> even when it is still in the COMING state.
> 
> But when the new patch is registered before the module notifier, it _should_
> not initialize the module object, see below for detailed explanation.
> 
> This patch solves both problems by calling klp_module_init() directly in
> load_module(). We could handle the error there. Also it is called in
> MODULE_STATE_UNFORMED and therefore before the module is visible via
> find_module().
> 
> The implementation creates three functions for module init and three
> functions for going modules. We need to revert already initialized
> patches when something fails and thus need to be able to call
> the code for going modules without leaving klp_mutex.
> 
> Detailed explanation of the last problem:
> 
> Why should not we initialize the module object for a new patch when
> the related module coming notifier has not been called yet?
> 
> Note that the notifier could _not_ _simply_ ignore already initialized module
> objects. The notifier initializes the module object for all existing patches.
> If the new patch is registered and enabled before, it would crate wrong
> order of patches in fops->func_stack.
> 
> For example, let's have three patches (P1, P2, P3) for the functions a()
> and b() where a() is from vmcore and b() is from a module M. Something
> like:
> 
> 	a()	b()
> P1	a1()	b1()
> P2	a2()	b2()
> P3	a3()	b3(3)
> 
> If you load the module M after all patches are registered and enabled.
> The ftrace ops for function a() and b() has listed the functions in this
> order
> 
> 	ops_a->func_stack -> list(a3,a2,a1)
> 	ops_b->func_stack -> list(b3,b2,b1)
> 
> , so the pointer to b3() is the first and will be used.
> 
> Then you might have the following scenario. Let's start with state
> when patches P1 and P2 are registered and enabled but the module M
> is not loaded. Then ftrace ops for b() does not exist. Then we
> get into the following race:
> 
> CPU0					CPU1
> 
> load_module(M)
> 
>   complete_formation()
> 
>   mod->state = MODULE_STATE_COMING;
>   mutex_unlock(&module_mutex);
> 
> 					klp_register_patch(P3);
> 					klp_enable_patch(P3);
> 
> 					# STATE 1
> 
>   klp_module_notify(M)
>     klp_module_notify_coming(P1);
>     klp_module_notify_coming(P2);
>     klp_module_notify_coming(P3);
> 
> 					# STATE 2
> 
> The ftrace ops for a() and b() then looks:
> 
>   STATE1:
> 
> 	ops_a->func_stack -> list(a3,a2,a1);
> 	ops_b->func_stack -> list(b3);
> 
>   STATE2:
> 	ops_a->func_stack -> list(a3,a2,a1);
> 	ops_b->func_stack -> list(b2,b1,b3);
> 
> therefore, b2() is used for the module but a3() is used for vmcore
> because they were the last added.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  include/linux/livepatch.h | 10 +++++
>  kernel/livepatch/core.c   | 94 +++++++++++++++++++++++++++++++++++------------
>  kernel/module.c           |  9 +++++
>  3 files changed, 89 insertions(+), 24 deletions(-)

Not sure why, but "git am" seemed to think this patch was malformed.  It
applied ok for me after I removed the diffstat.

> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index ee6dbb39a809..78ac10546160 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -128,6 +128,16 @@ int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
>  int klp_disable_patch(struct klp_patch *);
>  
> +int klp_module_init(struct module *mod);
> +
> +#else /* CONFIG_LIVEPATCH */
> +
> +inline int klp_module_init(struct module *mod)

Should it not be "static inline"?

/me prays not to have to break out the C spec again ;-)

> +{
> +	return 0;
> +}
> +
>  #endif /* CONFIG_LIVEPATCH */
>  
> +
>  #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 7e0c83dc7215..198f7733604b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -869,8 +869,8 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static void klp_module_notify_coming(struct klp_patch *patch,
> -				     struct klp_object *obj)
> +static int klp_module_coming_update_patch(struct klp_patch *patch,
> +					  struct klp_object *obj)

This function name confused me a little bit.  Not sure what would be
better, but it really updates the object, not the patch.  Maybe
klp_module_coming_object()?

>  {
>  	struct module *pmod = patch->mod;
>  	struct module *mod = obj->mod;
> @@ -881,22 +881,62 @@ static void klp_module_notify_coming(struct klp_patch *patch,
>  		goto err;
>  
>  	if (patch->state == KLP_DISABLED)
> -		return;
> +		return 0;
>  
>  	pr_notice("applying patch '%s' to loading module '%s'\n",
>  		  pmod->name, mod->name);
>  
>  	ret = klp_enable_object(obj);
>  	if (!ret)
> -		return;
> +		return 0;
>  
>  err:
>  	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
>  		pmod->name, mod->name, ret);

Does it still make sense to have this pr_warn() here now that we can
return an error and stop the module from loading?

I'm thinking it should be changed to pr_err() to be consistent with the
other klp error printks, and should probably say that we're preventing
the module from loading.

> +	return ret;
>  }
>  
> -static void klp_module_notify_going(struct klp_patch *patch,
> -				    struct klp_object *obj)
> +static void klp_module_going(struct module *mod);

It would probably be better to move klp_module_going() here so you don't
have to forward declare it.

> +
> +int klp_module_coming(struct module *mod)
> +{
> +	struct klp_patch *patch;
> +	struct klp_object *obj;
> +	int ret = 0;
> +
> +	list_for_each_entry(patch, &klp_patches, list) {
> +		for (obj = patch->objs; obj->funcs; obj++) {
> +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +				continue;
> +
> +			obj->mod = mod;
> +			ret = klp_module_coming_update_patch(patch, obj);
> +			if (ret)
> +				goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	klp_module_going(mod);
> +	return ret;
> +}
> +
> +
> +int klp_module_init(struct module *mod)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&klp_mutex);
> +	ret = klp_module_coming(mod);
> +	mutex_unlock(&klp_mutex);
> +
> +	return ret;
> +}
> +
> +static void klp_module_going_update_patch(struct klp_patch *patch,
> +					  struct klp_object *obj)
>  {
>  	struct module *pmod = patch->mod;
>  	struct module *mod = obj->mod;
> @@ -913,40 +953,46 @@ disabled:
>  	klp_free_object_loaded(obj);
>  }
>  
> -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> -			     void *data)
> +static void klp_module_going(struct module *mod)
>  {
> -	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
>  
> -	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> -		return 0;
> -
> -	mutex_lock(&klp_mutex);
> -
>  	list_for_each_entry(patch, &klp_patches, list) {
>  		for (obj = patch->objs; obj->funcs; obj++) {
> -			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +			/*
> +			 * Handle only loaded (initialized) modules.
> +			 * This is needed when used in an error path.
> +			 */
> +			if (!klp_is_object_loaded(obj) ||
> +			    strcmp(obj->name, mod->name))

Also need a klp_is_module() check here so it doesn't send NULL to strcmp
in the case of vmlinux.


>  				continue;
>  
> -			if (action == MODULE_STATE_COMING) {
> -				obj->mod = mod;
> -				klp_module_notify_coming(patch, obj);
> -			} else /* MODULE_STATE_GOING */
> -				klp_module_notify_going(patch, obj);
> -
> -			break;
> +			klp_module_going_update_patch(patch, obj);
>  		}
>  	}
>  
> -	mutex_unlock(&klp_mutex);
> +	return;

Redundant return.

> +}
> +
> +static int klp_module_notify_going(struct notifier_block *nb,
> +				   unsigned long action,
> +				   void *data)
> +{
> +	struct module *mod = data;
> +
> +	if (action != MODULE_STATE_GOING)
> +		return 0;
> +
> +	mutex_lock(&klp_mutex);
> +	klp_module_going(mod);
> +	mutex_lock(&klp_mutex);
>  
>  	return 0;
>  }
>  
>  static struct notifier_block klp_module_nb = {
> -	.notifier_call = klp_module_notify,
> +	.notifier_call = klp_module_notify_going,
>  	.priority = INT_MIN+1, /* called late but before ftrace notifier */
>  };
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index d856e96a3cce..f744a639460d 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>
> @@ -3321,6 +3322,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
>  	ftrace_module_init(mod);
>  
> +	/*
> +	 * LivePatch init must be called in the MODULE_STATE_UNFORMED state
> +	 * and it might reject the module to avoid a system inconsistency.
> +	 */

nit: I thought we were calling it livepatch (all lowercase).

> +	err = klp_module_init(mod);
> +	if (err)
> +		goto ddebug_cleanup;
> +
>  	/* Finally it's fully formed, ready to start executing. */
>  	err = complete_formation(mod, info);
>  	if (err)

Hm, we still have a problem with the timing here.  The kallsyms lookup
functions ignore MODULE_STATE_UNFORMED modules.  So
klp_find_verify_func_addr() will fail to find the func address and the
module will always fail to load.

-- 
Josh

Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Petr Mladek]

On Thu 2015-03-05 13:34:33, Josh Poimboeuf wrote:
> On Thu, Mar 05, 2015 at 04:45:13PM +0100, Petr Mladek wrote:
> > Existing live patches are applied to loaded modules using a notify handler.
> > There are two problems with this approach.
> > 
> > First, errors from module notifiers are ignored and could not stop the module
> > from being loaded. But we will need to refuse the module when there are
> > semantics dependencies between functions and there are some problems
> > to apply the patch to the module. Otherwise, the system might become
> > into an inconsistent state.
> > 
> > Second, the module notifiers are called when the module is in
> > STATE_MODULE_COMING. It means that it is visible by find_module()
> > and can be detected by klp_find_object_module() when a new patch is
> > registered.
> > 
> > Now, the timing is important. If the new patch is registered after the module
> > notifier has been called, it has to initialize the module object for the new
> > patch. Note that, in this case, the new patch has to see the module as loaded
> > even when it is still in the COMING state.
> > 
> > But when the new patch is registered before the module notifier, it _should_
> > not initialize the module object, see below for detailed explanation.
> > 
> > This patch solves both problems by calling klp_module_init() directly in
> > load_module(). We could handle the error there. Also it is called in
> > MODULE_STATE_UNFORMED and therefore before the module is visible via
> > find_module().
> > 
> > The implementation creates three functions for module init and three
> > functions for going modules. We need to revert already initialized
> > patches when something fails and thus need to be able to call
> > the code for going modules without leaving klp_mutex.
> > 
> > Detailed explanation of the last problem:
> > 
> > Why should not we initialize the module object for a new patch when
> > the related module coming notifier has not been called yet?
> > 
> > Note that the notifier could _not_ _simply_ ignore already initialized module
> > objects. The notifier initializes the module object for all existing patches.
> > If the new patch is registered and enabled before, it would crate wrong
> > order of patches in fops->func_stack.
> > 
> > For example, let's have three patches (P1, P2, P3) for the functions a()
> > and b() where a() is from vmcore and b() is from a module M. Something
> > like:
> > 
> > 	a()	b()
> > P1	a1()	b1()
> > P2	a2()	b2()
> > P3	a3()	b3(3)
> > 
> > If you load the module M after all patches are registered and enabled.
> > The ftrace ops for function a() and b() has listed the functions in this
> > order
> > 
> > 	ops_a->func_stack -> list(a3,a2,a1)
> > 	ops_b->func_stack -> list(b3,b2,b1)
> > 
> > , so the pointer to b3() is the first and will be used.
> > 
> > Then you might have the following scenario. Let's start with state
> > when patches P1 and P2 are registered and enabled but the module M
> > is not loaded. Then ftrace ops for b() does not exist. Then we
> > get into the following race:
> > 
> > CPU0					CPU1
> > 
> > load_module(M)
> > 
> >   complete_formation()
> > 
> >   mod->state = MODULE_STATE_COMING;
> >   mutex_unlock(&module_mutex);
> > 
> > 					klp_register_patch(P3);
> > 					klp_enable_patch(P3);
> > 
> > 					# STATE 1
> > 
> >   klp_module_notify(M)
> >     klp_module_notify_coming(P1);
> >     klp_module_notify_coming(P2);
> >     klp_module_notify_coming(P3);
> > 
> > 					# STATE 2
> > 
> > The ftrace ops for a() and b() then looks:
> > 
> >   STATE1:
> > 
> > 	ops_a->func_stack -> list(a3,a2,a1);
> > 	ops_b->func_stack -> list(b3);
> > 
> >   STATE2:
> > 	ops_a->func_stack -> list(a3,a2,a1);
> > 	ops_b->func_stack -> list(b2,b1,b3);
> > 
> > therefore, b2() is used for the module but a3() is used for vmcore
> > because they were the last added.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > ---
> >  include/linux/livepatch.h | 10 +++++
> >  kernel/livepatch/core.c   | 94 +++++++++++++++++++++++++++++++++++------------
> >  kernel/module.c           |  9 +++++
> >  3 files changed, 89 insertions(+), 24 deletions(-)
> 
> Not sure why, but "git am" seemed to think this patch was malformed.  It
> applied ok for me after I removed the diffstat.

Which branch have you tried it against, please? I have made them on
top of "for-next" branch in the git/jikos/livepatching.git tree.
The patch seems to apply fine there.

> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index ee6dbb39a809..78ac10546160 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -128,6 +128,16 @@ int klp_unregister_patch(struct klp_patch *);
> >  int klp_enable_patch(struct klp_patch *);
> >  int klp_disable_patch(struct klp_patch *);
> >  
> > +int klp_module_init(struct module *mod);
> > +
> > +#else /* CONFIG_LIVEPATCH */
> > +
> > +inline int klp_module_init(struct module *mod)
> 
> Should it not be "static inline"?

Yup, will fix it.
 
> /me prays not to have to break out the C spec again ;-)
> 
> > +{
> > +	return 0;
> > +}
> > +
> >  #endif /* CONFIG_LIVEPATCH */
> >  
> > +
> >  #endif /* _LINUX_LIVEPATCH_H_ */
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 7e0c83dc7215..198f7733604b 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -869,8 +869,8 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > -static void klp_module_notify_coming(struct klp_patch *patch,
> > -				     struct klp_object *obj)
> > +static int klp_module_coming_update_patch(struct klp_patch *patch,
> > +					  struct klp_object *obj)
> 
> This function name confused me a little bit.  Not sure what would be
> better, but it really updates the object, not the patch.  Maybe
> klp_module_coming_object()?

Yup, I was in doubt as well. The function is called only for one
object from a given patch. So, both "patch" and "object" seems
appropriate.

klp_module_coming_object() is confusing as well because it looks
like coming object but the object struct is from a patch that was
there before.

I could change this to klp_module_coming_update_object() if it
sounds better to you. Any beeter ideas are welcome.

> >  {
> >  	struct module *pmod = patch->mod;
> >  	struct module *mod = obj->mod;
> > @@ -881,22 +881,62 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> >  		goto err;
> >  
> >  	if (patch->state == KLP_DISABLED)
> > -		return;
> > +		return 0;
> >  
> >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> >  		  pmod->name, mod->name);
> >  
> >  	ret = klp_enable_object(obj);
> >  	if (!ret)
> > -		return;
> > +		return 0;
> >  
> >  err:
> >  	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> >  		pmod->name, mod->name, ret);
> 
> Does it still make sense to have this pr_warn() here now that we can
> return an error and stop the module from loading?
> 
> I'm thinking it should be changed to pr_err() to be consistent with the
> other klp error printks, and should probably say that we're preventing
> the module from loading.

Good catch. I'll change this to pr_err().

> > +	return ret;
> >  }
> >  
> > -static void klp_module_notify_going(struct klp_patch *patch,
> > -				    struct klp_object *obj)
> > +static void klp_module_going(struct module *mod);
> 
> It would probably be better to move klp_module_going() here so you don't
> have to forward declare it.

I did it but then the diff became a mess and was hard to read. OK,
I'll add one more patch before this one that will just switch the
order and do this changes on top of it.

> > +
> > +int klp_module_coming(struct module *mod)
> > +{
> > +	struct klp_patch *patch;
> > +	struct klp_object *obj;
> > +	int ret = 0;
> > +
> > +	list_for_each_entry(patch, &klp_patches, list) {
> > +		for (obj = patch->objs; obj->funcs; obj++) {
> > +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> > +				continue;
> > +
> > +			obj->mod = mod;
> > +			ret = klp_module_coming_update_patch(patch, obj);
> > +			if (ret)
> > +				goto err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +err:
> > +	klp_module_going(mod);
> > +	return ret;
> > +}
> > +
> > +
> > +int klp_module_init(struct module *mod)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&klp_mutex);
> > +	ret = klp_module_coming(mod);
> > +	mutex_unlock(&klp_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static void klp_module_going_update_patch(struct klp_patch *patch,
> > +					  struct klp_object *obj)
> >  {
> >  	struct module *pmod = patch->mod;
> >  	struct module *mod = obj->mod;
> > @@ -913,40 +953,46 @@ disabled:
> >  	klp_free_object_loaded(obj);
> >  }
> >  
> > -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > -			     void *data)
> > +static void klp_module_going(struct module *mod)
> >  {
> > -	struct module *mod = data;
> >  	struct klp_patch *patch;
> >  	struct klp_object *obj;
> >  
> > -	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> > -		return 0;
> > -
> > -	mutex_lock(&klp_mutex);
> > -
> >  	list_for_each_entry(patch, &klp_patches, list) {
> >  		for (obj = patch->objs; obj->funcs; obj++) {
> > -			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> > +			/*
> > +			 * Handle only loaded (initialized) modules.
> > +			 * This is needed when used in an error path.
> > +			 */
> > +			if (!klp_is_object_loaded(obj) ||
> > +			    strcmp(obj->name, mod->name))
> 
> Also need a klp_is_module() check here so it doesn't send NULL to strcmp
> in the case of vmlinux.

ah, yes, great catch

> 
> >  				continue;
> >  
> > -			if (action == MODULE_STATE_COMING) {
> > -				obj->mod = mod;
> > -				klp_module_notify_coming(patch, obj);
> > -			} else /* MODULE_STATE_GOING */
> > -				klp_module_notify_going(patch, obj);
> > -
> > -			break;
> > +			klp_module_going_update_patch(patch, obj);
> >  		}
> >  	}
> >  
> > -	mutex_unlock(&klp_mutex);
> > +	return;
> 
> Redundant return.

Will fix

> > +}
> > +
> > +static int klp_module_notify_going(struct notifier_block *nb,
> > +				   unsigned long action,
> > +				   void *data)
> > +{
> > +	struct module *mod = data;
> > +
> > +	if (action != MODULE_STATE_GOING)
> > +		return 0;
> > +
> > +	mutex_lock(&klp_mutex);
> > +	klp_module_going(mod);
> > +	mutex_lock(&klp_mutex);
> >  
> >  	return 0;
> >  }
> >  
> >  static struct notifier_block klp_module_nb = {
> > -	.notifier_call = klp_module_notify,
> > +	.notifier_call = klp_module_notify_going,
> >  	.priority = INT_MIN+1, /* called late but before ftrace notifier */
> >  };
> >  
> > diff --git a/kernel/module.c b/kernel/module.c
> > index d856e96a3cce..f744a639460d 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>
> > @@ -3321,6 +3322,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
> >  	ftrace_module_init(mod);
> >  
> > +	/*
> > +	 * LivePatch init must be called in the MODULE_STATE_UNFORMED state
> > +	 * and it might reject the module to avoid a system inconsistency.
> > +	 */
> 
> nit: I thought we were calling it livepatch (all lowercase).

will fix

> > +	err = klp_module_init(mod);
> > +	if (err)
> > +		goto ddebug_cleanup;
> > +
> >  	/* Finally it's fully formed, ready to start executing. */
> >  	err = complete_formation(mod, info);
> >  	if (err)
> 
> Hm, we still have a problem with the timing here.  The kallsyms lookup
> functions ignore MODULE_STATE_UNFORMED modules.  So
> klp_find_verify_func_addr() will fail to find the func address and the
> module will always fail to load.

Hrmmpfffff, my head was relaxing somewhere in the corner when I tested
the patch. You are right, it does not work. Huh, I wonder why we are
able to find the address in kGraft. We are using this approach there
for a long time.

I am going to investigate.

Thanks a lot for review.

Best Regards,
Petr

> -- 
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Petr Mladek]

On Fri 2015-03-06 11:20:32, Petr Mladek wrote:
> On Thu 2015-03-05 13:34:33, Josh Poimboeuf wrote:
> > On Thu, Mar 05, 2015 at 04:45:13PM +0100, Petr Mladek wrote:
> > > Existing live patches are applied to loaded modules using a notify handler.
> > > There are two problems with this approach.
> > > 
> > > First, errors from module notifiers are ignored and could not stop the module
> > > from being loaded. But we will need to refuse the module when there are
> > > semantics dependencies between functions and there are some problems
> > > to apply the patch to the module. Otherwise, the system might become
> > > into an inconsistent state.
> > > 
> > > Second, the module notifiers are called when the module is in
> > > STATE_MODULE_COMING. It means that it is visible by find_module()
> > > and can be detected by klp_find_object_module() when a new patch is
> > > registered.
> > > 
> > > Now, the timing is important. If the new patch is registered after the module
> > > notifier has been called, it has to initialize the module object for the new
> > > patch. Note that, in this case, the new patch has to see the module as loaded
> > > even when it is still in the COMING state.
> > > 
> > > But when the new patch is registered before the module notifier, it _should_
> > > not initialize the module object, see below for detailed explanation.
> > > 
> > > This patch solves both problems by calling klp_module_init() directly in
> > > load_module(). We could handle the error there. Also it is called in
> > > MODULE_STATE_UNFORMED and therefore before the module is visible via
> > > find_module().
> > > 
> > > The implementation creates three functions for module init and three
> > > functions for going modules. We need to revert already initialized
> > > patches when something fails and thus need to be able to call
> > > the code for going modules without leaving klp_mutex.
> > > 
> > > Detailed explanation of the last problem:
> > > 
> > > Why should not we initialize the module object for a new patch when
> > > the related module coming notifier has not been called yet?
> > > 
> > > Note that the notifier could _not_ _simply_ ignore already initialized module
> > > objects. The notifier initializes the module object for all existing patches.
> > > If the new patch is registered and enabled before, it would crate wrong
> > > order of patches in fops->func_stack.
> > > 
> > > For example, let's have three patches (P1, P2, P3) for the functions a()
> > > and b() where a() is from vmcore and b() is from a module M. Something
> > > like:
> > > 
> > > 	a()	b()
> > > P1	a1()	b1()
> > > P2	a2()	b2()
> > > P3	a3()	b3(3)
> > > 
> > > If you load the module M after all patches are registered and enabled.
> > > The ftrace ops for function a() and b() has listed the functions in this
> > > order
> > > 
> > > 	ops_a->func_stack -> list(a3,a2,a1)
> > > 	ops_b->func_stack -> list(b3,b2,b1)
> > > 
> > > , so the pointer to b3() is the first and will be used.
> > > 
> > > Then you might have the following scenario. Let's start with state
> > > when patches P1 and P2 are registered and enabled but the module M
> > > is not loaded. Then ftrace ops for b() does not exist. Then we
> > > get into the following race:
> > > 
> > > CPU0					CPU1
> > > 
> > > load_module(M)
> > > 
> > >   complete_formation()
> > > 
> > >   mod->state = MODULE_STATE_COMING;
> > >   mutex_unlock(&module_mutex);
> > > 
> > > 					klp_register_patch(P3);
> > > 					klp_enable_patch(P3);
> > > 
> > > 					# STATE 1
> > > 
> > >   klp_module_notify(M)
> > >     klp_module_notify_coming(P1);
> > >     klp_module_notify_coming(P2);
> > >     klp_module_notify_coming(P3);
> > > 
> > > 					# STATE 2
> > > 
> > > The ftrace ops for a() and b() then looks:
> > > 
> > >   STATE1:
> > > 
> > > 	ops_a->func_stack -> list(a3,a2,a1);
> > > 	ops_b->func_stack -> list(b3);
> > > 
> > >   STATE2:
> > > 	ops_a->func_stack -> list(a3,a2,a1);
> > > 	ops_b->func_stack -> list(b2,b1,b3);
> > > 
> > > therefore, b2() is used for the module but a3() is used for vmcore
> > > because they were the last added.
> > > 
> > > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > > ---
[...]

> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index d856e96a3cce..f744a639460d 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>
> > > @@ -3321,6 +3322,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > >  	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
> > >  	ftrace_module_init(mod);
> > >  
> > > +	/*
> > > +	 * LivePatch init must be called in the MODULE_STATE_UNFORMED state
> > > +	 * and it might reject the module to avoid a system inconsistency.
> > > +	 */
> > 
> > nit: I thought we were calling it livepatch (all lowercase).
> 
> will fix
> 
> > > +	err = klp_module_init(mod);
> > > +	if (err)
> > > +		goto ddebug_cleanup;
> > > +
> > >  	/* Finally it's fully formed, ready to start executing. */
> > >  	err = complete_formation(mod, info);
> > >  	if (err)
> > 
> > Hm, we still have a problem with the timing here.  The kallsyms lookup
> > functions ignore MODULE_STATE_UNFORMED modules.  So
> > klp_find_verify_func_addr() will fail to find the func address and the
> > module will always fail to load.
> 
> Hrmmpfffff, my head was relaxing somewhere in the corner when I tested
> the patch. You are right, it does not work. Huh, I wonder why we are
> able to find the address in kGraft. We are using this approach there
> for a long time.

Sigh, kGraft calls kgr_module_init() after complete_formation() and
thus in MODULE_STATE_COMING. I should have refreshed my mind. There is
even a comment about this that I have written many months ago.

This brings me back to the original idea with that boolean that
marks the state before and after the coming notifier (module_init).
We could use a bitfield instead of the two booleans when requested.


Alternative solutions:

+ reject new patches when a module is coming; this is ugly

+ wait with adding new patch until the module leaves the COMING
  state; this might be dangerous or complicated; we would need
  to leave kgr_lock in the middle of the patch registration to
  avoid a deadlock with klp_module_init(); also we might need
  a waitqueue for each module which seems to be even bigger
  overhead than the two booleans

+ always register/enable new patches and fix up the potential
  mess (registered patches order) in klp_module_init(); This
  is nasty and prone to regressions in the future development;

+ add another MODULE_STATE where the kallsyms are visible
  but the module is not used yet; this looks to complex;
  the module states are checked on "many" locations


I will wait with v3 over the weekend. I hope that it will bring fresh
mind. Sigh, if I could have slept more with the baby twins.

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Josh Poimboeuf]

On Fri, Mar 06, 2015 at 03:00:13PM +0100, Petr Mladek wrote:
> This brings me back to the original idea with that boolean that
> marks the state before and after the coming notifier (module_init).
> We could use a bitfield instead of the two booleans when requested.

Yeah, that would work.  Though I think one boolean is enough?

For example, just have a mod->klp_live which is initialized to false,
with its value set in the COMING notifier, and cleared in the GOING
notifier.

> Alternative solutions:
> 
> + reject new patches when a module is coming; this is ugly
> 
> + wait with adding new patch until the module leaves the COMING state;
> this might be dangerous or complicated; we would need to leave
> kgr_lock in the middle of the patch registration to avoid a deadlock
> with klp_module_init(); also we might need a waitqueue for each module
> which seems to be even bigger overhead than the two booleans
> 
> + always register/enable new patches and fix up the potential mess
> (registered patches order) in klp_module_init(); This is nasty and
> prone to regressions in the future development;
> 
> + add another MODULE_STATE where the kallsyms are visible but the
> module is not used yet; this looks to complex; the module states are
> checked on "many" locations
> 
> 
> I will wait with v3 over the weekend. I hope that it will bring fresh
> mind. Sigh, if I could have slept more with the baby twins.

Good luck!  In my experience, an entire weekend with babies is far from
restful ;-)

-- 
Josh

Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Petr Mladek]

On Fri 2015-03-06 08:54:23, Josh Poimboeuf wrote:
> On Fri, Mar 06, 2015 at 03:00:13PM +0100, Petr Mladek wrote:
> > This brings me back to the original idea with that boolean that
> > marks the state before and after the coming notifier (module_init).
> > We could use a bitfield instead of the two booleans when requested.
> 
> Yeah, that would work.  Though I think one boolean is enough?
> 
> For example, just have a mod->klp_live which is initialized to false,
> with its value set in the COMING notifier, and cleared in the GOING
> notifier.

Great optimization. I will use it.

> > Alternative solutions:
> > 
> > + reject new patches when a module is coming; this is ugly
> > 
> > + wait with adding new patch until the module leaves the COMING state;
> > this might be dangerous or complicated; we would need to leave
> > kgr_lock in the middle of the patch registration to avoid a deadlock
> > with klp_module_init(); also we might need a waitqueue for each module
> > which seems to be even bigger overhead than the two booleans
> > 
> > + always register/enable new patches and fix up the potential mess
> > (registered patches order) in klp_module_init(); This is nasty and
> > prone to regressions in the future development;
> > 
> > + add another MODULE_STATE where the kallsyms are visible but the
> > module is not used yet; this looks to complex; the module states are
> > checked on "many" locations
> > 
> > 
> > I will wait with v3 over the weekend. I hope that it will bring fresh
> > mind. Sigh, if I could have slept more with the baby twins.
> 
> Good luck!  In my experience, an entire weekend with babies is far from
> restful ;-)

Yeah, you are right :-)

Thanks for review and hint.

Best Regards,
Petr

[PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Petr Mladek]

Existing live patches are removed from going modules using a notify handler.
There are two problems with the current implementation.

First, new patch could still see the module in the GOING state even after
the notifier has been called. It will try to initialize the related
object structures but the module could disappear at any time. There will
stay mess in the structures. It might even cause an invalid memory access.

Second, if we start supporting patches with semantic changes between function
calls, we would need to apply any new patch even for going modules. Note that
the code from the module could be called even in the GOING state until
mod->exit() finishes. See below for example.

This patch solves the problem by adding boolean flag into struct module.
It is switched when the going module handler is called. It marks the point
when it is safe and we actually have to ignore the going module.

Alternative solutions:

We could add another lock to make the switch to GOING state and mod->exit()
call an atomic operation. But this a nogo. It might cause a dead lock when
some mod->exit() depends on mod->exit() from another module.

We could wait until the GOING module is moved to the UNFORMED state.
But this might take ages when mod->exit() has to wait for something.

We could refuse to load the patch when a module is going but this is
pretty ugly.

Example of the race:

CPU0					CPU1

delete_module()  #SYSCALL

   try_stop_module()
     mod->state = MODULE_STATE_GOING;

   mutex_unlock(&module_mutex);

					klp_register_patch()
					klp_enable_patch()

					#save place to switch universe

					b()     # from module that is going
					  a()   # from core (patched)

   mod->exit();

Note that the function b() can be called until we call mod->exit().

If we do not apply patch against b() because it is in MODULE_STATE_GOING.
It will call patched a() with modified semantic and things might get wrong.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 include/linux/module.h  |  4 ++++
 kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index b653d7c0a05a..c12f93497b74 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -344,6 +344,10 @@ struct module {
 	unsigned long *ftrace_callsites;
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+	bool klp_gone;
+#endif
+
 #ifdef CONFIG_MODULE_UNLOAD
 	/* What modules depend on me? */
 	struct list_head source_list;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 198f7733604b..0b38357fad0f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -89,16 +89,32 @@ static bool klp_is_object_loaded(struct klp_object *obj)
 /* sets obj->mod if object is not vmlinux and module is found */
 static void klp_find_object_module(struct klp_object *obj)
 {
+	struct module *mod;
+
 	if (!klp_is_module(obj))
 		return;
 
 	mutex_lock(&module_mutex);
+
+	/*
+	 * We do not want to block removal of patched modules and therefore
+	 * we do not take a reference here. Instead, the patches are removed
+	 * by the going module handler instead.
+	 */
+	mod = find_module(obj->name);
+
 	/*
-	 * We don't need to take a reference on the module here because we have
-	 * the klp_mutex, which is also taken by the module notifier.  This
-	 * prevents any module from unloading until we release the klp_mutex.
+	 * We must not init the object when the module is going and the notifier
+	 * has already been called. But the patch might still be needed before.
+	 * Note that 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.
 	 */
-	obj->mod = find_module(obj->name);
+	if (mod && mod->state == MODULE_STATE_GOING && mod->klp_gone)
+		mod = NULL;
+
+	obj->mod = mod;
+
 	mutex_unlock(&module_mutex);
 }
 
@@ -929,7 +945,10 @@ int klp_module_init(struct module *mod)
 	int ret = 0;
 
 	mutex_lock(&klp_mutex);
+
+	mod->klp_gone = false;
 	ret = klp_module_coming(mod);
+
 	mutex_unlock(&klp_mutex);
 
 	return ret;
@@ -985,7 +1004,10 @@ static int klp_module_notify_going(struct notifier_block *nb,
 		return 0;
 
 	mutex_lock(&klp_mutex);
+
 	klp_module_going(mod);
+	mod->klp_gone = true;
+
 	mutex_lock(&klp_mutex);
 
 	return 0;
-- 
1.8.5.6

Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Josh Poimboeuf]

On Thu, Mar 05, 2015 at 04:45:14PM +0100, Petr Mladek wrote:
> Existing live patches are removed from going modules using a notify handler.
> There are two problems with the current implementation.
> 
> First, new patch could still see the module in the GOING state even after
> the notifier has been called. It will try to initialize the related
> object structures but the module could disappear at any time. There will
> stay mess in the structures. It might even cause an invalid memory access.
> 
> Second, if we start supporting patches with semantic changes between function
> calls, we would need to apply any new patch even for going modules. Note that
> the code from the module could be called even in the GOING state until
> mod->exit() finishes. See below for example.
> 
> This patch solves the problem by adding boolean flag into struct module.
> It is switched when the going module handler is called. It marks the point
> when it is safe and we actually have to ignore the going module.
> 
> Alternative solutions:
> 
> We could add another lock to make the switch to GOING state and mod->exit()
> call an atomic operation. But this a nogo. It might cause a dead lock when
> some mod->exit() depends on mod->exit() from another module.
> 
> We could wait until the GOING module is moved to the UNFORMED state.
> But this might take ages when mod->exit() has to wait for something.
> 
> We could refuse to load the patch when a module is going but this is
> pretty ugly.
> 
> Example of the race:
> 
> CPU0					CPU1
> 
> delete_module()  #SYSCALL
> 
>    try_stop_module()
>      mod->state = MODULE_STATE_GOING;
> 
>    mutex_unlock(&module_mutex);
> 
> 					klp_register_patch()
> 					klp_enable_patch()
> 
> 					#save place to switch universe
> 
> 					b()     # from module that is going
> 					  a()   # from core (patched)
> 
>    mod->exit();
> 
> Note that the function b() can be called until we call mod->exit().
> 
> If we do not apply patch against b() because it is in MODULE_STATE_GOING.
> It will call patched a() with modified semantic and things might get wrong.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  include/linux/module.h  |  4 ++++
>  kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
>  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..c12f93497b74 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -344,6 +344,10 @@ struct module {
>  	unsigned long *ftrace_callsites;
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +	bool klp_gone;
> +#endif
> +
>  #ifdef CONFIG_MODULE_UNLOAD
>  	/* What modules depend on me? */
>  	struct list_head source_list;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 198f7733604b..0b38357fad0f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,16 +89,32 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {
> +	struct module *mod;
> +
>  	if (!klp_is_module(obj))
>  		return;
>  
>  	mutex_lock(&module_mutex);
> +
> +	/*
> +	 * We do not want to block removal of patched modules and therefore
> +	 * we do not take a reference here. Instead, the patches are removed
> +	 * by the going module handler instead.
> +	 */

Redundant "instead".


-- 
Josh

Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Rusty Russell]

Petr Mladek <pmladek@suse.cz> writes:
> Existing live patches are removed from going modules using a notify handler.
> There are two problems with the current implementation.
>
> First, new patch could still see the module in the GOING state even after
> the notifier has been called. It will try to initialize the related
> object structures but the module could disappear at any time. There will
> stay mess in the structures. It might even cause an invalid memory access.
>
> Second, if we start supporting patches with semantic changes between function
> calls, we would need to apply any new patch even for going modules. Note that
> the code from the module could be called even in the GOING state until
> mod->exit() finishes. See below for example.

I don't think you should handle going modules at all.  Rarely happens,
and it should happen fast.

If you can hold the module_lock, the easiest thing to do is have us wake
module_wq when a module is freed, then you can just:

        retry:
                err = wait_event_interruptible(module_wq,
                                               !modules_unloading());
                if (err)
                        goto out;

                /* Now re-check under lock. */
                mutex_lock(&module_lock);
                if (unlikely(modules_unloading()) {
                        mutex_unlock(&module_lock);
                        goto retry;
                }

Cheers,
Rusty.





>
> This patch solves the problem by adding boolean flag into struct module.
> It is switched when the going module handler is called. It marks the point
> when it is safe and we actually have to ignore the going module.
>
> Alternative solutions:
>
> We could add another lock to make the switch to GOING state and mod->exit()
> call an atomic operation. But this a nogo. It might cause a dead lock when
> some mod->exit() depends on mod->exit() from another module.
>
> We could wait until the GOING module is moved to the UNFORMED state.
> But this might take ages when mod->exit() has to wait for something.
>
> We could refuse to load the patch when a module is going but this is
> pretty ugly.
>
> Example of the race:
>
> CPU0					CPU1
>
> delete_module()  #SYSCALL
>
>    try_stop_module()
>      mod->state = MODULE_STATE_GOING;
>
>    mutex_unlock(&module_mutex);
>
> 					klp_register_patch()
> 					klp_enable_patch()
>
> 					#save place to switch universe
>
> 					b()     # from module that is going
> 					  a()   # from core (patched)
>
>    mod->exit();
>
> Note that the function b() can be called until we call mod->exit().
>
> If we do not apply patch against b() because it is in MODULE_STATE_GOING.
> It will call patched a() with modified semantic and things might get wrong.
>
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  include/linux/module.h  |  4 ++++
>  kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
>  2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..c12f93497b74 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -344,6 +344,10 @@ struct module {
>  	unsigned long *ftrace_callsites;
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +	bool klp_gone;
> +#endif
> +
>  #ifdef CONFIG_MODULE_UNLOAD
>  	/* What modules depend on me? */
>  	struct list_head source_list;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 198f7733604b..0b38357fad0f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,16 +89,32 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {
> +	struct module *mod;
> +
>  	if (!klp_is_module(obj))
>  		return;
>  
>  	mutex_lock(&module_mutex);
> +
> +	/*
> +	 * We do not want to block removal of patched modules and therefore
> +	 * we do not take a reference here. Instead, the patches are removed
> +	 * by the going module handler instead.
> +	 */
> +	mod = find_module(obj->name);
> +
>  	/*
> -	 * We don't need to take a reference on the module here because we have
> -	 * the klp_mutex, which is also taken by the module notifier.  This
> -	 * prevents any module from unloading until we release the klp_mutex.
> +	 * We must not init the object when the module is going and the notifier
> +	 * has already been called. But the patch might still be needed before.
> +	 * Note that 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.
>  	 */
> -	obj->mod = find_module(obj->name);
> +	if (mod && mod->state == MODULE_STATE_GOING && mod->klp_gone)
> +		mod = NULL;
> +
> +	obj->mod = mod;
> +
>  	mutex_unlock(&module_mutex);
>  }
>  
> @@ -929,7 +945,10 @@ int klp_module_init(struct module *mod)
>  	int ret = 0;
>  
>  	mutex_lock(&klp_mutex);
> +
> +	mod->klp_gone = false;
>  	ret = klp_module_coming(mod);
> +
>  	mutex_unlock(&klp_mutex);
>  
>  	return ret;
> @@ -985,7 +1004,10 @@ static int klp_module_notify_going(struct notifier_block *nb,
>  		return 0;
>  
>  	mutex_lock(&klp_mutex);
> +
>  	klp_module_going(mod);
> +	mod->klp_gone = true;
> +
>  	mutex_lock(&klp_mutex);
>  
>  	return 0;
> -- 
> 1.8.5.6

Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Petr Mladek]

On Sat 2015-03-07 11:34:36, Rusty Russell wrote:
> Petr Mladek <pmladek@suse.cz> writes:
> > Existing live patches are removed from going modules using a notify handler.
> > There are two problems with the current implementation.
> >
> > First, new patch could still see the module in the GOING state even after
> > the notifier has been called. It will try to initialize the related
> > object structures but the module could disappear at any time. There will
> > stay mess in the structures. It might even cause an invalid memory access.
> >
> > Second, if we start supporting patches with semantic changes between function
> > calls, we would need to apply any new patch even for going modules. Note that
> > the code from the module could be called even in the GOING state until
> > mod->exit() finishes. See below for example.
> 
> I don't think you should handle going modules at all.  Rarely happens,
> and it should happen fast.

I would like to handle it correctly. It would be pity to break a system
just because of a module removal. Also the extra overhead will be
very small and it will happen only very rarely.

We will apply one new patch and remove it quickly after that. But this
will happen only when a module is removed and a patch is added at at
the "same" time.


> If you can hold the module_lock, the easiest thing to do is have us wake
> module_wq when a module is freed, then you can just:

Unfortunately, we could not use a waitqueue easily. We would need to
release klp_mutex to do not block going modules. But we could not
do so in the middle of a patch adding.

BTW: It seems that module_wq is used for coming modules. We could not
use it for coming modules from the same reason. In addition, waiters
are weaken after mod->init(). But we would need to apply the patch
before mod->init() to avoid any inconsistency.

Anyway, thanks for feedback.

Best Regards,
Petr

 
>         retry:
>                 err = wait_event_interruptible(module_wq,
>                                                !modules_unloading());
>                 if (err)
>                         goto out;
> 
>                 /* Now re-check under lock. */
>                 mutex_lock(&module_lock);
>                 if (unlikely(modules_unloading()) {
>                         mutex_unlock(&module_lock);
>                         goto retry;
>                 }
> 
> Cheers,
> Rusty.
> 
>

Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Rusty Russell]

Petr Mladek <pmladek@suse.cz> writes:
> On Sat 2015-03-07 11:34:36, Rusty Russell wrote:
>> I don't think you should handle going modules at all.  Rarely happens,
>> and it should happen fast.
>
> I would like to handle it correctly. It would be pity to break a system
> just because of a module removal. Also the extra overhead will be
> very small and it will happen only very rarely.

I don't understand why you don't just stop modules.  I'm happy to write
"int stop_module_changes() / void restart_module_changes()" for you.

This is far far simpler.  Stop module changes before you start patching.
Restart after it's done.

Is your intent to apply patches to modules which are applied (long)
after the original patch?  Or leave that problem to userspace
(ie. assume you've updated the on-disk modules)?

>> If you can hold the module_lock, the easiest thing to do is have us wake
>> module_wq when a module is freed, then you can just:
>
> Unfortunately, we could not use a waitqueue easily. We would need to
> release klp_mutex to do not block going modules. But we could not
> do so in the middle of a patch adding.
>
> BTW: It seems that module_wq is used for coming modules. We could not
> use it for coming modules from the same reason. In addition, waiters
> are weaken after mod->init(). But we would need to apply the patch
> before mod->init() to avoid any inconsistency.

You grab the module mutex using stop_module_changes() before anything
else.

Or are you using the "failed module loading" hack to apply patches?
That would imply that the current module would have to be excluded
from the stop_module_changes() check, but should still be possible.

Cheers,
Rusty.

Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Petr Mladek]

On Tue 2015-03-10 12:53:21, Rusty Russell wrote:
> Petr Mladek <pmladek@suse.cz> writes:
> > On Sat 2015-03-07 11:34:36, Rusty Russell wrote:
> >> I don't think you should handle going modules at all.  Rarely happens,
> >> and it should happen fast.
> >
> > I would like to handle it correctly. It would be pity to break a system
> > just because of a module removal. Also the extra overhead will be
> > very small and it will happen only very rarely.
> 
> I don't understand why you don't just stop modules.  I'm happy to write
> "int stop_module_changes() / void restart_module_changes()" for you.
> 
> This is far far simpler.  Stop module changes before you start patching.
> Restart after it's done.

Interesting idea. stop_module_changes() would need to prevent anyone
from entering COMING and GOING module states. Also it would need to
wait for other modules to leave these states.

As you mentioned later, we would need to exclude the current module
because patches are added by a module and stop_module_changes() would
need to be called from mod->init() script.

In fact, we would need to exclude all modules that called
stop_module_changes() to prevent a deadlock.


> Is your intent to apply patches to modules which are applied (long)
> after the original patch?

Yes, we want to apply already loaded patches to coming modules.

> Or leave that problem to userspace (ie. assume you've updated the
> on-disk modules)?

It would make things too complicated for our use case. We allow to
install any kernel build in parallel with another kernel build. It makes
it easier to reboot with the old working kernel if things went wrong.
Most (default) modules are distributed with the kernel, so we
would need to update the kernel as well and make it special to override
the patched one.

Also it would create a strange mix. The kernel would be able to
load modules from different builds. Therefore crashdump would be much
harder to analyze.

Another problem will be with the module providing the patch. It will
be needed for the original kernel but it might[*] be incompatible with
the fixed one.

[*] There is a possibility to hardcode relocation tables. It is handy
    when the patch is generated automatically or when the patch need
    to modify a module function with an ambiguous name.

> >> If you can hold the module_lock, the easiest thing to do is have us wake
> >> module_wq when a module is freed, then you can just:
> >
> > Unfortunately, we could not use a waitqueue easily. We would need to
> > release klp_mutex to do not block going modules. But we could not
> > do so in the middle of a patch adding.
> >
> > BTW: It seems that module_wq is used for coming modules. We could not
> > use it for coming modules from the same reason. In addition, waiters
> > are weaken after mod->init(). But we would need to apply the patch
> > before mod->init() to avoid any inconsistency.
> 
> You grab the module mutex using stop_module_changes() before anything
> else.
> 
> Or are you using the "failed module loading" hack to apply patches?
> That would imply that the current module would have to be excluded
> from the stop_module_changes() check, but should still be possible.

Yes, we will need to be careful to avoid deadlocks. I think that
stop_module_changes/restart_module_changes feature makes sense only if
it will have more users. Otherwise, the approach with the extra flag
looks much easier to me. Note that there is only one boolean/bit
needed with the last version of the patch.

Thanks a lot for review and feedback. I could try to implement it
another way if you give me hints.

Best Regards,
Petr