LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3] livepatch/module: Correctly handle coming and going modules
@ 2015-03-09 13:25 Petr Mladek
  2015-03-09 14:40 ` Josh Poimboeuf
  0 siblings, 1 reply; 10+ messages in thread
From: Petr Mladek @ 2015-03-09 13:25 UTC (permalink / raw)
  To: Seth Jennings, Josh Poimboeuf, Jiri Kosina, Rusty Russell
  Cc: Miroslav Benes, Masami Hiramatsu, mingo, mathieu.desnoyers, oleg,
	paulmck, live-patching, linux-kernel, andi, rostedt, tglx,
	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 but
it does not keep the lock all the time. Therefore the following races are
possible:

  1. The notifier is called sometime in STATE_MODULE_COMING. The module
     is visible by find_module() in this state all the time. It means that
     new patch can be registered and enabled even before the notifier is
     called. It might create wrong order of stacked patches, see below
     for an example.

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

This patch solves the problem by adding a boolean variable into struct module.
The value is true after the coming and before the going handler is called.
New patches need to be applied when the value is true and they need to ignore
the module when the value is false. New patches have to ignore the module
also in the UNFORMED module state because the value might be undefined.

Note that we need to know state of all modules on the system. The races are
related to new patches. Therefore we do not know what modules will get
patched.

Also note that we could not simply ignore going modules. The code from the
module could be called even in the GOING state until mod->exit() finishes.
If we start supporting patches with semantic changes between function
calls, we need to apply new patches to any still usable code.
See below for an example.

Finally note that the patch solves only the situation when a new patch is
registered. There are no such problems when the patch is being removed.
It does not matter who disable the patch first, whether the normal
disable_patch() or the module notifier. There is nothing to do
once the patch is disabled.

Alternative solutions:
======================

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

+ wait with adding new patch until the module leaves the COMING and GOING
  states; this might be dangerous and complicated; we would need to release
  kgr_lock in the middle of the patch registration to avoid a deadlock
  with the coming and going handlers; also we might need a waitqueue for
  each module which seems to be even bigger overhead than the boolean

+ 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 too complex; the module states are checked on "many"
  locations

Example of patch stacking breakage:
===================================

The notifier could _not_ _simply_ ignore already initialized module objects.
For example, let's have three patches (P1, P2, P3) for 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.

Example of the race with going modules:
=======================================

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.

[jpoimboe@redhat.com: use one boolean instead of two]
Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
Changes in v3:

+ reverted back to v1:
  + cannot handle coming modules in UNFORMED module state because
    kallsyms are not ready => need to use the boolean again
  + neither split nor handle errors in the module coming handler for now;
    this change will be need only for more complex consistency model;
    let's keep this patch(set) as easy as possible
+ just keep the check for mod is not NULL from v2
+ use one boolean as suggested by Josh

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


 include/linux/module.h  |  4 ++++
 kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
 kernel/module.c         |  4 ++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index b653d7c0a05a..7232fde6a991 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_alive;
+#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 fc037345dbd4..2bc0d1dd2f62 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -89,16 +89,28 @@ 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 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 do not want to block removal of patched modules and therefore
+	 * we do not take a reference here. The patches are removed by
+	 * a going module handler instead.
+	 */
+	mod = find_module(obj->name);
+	/*
+	 * Do not mess work of the module coming and going notifiers.
+	 * Note that the patch might still be needed before the going handler
+	 * is called. Module functions can be called even in the GOING state
+	 * until mod->exit() finishes. This is especially important for
+	 * patches that modify semantic of the functions.
 	 */
-	obj->mod = find_module(obj->name);
+	if (mod && mod->state != MODULE_STATE_UNFORMED && mod->klp_alive)
+		obj->mod = mod;
+
 	mutex_unlock(&module_mutex);
 }
 
@@ -736,6 +748,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 		return -EINVAL;
 
 	obj->state = KLP_DISABLED;
+	obj->mod = NULL;
 
 	klp_find_object_module(obj);
 
@@ -926,6 +939,15 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 
 	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.
+	 */
+	if (action == MODULE_STATE_COMING)
+		mod->klp_alive = true;
+	else /* MODULE_STATE_GOING */
+		mod->klp_alive = false;
+
 	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))
diff --git a/kernel/module.c b/kernel/module.c
index d856e96a3cce..b3ffc231ce0d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	}
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+	mod->klp_alive = false;
+#endif
+
 	/* To avoid stressing percpu allocator, do this once we're unique. */
 	err = percpu_modalloc(mod, info);
 	if (err)
-- 
1.8.5.6


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-09 13:25 [PATCH v3] livepatch/module: Correctly handle coming and going modules Petr Mladek
@ 2015-03-09 14:40 ` Josh Poimboeuf
  2015-03-10 12:01   ` Petr Mladek
  0 siblings, 1 reply; 10+ messages in thread
From: Josh Poimboeuf @ 2015-03-09 14:40 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> 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 but
> it does not keep the lock all the time. Therefore the following races are
> possible:
> 
>   1. The notifier is called sometime in STATE_MODULE_COMING. The module
>      is visible by find_module() in this state all the time. It means that
>      new patch can be registered and enabled even before the notifier is
>      called. It might create wrong order of stacked patches, see below
>      for an example.
> 
>    2. 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.
> 
> This patch solves the problem by adding a boolean variable into struct module.
> The value is true after the coming and before the going handler is called.
> New patches need to be applied when the value is true and they need to ignore
> the module when the value is false. New patches have to ignore the module
> also in the UNFORMED module state because the value might be undefined.
> 
> Note that we need to know state of all modules on the system. The races are
> related to new patches. Therefore we do not know what modules will get
> patched.
> 
> Also note that we could not simply ignore going modules. The code from the
> module could be called even in the GOING state until mod->exit() finishes.
> If we start supporting patches with semantic changes between function
> calls, we need to apply new patches to any still usable code.
> See below for an example.
> 
> Finally note that the patch solves only the situation when a new patch is
> registered. There are no such problems when the patch is being removed.
> It does not matter who disable the patch first, whether the normal
> disable_patch() or the module notifier. There is nothing to do
> once the patch is disabled.
> 
> Alternative solutions:
> ======================
> 
> + reject new patches when a patched module is coming or going; this is ugly
> 
> + wait with adding new patch until the module leaves the COMING and GOING
>   states; this might be dangerous and complicated; we would need to release
>   kgr_lock in the middle of the patch registration to avoid a deadlock
>   with the coming and going handlers; also we might need a waitqueue for
>   each module which seems to be even bigger overhead than the boolean
> 
> + 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 too complex; the module states are checked on "many"
>   locations
> 
> Example of patch stacking breakage:
> ===================================
> 
> The notifier could _not_ _simply_ ignore already initialized module objects.
> For example, let's have three patches (P1, P2, P3) for 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.
> 
> Example of the race with going modules:
> =======================================
> 
> 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.
> 
> [jpoimboe@redhat.com: use one boolean instead of two]
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
> Changes in v3:
> 
> + reverted back to v1:
>   + cannot handle coming modules in UNFORMED module state because
>     kallsyms are not ready => need to use the boolean again
>   + neither split nor handle errors in the module coming handler for now;
>     this change will be need only for more complex consistency model;
>     let's keep this patch(set) as easy as possible
> + just keep the check for mod is not NULL from v2
> + use one boolean as suggested by Josh
> 
> 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
> 
> 
>  include/linux/module.h  |  4 ++++
>  kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
>  kernel/module.c         |  4 ++++
>  3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..7232fde6a991 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_alive;
> +#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 fc037345dbd4..2bc0d1dd2f62 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,16 +89,28 @@ 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 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 do not want to block removal of patched modules and therefore
> +	 * we do not take a reference here. The patches are removed by
> +	 * a going module handler instead.
> +	 */
> +	mod = find_module(obj->name);
> +	/*
> +	 * Do not mess work of the module coming and going notifiers.
> +	 * Note that the patch might still be needed before the going handler
> +	 * is called. Module functions can be called even in the GOING state
> +	 * until mod->exit() finishes. This is especially important for
> +	 * patches that modify semantic of the functions.
>  	 */
> -	obj->mod = find_module(obj->name);
> +	if (mod && mod->state != MODULE_STATE_UNFORMED && mod->klp_alive)
> +		obj->mod = mod;

It looks like find_module() doesn't return UNFORMED modules, so no need
check for them here.

> +
>  	mutex_unlock(&module_mutex);
>  }
>  
> @@ -736,6 +748,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  		return -EINVAL;
>  
>  	obj->state = KLP_DISABLED;
> +	obj->mod = NULL;
>  
>  	klp_find_object_module(obj);
>  
> @@ -926,6 +939,15 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  
>  	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.
> +	 */
> +	if (action == MODULE_STATE_COMING)
> +		mod->klp_alive = true;
> +	else /* MODULE_STATE_GOING */
> +		mod->klp_alive = false;
> +

Any reason why this needs to be protected by the 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))
> diff --git a/kernel/module.c b/kernel/module.c
> index d856e96a3cce..b3ffc231ce0d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	}
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +	mod->klp_alive = false;
> +#endif
> +

I don't think you need this initialization.  It looks like the module
struct is embedded in the mod->module_core region which is initialized
to zero in move_module().

-- 
Josh

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-09 14:40 ` Josh Poimboeuf
@ 2015-03-10 12:01   ` Petr Mladek
  2015-03-10 14:22     ` Josh Poimboeuf
  0 siblings, 1 reply; 10+ messages in thread
