lkml.org 
[lkml]   [2015]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed
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


\
 
 \ /
  Last update: 2015-03-06 15:21    [W:0.057 / U:0.796 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site