Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed

[Petr Mladek <pmladek@suse.cz>]

On Fri 2015-03-06 11:20:32, Petr Mladek wrote:
> 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@suse.cz>
> > > ---
[...]

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

Sigh, kGraft calls kgr_module_init() after complete_formation() and
thus in MODULE_STATE_COMING. I should have refreshed my mind. There is
even a comment about this that I have written many months ago.

This brings me back to the original idea with that boolean that
marks the state before and after the coming notifier (module_init).
We could use a bitfield instead of the two booleans when requested.


Alternative solutions:

+ reject new patches when a module is coming; this is ugly

+ wait with adding new patch until the module leaves the COMING
  state; this might be dangerous or complicated; we would need
  to leave kgr_lock in the middle of the patch registration to
  avoid a deadlock with klp_module_init(); also we might need
  a waitqueue for each module which seems to be even bigger
  overhead than the two booleans

+ 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 to complex;
  the module states are checked on "many" locations


I will wait with v3 over the weekend. I hope that it will bring fresh
mind. Sigh, if I could have slept more with the baby twins.

Best Regards,
Petr