From: Petr Mladek @ 2015-03-10 12:01 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > 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 but
> > it does not keep the lock all the time. Therefore the following races are
> > possible:
> > 
> >   1. The notifier is called sometime in STATE_MODULE_COMING. The module
> >      is visible by find_module() in this state all the time. It means that
> >      new patch can be registered and enabled even before the notifier is
> >      called. It might create wrong order of stacked patches, see below
> >      for an example.
> > 
> >    2. 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.
> > 
> > This patch solves the problem by adding a boolean variable into struct module.
> > The value is true after the coming and before the going handler is called.
> > New patches need to be applied when the value is true and they need to ignore
> > the module when the value is false. New patches have to ignore the module
> > also in the UNFORMED module state because the value might be undefined.
> > 
> > Note that we need to know state of all modules on the system. The races are
> > related to new patches. Therefore we do not know what modules will get
> > patched.
> > 
> > Also note that we could not simply ignore going modules. The code from the
> > module could be called even in the GOING state until mod->exit() finishes.
> > If we start supporting patches with semantic changes between function
> > calls, we need to apply new patches to any still usable code.
> > See below for an example.
> > 
> > Finally note that the patch solves only the situation when a new patch is
> > registered. There are no such problems when the patch is being removed.
> > It does not matter who disable the patch first, whether the normal
> > disable_patch() or the module notifier. There is nothing to do
> > once the patch is disabled.
> > 
> > Alternative solutions:
> > ======================
> > 
> > + reject new patches when a patched module is coming or going; this is ugly
> > 
> > + wait with adding new patch until the module leaves the COMING and GOING
> >   states; this might be dangerous and complicated; we would need to release
> >   kgr_lock in the middle of the patch registration to avoid a deadlock
> >   with the coming and going handlers; also we might need a waitqueue for
> >   each module which seems to be even bigger overhead than the boolean
> > 
> > + 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 too complex; the module states are checked on "many"
> >   locations
> > 
> > Example of patch stacking breakage:
> > ===================================
> > 
> > The notifier could _not_ _simply_ ignore already initialized module objects.
> > For example, let's have three patches (P1, P2, P3) for 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.
> > 
> > Example of the race with going modules:
> > =======================================
> > 
> > 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.
> > 
> > [jpoimboe@redhat.com: use one boolean instead of two]
> > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > ---
> > Changes in v3:
> > 
> > + reverted back to v1:
> >   + cannot handle coming modules in UNFORMED module state because
> >     kallsyms are not ready => need to use the boolean again
> >   + neither split nor handle errors in the module coming handler for now;
> >     this change will be need only for more complex consistency model;
> >     let's keep this patch(set) as easy as possible
> > + just keep the check for mod is not NULL from v2
> > + use one boolean as suggested by Josh
> > 
> > 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
> > 
> > 
> >  include/linux/module.h  |  4 ++++
> >  kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
> >  kernel/module.c         |  4 ++++
> >  3 files changed, 34 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index b653d7c0a05a..7232fde6a991 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_alive;
> > +#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 fc037345dbd4..2bc0d1dd2f62 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -89,16 +89,28 @@ 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 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 do not want to block removal of patched modules and therefore
> > +	 * we do not take a reference here. The patches are removed by
> > +	 * a going module handler instead.
> > +	 */
> > +	mod = find_module(obj->name);
> > +	/*
> > +	 * Do not mess work of the module coming and going notifiers.
> > +	 * Note that the patch might still be needed before the going handler
> > +	 * is called. Module functions can be called even in the GOING state
> > +	 * until mod->exit() finishes. This is especially important for
> > +	 * patches that modify semantic of the functions.
> >  	 */
> > -	obj->mod = find_module(obj->name);
> > +	if (mod && mod->state != MODULE_STATE_UNFORMED && mod->klp_alive)
> > +		obj->mod = mod;
> 
> It looks like find_module() doesn't return UNFORMED modules, so no need
> check for them here.

