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

[Petr Mladek]

Ah, I have missed the comments in the code in the first reply.

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

We need this for new patches. There we do not know if the module
notifier has already been called or not, see also below.

Of course, we might use more optimal storage for the two flags
if requested. For example, two bits in an unsigned char.


> 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)

The check of the MODULE_STATE_LIVE is not enough. We need to init
modules when a new patch is registered, the module is still in
MODULE_STATE_COMING and the module notify handler has already
been called.


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

Ah, great catch. I will remove this in v2.

Best Regards,
Petr


> > @@ -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/