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 <pmla...@suse.cz>
> ---
> include/linux/livepatch.h | 10 +++++
> kernel/livepatch/core.c | 94
> +++++++++++++++++++++++++++++++++++------------
> kernel/module.c | 9 +++++
> 3 files changed, 89 insertions(+), 24 deletions(-)
Not sure why, but "git am" seemed to think this patch was malformed. It
applied ok for me after I removed the diffstat.
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index ee6dbb39a809..78ac10546160 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -128,6 +128,16 @@ int klp_unregister_patch(struct klp_patch *);
> int klp_enable_patch(struct klp_patch *);
> int klp_disable_patch(struct klp_patch *);
>
> +int klp_module_init(struct module *mod);
> +
> +#else /* CONFIG_LIVEPATCH */
> +
> +inline int klp_module_init(struct module *mod)
Should it not be "static inline"?
/me prays not to have to break out the C spec again ;-)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_LIVEPATCH */
>
> +
> #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 7e0c83dc7215..198f7733604b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -869,8 +869,8 @@ int klp_register_patch(struct klp_patch *patch)
> }
> EXPORT_SYMBOL_GPL(klp_register_patch);
>
> -static void klp_module_notify_coming(struct klp_patch *patch,
> - struct klp_object *obj)
> +static int klp_module_coming_update_patch(struct klp_patch *patch,
> + struct klp_object *obj)
This function name confused me a little bit. Not sure what would be
better, but it really updates the object, not the patch. Maybe
klp_module_coming_object()?
> {
> struct module *pmod = patch->mod;
> struct module *mod = obj->mod;
> @@ -881,22 +881,62 @@ static void klp_module_notify_coming(struct klp_patch
> *patch,
> goto err;
>
> if (patch->state == KLP_DISABLED)
> - return;
> + return 0;
>
> pr_notice("applying patch '%s' to loading module '%s'\n",
> pmod->name, mod->name);
>
> ret = klp_enable_object(obj);
> if (!ret)
> - return;
> + return 0;
>
> err:
> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> pmod->name, mod->name, ret);
Does it still make sense to have this pr_warn() here now that we can
return an error and stop the module from loading?
I'm thinking it should be changed to pr_err() to be consistent with the
other klp error printks, and should probably say that we're preventing
the module from loading.
> + return ret;
> }
>
> -static void klp_module_notify_going(struct klp_patch *patch,
> - struct klp_object *obj)
> +static void klp_module_going(struct module *mod);
It would probably be better to move klp_module_going() here so you don't
have to forward declare it.
> +
> +int klp_module_coming(struct module *mod)
> +{
> + struct klp_patch *patch;
> + struct klp_object *obj;
> + int ret = 0;
> +
> + 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))
> + continue;
> +
> + obj->mod = mod;
> + ret = klp_module_coming_update_patch(patch, obj);
> + if (ret)
> + goto err;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + klp_module_going(mod);
> + return ret;
> +}
> +
> +
> +int klp_module_init(struct module *mod)
> +{
> + int ret = 0;
> +
> + mutex_lock(&klp_mutex);
> + ret = klp_module_coming(mod);
> + mutex_unlock(&klp_mutex);
> +
> + return ret;
> +}
> +
> +static void klp_module_going_update_patch(struct klp_patch *patch,
> + struct klp_object *obj)
> {
> struct module *pmod = patch->mod;
> struct module *mod = obj->mod;
> @@ -913,40 +953,46 @@ disabled:
> klp_free_object_loaded(obj);
> }
>
> -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> - void *data)
> +static void klp_module_going(struct module *mod)
> {
> - struct module *mod = data;
> struct klp_patch *patch;
> struct klp_object *obj;
>
> - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> - return 0;
> -
> - mutex_lock(&klp_mutex);
> -
> 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))
> + /*
> + * Handle only loaded (initialized) modules.
> + * This is needed when used in an error path.
> + */
> + if (!klp_is_object_loaded(obj) ||
> + strcmp(obj->name, mod->name))
Also need a klp_is_module() check here so it doesn't send NULL to strcmp
in the case of vmlinux.
> continue;
>
> - if (action == MODULE_STATE_COMING) {
> - obj->mod = mod;
> - klp_module_notify_coming(patch, obj);
> - } else /* MODULE_STATE_GOING */
> - klp_module_notify_going(patch, obj);
> -
> - break;
> + klp_module_going_update_patch(patch, obj);
> }
> }
>
> - mutex_unlock(&klp_mutex);
> + return;
Redundant return.
> +}
> +
> +static int klp_module_notify_going(struct notifier_block *nb,
> + unsigned long action,
> + void *data)
> +{
> + struct module *mod = data;
> +
> + if (action != MODULE_STATE_GOING)
> + return 0;
> +
> + mutex_lock(&klp_mutex);
> + klp_module_going(mod);
> + mutex_lock(&klp_mutex);
>
> return 0;
> }
>
> static struct notifier_block klp_module_nb = {
> - .notifier_call = klp_module_notify,
> + .notifier_call = klp_module_notify_going,
> .priority = INT_MIN+1, /* called late but before ftrace notifier */
> };
>
> 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).
> + 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.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