Good catch.

> > +
> >  	mutex_unlock(&module_mutex);
> >  }
> >  
> > @@ -736,6 +748,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> >  		return -EINVAL;
> >  
> >  	obj->state = KLP_DISABLED;
> > +	obj->mod = NULL;
> >  
> >  	klp_find_object_module(obj);
> >  
> > @@ -926,6 +939,15 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> >  
> >  	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.
> > +	 */
> > +	if (action == MODULE_STATE_COMING)
> > +		mod->klp_alive = true;
> > +	else /* MODULE_STATE_GOING */
> > +		mod->klp_alive = false;
> > +
> 
> Any reason why this needs to be protected by the mutex?

We need to synchronize it with the check in
klp_find_object_module(). Otherwise, for example, the check might read
"true" and add/enable new patch but this notify handler will be
blocked until the patch is added => it will mess the order of
patches.

It might be more clean to take module_mutex here but the value is
needed only by livepatching, so klp_mutex seems to be enough.


> >  	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))
> > diff --git a/kernel/module.c b/kernel/module.c
> > index d856e96a3cce..b3ffc231ce0d 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	}
> >  #endif
> >  
> > +#ifdef CONFIG_LIVEPATCH
> > +	mod->klp_alive = false;
> > +#endif
> > +
> 
> I don't think you need this initialization.  It looks like the module
> struct is embedded in the mod->module_core region which is initialized
> to zero in move_module().

