Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Mar 05, 2015 at 04:45:14PM +0100, Petr Mladek wrote:
> Existing live patches are removed from going modules using a notify handler.
> There are two problems with the current implementation.
> 
> First, 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.
> 
> Second, if we start supporting patches with semantic changes between function
> calls, we would need to apply any new patch even for going modules. Note that
> the code from the module could be called even in the GOING state until
> mod->exit() finishes. See below for example.
> 
> This patch solves the problem by adding boolean flag into struct module.
> It is switched when the going module handler is called. It marks the point
> when it is safe and we actually have to ignore the going module.
> 
> Alternative solutions:
> 
> We could add another lock to make the switch to GOING state and mod->exit()
> call an atomic operation. But this a nogo. It might cause a dead lock when
> some mod->exit() depends on mod->exit() from another module.
> 
> We could wait until the GOING module is moved to the UNFORMED state.
> But this might take ages when mod->exit() has to wait for something.
> 
> We could refuse to load the patch when a module is going but this is
> pretty ugly.
> 
> Example of the race:
> 
> 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.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  include/linux/module.h  |  4 ++++
>  kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
>  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..c12f93497b74 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_gone;
> +#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 198f7733604b..0b38357fad0f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,16 +89,32 @@ 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 do not want to block removal of patched modules and therefore
> +	 * we do not take a reference here. Instead, the patches are removed
> +	 * by the going module handler instead.
> +	 */

Redundant "instead".


-- 
Josh