Message-ID: <20150306102032.GI15177@pathway.suse.cz>
Date:	Fri, 6 Mar 2015 11:20:32 +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 v2 1/2] livepatch/module: Apply patch when loaded module
 is unformed

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@...e.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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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