I have looked at this before but I was not able to find a code
zeroing struct module. If I get it correctly, mod->module_core
is a location where symbol table sections are copied or so.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 12:01   ` Petr Mladek
@ 2015-03-10 14:22     ` Josh Poimboeuf
  2015-03-10 14:36       ` Petr Mladek
  0 siblings, 1 reply; 10+ messages in thread
From: Josh Poimboeuf @ 2015-03-10 14:22 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > +
> > >  	mutex_unlock(&module_mutex);
> > >  }
> > >  
> > > @@ -736,6 +748,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> > >  		return -EINVAL;
> > >  
> > >  	obj->state = KLP_DISABLED;
> > > +	obj->mod = NULL;
> > >  
> > >  	klp_find_object_module(obj);
> > >  
> > > @@ -926,6 +939,15 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  
> > >  	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.
> > > +	 */
> > > +	if (action == MODULE_STATE_COMING)
> > > +		mod->klp_alive = true;
> > > +	else /* MODULE_STATE_GOING */
> > > +		mod->klp_alive = false;
> > > +
> > 
> > Any reason why this needs to be protected by the mutex?
> 
> We need to synchronize it with the check in
> klp_find_object_module(). Otherwise, for example, the check might read
> "true" and add/enable new patch but this notify handler will be
> blocked until the patch is added => it will mess the order of
> patches.
> 
> It might be more clean to take module_mutex here but the value is
> needed only by livepatching, so klp_mutex seems to be enough.

Ah, right.  Looks good to me.

> > >  	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))
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index d856e96a3cce..b3ffc231ce0d 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > >  	}
> > >  #endif
> > >  
> > > +#ifdef CONFIG_LIVEPATCH
> > > +	mod->klp_alive = false;
> > > +#endif
> > > +
> > 
> > I don't think you need this initialization.  It looks like the module
> > struct is embedded in the mod->module_core region which is initialized
> > to zero in move_module().
> 
> I have looked at this before but I was not able to find a code
> zeroing struct module. If I get it correctly, mod->module_core
> is a location where symbol table sections are copied or so.

Yeah, it's far from obvious.  AFAICT, it's cleared by the
"memset(ptr, 0, mod->core_size)" line.

-- 
Josh

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 14:22     ` Josh Poimboeuf
@ 2015-03-10 14:36       ` Petr Mladek
  2015-03-10 14:47         ` Josh Poimboeuf
  0 siblings, 1 reply; 10+ messages in thread
