On Mon, Mar 09, 2015 at 02:25:28PM +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 but
> it does not keep the lock all the time. Therefore the following races are
> possible:
>
> 1. The notifier is called sometime in STATE_MODULE_COMING. The module
> is visible by find_module() in this state all the time. It means that
> new patch can be registered and enabled even before the notifier is
> called. It might create wrong order of stacked patches, see below
> for an example.
>
> 2. New patch could still see the module in the GOING state even after
> the notifier has been called. It will try to initialize the related
> object structures but the module could disappear at any time. There
> will stay mess in the structures. It might even cause an invalid
> memory access.
>
> This patch solves the problem by adding a boolean variable into struct module.
> The value is true after the coming and before the going handler is called.
> New patches need to be applied when the value is true and they need to ignore
> the module when the value is false. New patches have to ignore the module
> also in the UNFORMED module state because the value might be undefined.
>
> Note that we need to know state of all modules on the system. The races are
> related to new patches. Therefore we do not know what modules will get
> patched.
>
> Also note that we could not simply ignore going modules. The code from the
> module could be called even in the GOING state until mod->exit() finishes.
> If we start supporting patches with semantic changes between function
> calls, we need to apply new patches to any still usable code.
> See below for an example.
>
> Finally note that the patch solves only the situation when a new patch is
> registered. 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.
>
> Alternative solutions:
> ======================
>
> + reject new patches when a patched module is coming or going; this is ugly
>
> + wait with adding new patch until the module leaves the COMING and GOING
> states; this might be dangerous and complicated; we would need to release
> kgr_lock in the middle of the patch registration to avoid a deadlock
> with the coming and going handlers; also we might need a waitqueue for
> each module which seems to be even bigger overhead than the boolean
>
> + 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 too complex; the module states are checked on "many"
> locations
>
> Example of patch stacking breakage:
> ===================================
>
> The notifier could _not_ _simply_ ignore already initialized module objects.
> For example, let's have three patches (P1, P2, P3) for 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.
>
> Example of the race with going modules:
> =======================================
>
> CPU0 CPU1
>
> delete_module() #SYSCALL
>
> try_stop_module()
> mod->state = MODULE_STATE_GOING;
>
> mutex_unlock(&module_mutex);
>
> klp_register_patch()
> klp_enable_patch()
>
> #save place to switch universe
>
> b() # from module that is going
> a() # from core (patched)
>
> mod->exit();
>
> Note that the function b() can be called until we call mod->exit().
>
> If we do not apply patch against b() because it is in MODULE_STATE_GOING,
> it will call patched a() with modified semantic and things might get wrong.
>
> [jpoimboe@xxxxxxxxxx: use one boolean instead of two]
> Signed-off-by: Petr Mladek <pmladek@xxxxxxx>
> ---
> Changes in v3:
>
> + reverted back to v1:
> + cannot handle coming modules in UNFORMED module state because
> kallsyms are not ready => need to use the boolean again
> + neither split nor handle errors in the module coming handler for now;
> this change will be need only for more complex consistency model;
> let's keep this patch(set) as easy as possible
> + just keep the check for mod is not NULL from v2
> + use one boolean as suggested by Josh
>
> Changes in v2:
>
> + split fix for coming and going modules
> + call klp_module_init() directly instead of using a handler
> + check if mod is not NULL when checking the module state
> + use the boolean flag only for going handler
>
>
> include/linux/module.h | 4 ++++
> kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
> kernel/module.c | 4 ++++
> 3 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..7232fde6a991 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -344,6 +344,10 @@ struct module {
> unsigned long *ftrace_callsites;
> #endif
>
> +#ifdef CONFIG_LIVEPATCH
> + bool klp_alive;
> +#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 fc037345dbd4..2bc0d1dd2f62 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,16 +89,28 @@ 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;
>
> mutex_lock(&module_mutex);
> /*
> - * We don't need to take a reference on the module here because we have
> - * the klp_mutex, which is also taken by the module notifier. This
> - * prevents any module from unloading until we release the klp_mutex.
> + * We do not want to block removal of patched modules and therefore
> + * we do not take a reference here. The patches are removed by
> + * a going module handler instead.
> + */
> + mod = find_module(obj->name);
> + /*
> + * Do not mess work of the module coming and going notifiers.
> + * Note that the patch might still be needed before the going handler
> + * is called. Module functions can be called even in the GOING state
> + * until mod->exit() finishes. This is especially important for
> + * patches that modify semantic of the functions.
> */
> - obj->mod = find_module(obj->name);
> + if (mod && mod->state != MODULE_STATE_UNFORMED && mod->klp_alive)
> + obj->mod = mod;
It looks like find_module() doesn't return UNFORMED modules, so no need
check for them here.
> +
> mutex_unlock(&module_mutex);
> }
>
> @@ -736,6 +748,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> return -EINVAL;
>
> obj->state = KLP_DISABLED;
> + obj->mod = NULL;
>
> klp_find_object_module(obj);
>
> @@ -926,6 +939,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_alive = true;
> + else /* MODULE_STATE_GOING */
> + mod->klp_alive = false;
> +
Any reason why this needs to be protected by the 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))
> diff --git a/kernel/module.c b/kernel/module.c
> index d856e96a3cce..b3ffc231ce0d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> }
> #endif
>
> +#ifdef CONFIG_LIVEPATCH
> + mod->klp_alive = false;
> +#endif
> +
I don't think you need this initialization. It looks like the module
struct is embedded in the mod->module_core region which is initialized
to zero in move_module().
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
