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

[Rusty Russell]

Petr Mladek <pmladek@xxxxxxx> writes:
> 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.

I don't think you should handle going modules at all.  Rarely happens,
and it should happen fast.

If you can hold the module_lock, the easiest thing to do is have us wake
module_wq when a module is freed, then you can just:

        retry:
                err = wait_event_interruptible(module_wq,
                                               !modules_unloading());
                if (err)
                        goto out;

                /* Now re-check under lock. */
                mutex_lock(&module_lock);
                if (unlikely(modules_unloading()) {
                        mutex_unlock(&module_lock);
                        goto retry;
                }

Cheers,
Rusty.





>
> 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@xxxxxxx>
> ---
>  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.
> +	 */
> +	mod = find_module(obj->name);
> +
>  	/*
> -	 * 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 must not init the object when the module is going and the notifier
> +	 * has already been called. But the patch might still be needed before.
> +	 * Note that 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_GOING && mod->klp_gone)
> +		mod = NULL;
> +
> +	obj->mod = mod;
> +
>  	mutex_unlock(&module_mutex);
>  }
>  
> @@ -929,7 +945,10 @@ int klp_module_init(struct module *mod)
>  	int ret = 0;
>  
>  	mutex_lock(&klp_mutex);
> +
> +	mod->klp_gone = false;
>  	ret = klp_module_coming(mod);
> +
>  	mutex_unlock(&klp_mutex);
>  
>  	return ret;
> @@ -985,7 +1004,10 @@ static int klp_module_notify_going(struct notifier_block *nb,
>  		return 0;
>  
>  	mutex_lock(&klp_mutex);
> +
>  	klp_module_going(mod);
> +	mod->klp_gone = true;
> +
>  	mutex_lock(&klp_mutex);
>  
>  	return 0;
> -- 
> 1.8.5.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/