From: Petr Mladek @ 2015-03-10 14:36 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Tue 2015-03-10 09:22:04, Josh Poimboeuf wrote:
> On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> > On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > index d856e96a3cce..b3ffc231ce0d 100644
> > > > --- a/kernel/module.c
> > > > +++ b/kernel/module.c
> > > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > > >  	}
> > > >  #endif
> > > >  
> > > > +#ifdef CONFIG_LIVEPATCH
> > > > +	mod->klp_alive = false;
> > > > +#endif
> > > > +
> > > 
> > > I don't think you need this initialization.  It looks like the module
> > > struct is embedded in the mod->module_core region which is initialized
> > > to zero in move_module().
> > 
> > I have looked at this before but I was not able to find a code
> > zeroing struct module. If I get it correctly, mod->module_core
> > is a location where symbol table sections are copied or so.
> 
> Yeah, it's far from obvious.  AFAICT, it's cleared by the
> "memset(ptr, 0, mod->core_size)" line.

What I wanted to say is that module_core is not struct module. It
seems that it points to the module code. See within_module_core() and
how it is used().

By other words, I think that struct module is not zeroed and we need
to initialize the bool.

Or did I miss anything?

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 14:36       ` Petr Mladek
@ 2015-03-10 14:47         ` Josh Poimboeuf
  2015-03-10 16:58           ` Petr Mladek
  0 siblings, 1 reply; 10+ messages in thread
From: Josh Poimboeuf @ 2015-03-10 14:47 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Tue, Mar 10, 2015 at 03:36:17PM +0100, Petr Mladek wrote:
> On Tue 2015-03-10 09:22:04, Josh Poimboeuf wrote:
> > On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> > > On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > > > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > > index d856e96a3cce..b3ffc231ce0d 100644
> > > > > --- a/kernel/module.c
> > > > > +++ b/kernel/module.c
> > > > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > > > >  	}
> > > > >  #endif
> > > > >  
> > > > > +#ifdef CONFIG_LIVEPATCH
> > > > > +	mod->klp_alive = false;
> > > > > +#endif
> > > > > +
> > > > 
> > > > I don't think you need this initialization.  It looks like the module
> > > > struct is embedded in the mod->module_core region which is initialized
> > > > to zero in move_module().
> > > 
> > > I have looked at this before but I was not able to find a code
> > > zeroing struct module. If I get it correctly, mod->module_core
> > > is a location where symbol table sections are copied or so.
> > 
> > Yeah, it's far from obvious.  AFAICT, it's cleared by the
> > "memset(ptr, 0, mod->core_size)" line.
> 
> What I wanted to say is that module_core is not struct module. It
> seems that it points to the module code. See within_module_core() and
> how it is used().
> 
> By other words, I think that struct module is not zeroed and we need
> to initialize the bool.
> 
> Or did I miss anything?

My understanding is that module_core is not only code.  It also contains the
struct module itself.  Verified in crash:

  crash> mod |head -n2
       MODULE       NAME                        SIZE  OBJECT FILE
  ffffffffa0003180  video                      19905  (not loaded)  [CONFIG_KALLSYMS]
  crash> module.module_core,core_size 0xffffffffa0003180
    module_core = 0xffffffffa0000000
    core_size = 0x4dc1


-- 
Josh

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 14:47         ` Josh Poimboeuf
@ 2015-03-10 16:58           ` Petr Mladek
  2015-03-10 17:07             ` Petr Mladek
  2015-03-10 17:30             ` Josh Poimboeuf
  0 siblings, 2 replies; 10+ messages in thread
From: Petr Mladek @ 2015-03-10 16:58 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Tue 2015-03-10 09:47:01, Josh Poimboeuf wrote:
> On Tue, Mar 10, 2015 at 03:36:17PM +0100, Petr Mladek wrote:
> > On Tue 2015-03-10 09:22:04, Josh Poimboeuf wrote:
> > > On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> > > > On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > > > > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > > > index d856e96a3cce..b3ffc231ce0d 100644
> > > > > > --- a/kernel/module.c
> > > > > > +++ b/kernel/module.c
> > > > > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > > > > >  	}
> > > > > >  #endif
> > > > > >  
> > > > > > +#ifdef CONFIG_LIVEPATCH
> > > > > > +	mod->klp_alive = false;
> > > > > > +#endif
> > > > > > +
> > > > > 
> > > > > I don't think you need this initialization.  It looks like the module
> > > > > struct is embedded in the mod->module_core region which is initialized
> > > > > to zero in move_module().
> > > > 
> > > > I have looked at this before but I was not able to find a code
> > > > zeroing struct module. If I get it correctly, mod->module_core
> > > > is a location where symbol table sections are copied or so.
> > > 
> > > Yeah, it's far from obvious.  AFAICT, it's cleared by the
> > > "memset(ptr, 0, mod->core_size)" line.
> > 
> > What I wanted to say is that module_core is not struct module. It
> > seems that it points to the module code. See within_module_core() and
> > how it is used().
> > 
> > By other words, I think that struct module is not zeroed and we need
> > to initialize the bool.
> > 
> > Or did I miss anything?
> 
> My understanding is that module_core is not only code.  It also contains the
> struct module itself.  Verified in crash:
> 
>   crash> mod |head -n2
>        MODULE       NAME                        SIZE  OBJECT FILE
>   ffffffffa0003180  video                      19905  (not loaded)  [CONFIG_KALLSYMS]
>   crash> module.module_core,core_size 0xffffffffa0003180
>     module_core = 0xffffffffa0000000
>     core_size = 0x4dc1

OK, you are right that struct module is inside mod->module_core. But I
am still not convinced that the structure is zeroed.

There are the following commands in move_module()

	ptr = module_alloc_update_bounds(mod->core_size);
[...]
	memset(ptr, 0, mod->core_size);
	mod->module_core = ptr;

	if (mod->init_size) {
[...]
	} else
		mod->module_init = NULL;


The needed memory is allocated and zeroed but the pointer
is written to the temporary place.

I do not see any code that would copy parts of struct module from the
temporary place to the newly allocated one. It seems that the whole
structure is copied.

Huh, the code is really twisted but I think that the space for the
temporary structure is not zeroed. One week proof is that the code
does mod->module_init = NULL; It would not make sense if the
temporary location was zeroed.


Of course, I will be happy if anyone convince me that I am wrong and
we could omit the initialization.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 16:58           ` Petr Mladek
@ 2015-03-10 17:07             ` Petr Mladek
  2015-03-10 17:30             ` Josh Poimboeuf
  1 sibling, 0 replies; 10+ messages in thread
