Re: [RFC PATCH] livepatch/module: Do not patch modules that are not ready

[Petr Mladek]

On Tue 2015-03-03 08:55:17, Josh Poimboeuf wrote:
> Hi Petr,
> 
> Good find.  Some comments below.
> 
> On Tue, Mar 03, 2015 at 12:38:29PM +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.
> > 
> > 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.
> > 
> > First, the module is visible even before ftrace is ready. If we enable a patch
> > in this time frame, adding ftrace ops will fail and the patch will get rejected
> > just because bad timing.
> > 
> > Second, if we are "lucky" and enable the patch for the coming module when the
> > ftrace is ready but before the module notifier has been called.
> 
> Based on the notifier priorities, it looks like the ftrace notifier gets
> called last, so I think this particular case can't happen.

Ftrace handles only going modules using the module notifier.

Coming modules are handled by ftrace_module_init() that is called directly
from load_module(). The notifiers for the coming modules seems to be
called right after by complete_formation().


> > The notifier
> > will try to enable the patch as well. It will detect that it is already patched,
> > return error, and the module will get rejected just because bad timing.
> > The more serious problem is that it will not call the notifier for
> > going module, so that the mess will stay there and we wont be able to load
> > the module later.
> > 
> > Third, similar problems are there for going module. If a patch is enabled after
> > the notifier finishes but before the module is removed from the list of modules,
> > the new patch will be applied to the module. The module might disappear at
> > anytime when the patch enabling is in progress, so there might be an access out
> > of memory. Or the whole patch might be applied and some mess will be left,
> > so it will not be possible to load/patch the module again.
> > 
> > This patch solves the problem by adding two flags into struct module. They are
> > switched when the notifier is called. Note that we try to solve a race with a
> > coming patch, therefore we do not know which modules will get patched and we
> > need to monitor all modules. This is why I added this to the struct module.
> > 
> > The flags are set and checked under the klp_mutex lock. The related operation
> > is finished under the same lock. Therefore they are properly serialized now.
> > 
> > Note that the patch solves only the situation when a new patch is registered or
> > enabled.
> 
> Did we have a reason for calling klp_find_object_module() in both
> register and enable?  I'd think we'd only need it for the register path,
> since the notifier would catch any future loads/unloads.  Or am I
> missing something?

Good question! I guess that it was there before we added the module
notifier and it was supposed to detect modules that were loaded after
the patch registration.

IMHO, we could replace klp_find_object_module() with
klp_is_object_loaded() in __klp_enable_patch(). It will work because
it will see only modules that were there during registration or
modules that were added by coming handler. It will not see modules
removed by module going handler.

I saw this problem and I considered it only an optimization. We need this
patch anyway because the race might get propagated via
klp_register_patch() and followup klp_enable_patch().

Best Regards,
Petr

 
> > 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.
> > 
> > Signed-off-by: Petr Mladek <pmladek@xxxxxxx>
> > ---
> >  include/linux/module.h  |  5 +++++
> >  kernel/livepatch/core.c | 20 +++++++++++++++++++-
> >  kernel/module.c         |  6 +++++-
> >  3 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index b653d7c0a05a..7e50d87da510 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -344,6 +344,11 @@ struct module {
> >  	unsigned long *ftrace_callsites;
> >  #endif
> >  
> > +#ifdef CONFIG_LIVEPATCH
> > +	bool klp_patched;
> > +	bool klp_unpatched;
> > +#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 a664e485365f..dee4bbcb60e6 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -89,6 +89,8 @@ 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;
> >  
> > @@ -98,7 +100,14 @@ static void klp_find_object_module(struct klp_object *obj)
> >  	 * the klp_mutex, which is also taken by the module notifier.  This
> >  	 * prevents any module from unloading until we release the klp_mutex.
> >  	 */
> > -	obj->mod = find_module(obj->name);
> > +	mod = find_module(obj->name);
> > +	/* Do not mess work of the module notifier */
> > +	if ((mod->state == MODULE_STATE_COMING && !mod->klp_patched) ||
> > +	    (mod->state == MODULE_STATE_GOING && mod->klp_unpatched))
> > +		obj->mod = NULL;
> > +	else
> > +		obj->mod = mod;
> > +
> >  	mutex_unlock(&module_mutex);
> >  }
> >  
> 
> Why do we need two flags for this?  The notifer already
> sets/clears obj->mod, so can we rely on the value obj->mod to determine
> if the notifier already ran?
> 
> For example:
> 
> @@ -89,7 +89,7 @@ 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)
>  {
> -	if (!klp_is_module(obj))
> +	if (!klp_is_module(obj) || obj->mod)
>  		return;
>  
>  	mutex_lock(&module_mutex);
> @@ -98,7 +98,9 @@ static void klp_find_object_module(struct klp_object *obj)
>  	 * the klp_mutex, which is also taken by the module notifier.  This
>  	 * prevents any module from unloading until we release the klp_mutex.
>  	 */
> -	obj->mod = find_module(obj->name);
> +	mod = find_module(obj->name);
> +	if (mod->state == MODULE_STATE_LIVE)
> +		obj->mod = mod;
>  	mutex_unlock(&module_mutex);
>  }
>  
> > @@ -927,6 +936,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_patched = true;
> > +	else
> > +		mod->klp_unpatched = true;
> > +
> >  	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..8357f15b7ed0 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -852,7 +852,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> >  
> >  	/* Store the name of the last unloaded module for diagnostic purposes */
> >  	strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
> > -
> >  	free_module(mod);
> >  	return 0;
> >  out:
> 
> Gratuitous whitespace change.
> 
> > @@ -3271,6 +3270,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	}
> >  #endif
> >  
> > +#ifdef CONFIG_LIVEPATCH
> > +	mod->klp_patched = false;
> > +	mod->klp_unpatched = false;
> > +#endif
> > +
> >  	/* To avoid stressing percpu allocator, do this once we're unique. */
> >  	err = percpu_modalloc(mod, info);
> >  	if (err)
> > -- 
> > 1.8.5.6
> > 
> 
> -- 
> Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/