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

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Mar 03, 2015 at 04:48:16PM +0100, Petr Mladek wrote:
> 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().

Ah, ok.  Makes sense.

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

Yeah, and __klp_enable_patch() already calls klp_is_object_loaded(), so
we can just remove its call to klp_find_object_module().

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

I agree, the race exists regardless.

Also, you missed my two code comments which are buried below :-)

Thanks,
Josh

> > > 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@suse.cz>
> > > ---
> > >  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
> > >