From: Petr Mladek @ 2015-03-10 17:07 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Tue 2015-03-10 17:58:02, Petr Mladek wrote:
> On Tue 2015-03-10 09:47:01, Josh Poimboeuf wrote:
> > On Tue, Mar 10, 2015 at 03:36:17PM +0100, Petr Mladek wrote:
> > > On Tue 2015-03-10 09:22:04, Josh Poimboeuf wrote:
> > > > On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> > > > > On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > > > > > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > > > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > > > > index d856e96a3cce..b3ffc231ce0d 100644
> > > > > > > --- a/kernel/module.c
> > > > > > > +++ b/kernel/module.c
> > > > > > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > > > > > >  	}
> > > > > > >  #endif
> > > > > > >  
> > > > > > > +#ifdef CONFIG_LIVEPATCH
> > > > > > > +	mod->klp_alive = false;
> > > > > > > +#endif
> > > > > > > +
> > > > > > 
> > > > > > I don't think you need this initialization.  It looks like the module
> > > > > > struct is embedded in the mod->module_core region which is initialized
> > > > > > to zero in move_module().
> > > > > 
> > > > > I have looked at this before but I was not able to find a code
> > > > > zeroing struct module. If I get it correctly, mod->module_core
> > > > > is a location where symbol table sections are copied or so.
> > > > 
> > > > Yeah, it's far from obvious.  AFAICT, it's cleared by the
> > > > "memset(ptr, 0, mod->core_size)" line.
> > > 
> > > What I wanted to say is that module_core is not struct module. It
> > > seems that it points to the module code. See within_module_core() and
> > > how it is used().
> > > 
> > > By other words, I think that struct module is not zeroed and we need
> > > to initialize the bool.
> > > 
> > > Or did I miss anything?
> > 
> > My understanding is that module_core is not only code.  It also contains the
> > struct module itself.  Verified in crash:
> > 
> >   crash> mod |head -n2
> >        MODULE       NAME                        SIZE  OBJECT FILE
> >   ffffffffa0003180  video                      19905  (not loaded)  [CONFIG_KALLSYMS]
> >   crash> module.module_core,core_size 0xffffffffa0003180
> >     module_core = 0xffffffffa0000000
> >     core_size = 0x4dc1
> 
> OK, you are right that struct module is inside mod->module_core. But I
> am still not convinced that the structure is zeroed.
> 
> There are the following commands in move_module()
> 
> 	ptr = module_alloc_update_bounds(mod->core_size);
> [...]
> 	memset(ptr, 0, mod->core_size);
> 	mod->module_core = ptr;
> 
> 	if (mod->init_size) {
> [...]
> 	} else
> 		mod->module_init = NULL;
> 
> 
> The needed memory is allocated and zeroed but the pointer
> is written to the temporary place.
> 
> I do not see any code that would copy parts of struct module from the
> temporary place to the newly allocated one. It seems that the whole
> structure is copied.
> 
> Huh, the code is really twisted but I think that the space for the
> temporary structure is not zeroed. One week proof is that the code
> does mod->module_init = NULL; It would not make sense if the
> temporary location was zeroed.

