Date:	Tue, 10 Mar 2015 13:01:07 +0100
From:	Petr Mladek <pmladek@...e.cz>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Miroslav Benes <mbenes@...e.cz>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	mingo@...nel.org, mathieu.desnoyers@...icios.com, oleg@...hat.com,
	paulmck@...ux.vnet.ibm.com, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, andi@...stfloor.org,
	rostedt@...dmis.org, tglx@...utronix.de
Subject: Re: [PATCH v3] livepatch/module: Correctly handle coming and going
 modules

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@...hat.com: use one boolean instead of two]
> > Signed-off-by: Petr Mladek <pmladek@...e.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists