Messages in this thread | | | Date | Fri, 6 Mar 2015 15:00:13 +0100 | From | Petr Mladek <> | Subject | Re: [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
| |