Just got an idea and found one more piece. The original structure is
loaded from the module binary. It is stored in the special section
.gnu.linkonce.this_module. I guess that the linker zeroes unknown
fields but I am not sure if we could rely on it.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 16:58           ` Petr Mladek
  2015-03-10 17:07             ` Petr Mladek
@ 2015-03-10 17:30             ` Josh Poimboeuf
  2015-03-12  0:33               ` Rusty Russell
  1 sibling, 1 reply; 10+ messages in thread
From: Josh Poimboeuf @ 2015-03-10 17:30 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Seth Jennings, Jiri Kosina, Rusty Russell, Miroslav Benes,
	Masami Hiramatsu, mingo, mathieu.desnoyers, oleg, paulmck,
	live-patching, linux-kernel, andi, rostedt, tglx

On Tue, Mar 10, 2015 at 05:58:02PM +0100, Petr Mladek wrote:
> On Tue 2015-03-10 09:47:01, Josh Poimboeuf wrote:
> > On Tue, Mar 10, 2015 at 03:36:17PM +0100, Petr Mladek wrote:
> > > On Tue 2015-03-10 09:22:04, Josh Poimboeuf wrote:
> > > > On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> > > > > On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > > > > > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > > > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > > > > index d856e96a3cce..b3ffc231ce0d 100644
> > > > > > > --- a/kernel/module.c
> > > > > > > +++ b/kernel/module.c
> > > > > > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > > > > > >  	}
> > > > > > >  #endif
> > > > > > >  
> > > > > > > +#ifdef CONFIG_LIVEPATCH
> > > > > > > +	mod->klp_alive = false;
> > > > > > > +#endif
> > > > > > > +
> > > > > > 
> > > > > > I don't think you need this initialization.  It looks like the module
> > > > > > struct is embedded in the mod->module_core region which is initialized
> > > > > > to zero in move_module().
> > > > > 
> > > > > I have looked at this before but I was not able to find a code
> > > > > zeroing struct module. If I get it correctly, mod->module_core
> > > > > is a location where symbol table sections are copied or so.
> > > > 
> > > > Yeah, it's far from obvious.  AFAICT, it's cleared by the
> > > > "memset(ptr, 0, mod->core_size)" line.
> > > 
> > > What I wanted to say is that module_core is not struct module. It
> > > seems that it points to the module code. See within_module_core() and
> > > how it is used().
> > > 
> > > By other words, I think that struct module is not zeroed and we need
> > > to initialize the bool.
> > > 
> > > Or did I miss anything?
> > 
> > My understanding is that module_core is not only code.  It also contains the
> > struct module itself.  Verified in crash:
> > 
> >   crash> mod |head -n2
> >        MODULE       NAME                        SIZE  OBJECT FILE
> >   ffffffffa0003180  video                      19905  (not loaded)  [CONFIG_KALLSYMS]
> >   crash> module.module_core,core_size 0xffffffffa0003180
> >     module_core = 0xffffffffa0000000
> >     core_size = 0x4dc1
> 
> OK, you are right that struct module is inside mod->module_core. But I
> am still not convinced that the structure is zeroed.
> 
> There are the following commands in move_module()
> 
> 	ptr = module_alloc_update_bounds(mod->core_size);
> [...]
> 	memset(ptr, 0, mod->core_size);
> 	mod->module_core = ptr;
> 
> 	if (mod->init_size) {
> [...]
> 	} else
> 		mod->module_init = NULL;
> 
> 
> The needed memory is allocated and zeroed but the pointer
> is written to the temporary place.
> 
> I do not see any code that would copy parts of struct module from the
> temporary place to the newly allocated one. It seems that the whole
> structure is copied.
> 
> Huh, the code is really twisted but I think that the space for the
> temporary structure is not zeroed. One week proof is that the code
> does mod->module_init = NULL; It would not make sense if the
> temporary location was zeroed.
> 
> 
> Of course, I will be happy if anyone convince me that I am wrong and
> we could omit the initialization.

Ok, I didn't realize the struct module was copied as part of the elf
section copy logic.  I think we're getting close now :-)

>From a generated mod.C file:

  __visible struct module __this_module
  __attribute__((section(".gnu.linkonce.this_module"))) = {
          .name = KBUILD_MODNAME,
          .init = init_module,
  #ifdef CONFIG_MODULE_UNLOAD
          .exit = cleanup_module,
  #endif
          .arch = MODULE_ARCH_INIT,
  };

The struct is statically initialized as a global variable in the module.
So the elements not specified in the static initializer should all be
cleared to zero by the compiler.

-- 
Josh

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules
  2015-03-10 17:30             ` Josh Poimboeuf
@ 2015-03-12  0:33               ` Rusty Russell
  0 siblings, 0 replies; 10+ messages in thread
From: Rusty Russell @ 2015-03-12  0:33 UTC (permalink / raw)
  To: Josh Poimboeuf, Petr Mladek
  Cc: Seth Jennings, Jiri Kosina, Miroslav Benes, Masami Hiramatsu,
	mingo, mathieu.desnoyers, oleg, paulmck, live-patching,
	linux-kernel, andi, rostedt, tglx

Josh Poimboeuf <jpoimboe@redhat.com> writes:
> The struct is statically initialized as a global variable in the module.
> So the elements not specified in the static initializer should all be
> cleared to zero by the compiler.

Yep, it's zeroed.

Cheers,
Rusty.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-03-12  1:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-09 13:25 [PATCH v3] livepatch/module: Correctly handle coming and going modules Petr Mladek
2015-03-09 14:40 ` Josh Poimboeuf
2015-03-10 12:01   ` Petr Mladek
2015-03-10 14:22     ` Josh Poimboeuf
2015-03-10 14:36       ` Petr Mladek
2015-03-10 14:47         ` Josh Poimboeuf
2015-03-10 16:58           ` Petr Mladek
2015-03-10 17:07             ` Petr Mladek
2015-03-10 17:30             ` Josh Poimboeuf
2015-03-12  0:33               ` Rusty Russell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).