Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier

[Petr Mladek]

On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> Remove the livepatch module notifier in favor of directly enabling and
> disabling patches to modules in the module loader. Hard-coding the
> function calls ensures that ftrace_module_enable() is run before
> klp_module_enable() during module load, and that klp_module_disable() is
> run before ftrace_release_mod() during module unload. This way, ftrace
> and livepatch code is run in the correct order during the module
> load/unload sequence without dependence on the module notifier call chain.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly.
> 
> Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
> ---
>  include/linux/livepatch.h |   9 +++
>  kernel/livepatch/core.c   | 144 ++++++++++++++++++++++------------------------
>  kernel/module.c           |   8 +++
>  3 files changed, 86 insertions(+), 75 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a882865..fdd5f1c 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
>  int klp_disable_patch(struct klp_patch *);
>  
> +/* Called from the module loader during module coming/going states */
> +extern int klp_module_enable(struct module *mod);
> +extern void klp_module_disable(struct module *mod);
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline int klp_module_enable(struct module *mod) { return 0; }
> +static inline void klp_module_disable(struct module *mod) { }
> +
>  #endif /* CONFIG_LIVEPATCH */
>  
>  #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..7aa975d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj)
>  	 */
>  	mod = find_module(obj->name);
>  	/*
> -	 * Do not mess work of the module coming and going notifiers.
> +	 * Do not mess work of the klp module coming and going handlers.

This is a bit confusing because you removed all functions called
*coming* and *going*. I would say something like:

	* Do not mess work of klp_module_enable() and klp_module_disable().


>  	 * 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
> @@ -866,103 +866,107 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static int klp_module_notify_coming(struct klp_patch *patch,
> -				     struct klp_object *obj)
> +/* Called when module state is MODULE_STATE_COMING */
> +int klp_module_enable(struct module *mod)
>  {
> -	struct module *pmod = patch->mod;
> -	struct module *mod = obj->mod;
>  	int ret;
> +	struct klp_patch *patch;
> +	struct klp_object *obj;
>  
> -	ret = klp_init_object_loaded(patch, obj);
> -	if (ret) {
> -		pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> -			pmod->name, mod->name, ret);
> -		return ret;
> -	}
> -
> -	if (patch->state == KLP_DISABLED)
> +	if (mod->state != MODULE_STATE_COMING)
>  		return 0;

The function is not longer called from another state. I would replace
this by:

	if (WARN_ON(mod->state != MODULE_STATE_COMING))
		return -EINVAL;


> -	pr_notice("applying patch '%s' to loading module '%s'\n",
> -		  pmod->name, mod->name);
> +	mutex_lock(&klp_mutex);
> +	/*
> +	 * Each module has to know that the coming handler has
> +	 * been called. We never know what module will get
> +	 * patched by a new patch.
> +	 */
> +	mod->klp_alive = true;
>  
> -	ret = klp_enable_object(obj);
> -	if (ret)
> -		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> -			pmod->name, mod->name, ret);
> -	return ret;
> -}
> +	list_for_each_entry(patch, &klp_patches, list) {
> +		klp_for_each_object(patch, obj) {
> +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +				continue;
>  
> -static void klp_module_notify_going(struct klp_patch *patch,
> -				    struct klp_object *obj)
> -{
> -	struct module *pmod = patch->mod;
> -	struct module *mod = obj->mod;
> +			obj->mod = mod;
>  
> -	if (patch->state == KLP_DISABLED)
> -		goto disabled;
> +			ret = klp_init_object_loaded(patch, obj);
> +			if (ret) {
> +				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> +					patch->mod->name, obj->mod->name, ret);
> +				goto err;
> +			}
> +
> +			if (patch->state == KLP_DISABLED)
> +				break;
>  
> -	pr_notice("reverting patch '%s' on unloading module '%s'\n",
> -		  pmod->name, mod->name);
> +			pr_notice("applying patch '%s' to loading module '%s'\n",
> +				  patch->mod->name, obj->mod->name);
>  
> -	klp_disable_object(obj);
> +			ret = klp_enable_object(obj);
> +			if (ret) {
> +				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> +					patch->mod->name, obj->mod->name, ret);
> +				goto err;
> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&klp_mutex);
>  
> -disabled:
> -	klp_free_object_loaded(obj);
> +	return 0;
> +
> +err:
> +	/*
> +	 * If a patch is unsuccessfully applied, return
> +	 * error to the module loader.
> +	 */
> +	obj->mod = NULL;
> +	pr_warn("patch '%s' is in an inconsistent state!\n", patch->mod->name);

This message is not correct. The module will not get loaded
when the patch is not applied.

Instead, we need to revert all the operations that has already
been done for this module. Note that the module stayed loaded
before, so we did not need to release any memory or revert
any ftrace call registration but we need to do so now!


> +	mutex_unlock(&klp_mutex);
> +
> +	return ret;
>  }
>  
> -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> -			     void *data)
> +/* Called when module state is MODULE_STATE_GOING */
> +void klp_module_disable(struct module *mod)
>  {
> -	int ret;
> -	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
>  
> -	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> -		return 0;
> +	if (mod->state != MODULE_STATE_GOING)
> +		return;
>  
>  	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.
> +	 * Each module has to know that the going handler
> +	 * 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;
> +	mod->klp_alive = false;
>  
>  	list_for_each_entry(patch, &klp_patches, list) {
>  		klp_for_each_object(patch, obj) {
>  			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
>  				continue;
>  
> -			if (action == MODULE_STATE_COMING) {
> -				obj->mod = mod;
> -				ret = klp_module_notify_coming(patch, obj);
> -				if (ret) {
> -					obj->mod = NULL;
> -					pr_warn("patch '%s' is in an inconsistent state!\n",
> -						patch->mod->name);
> -				}
> -			} else /* MODULE_STATE_GOING */
> -				klp_module_notify_going(patch, obj);
> +			if (patch->state != KLP_DISABLED) {
> +				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> +					  patch->mod->name, obj->mod->name);
> +				klp_disable_object(obj);
> +			}
>  
> +			klp_free_object_loaded(obj);
>  			break;
>  		}
>  	}
>  
>  	mutex_unlock(&klp_mutex);
> -
> -	return 0;
>  }
>  
> -static struct notifier_block klp_module_nb = {
> -	.notifier_call = klp_module_notify,
> -	.priority = INT_MIN+1, /* called late but before ftrace notifier */
> -};
> -
>  static int __init klp_init(void)
>  {
>  	int ret;
> @@ -973,21 +977,11 @@ static int __init klp_init(void)
>  		return -EINVAL;
>  	}
>  
> -	ret = register_module_notifier(&klp_module_nb);
> -	if (ret)
> -		return ret;
> -
>  	klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj);
> -	if (!klp_root_kobj) {
> -		ret = -ENOMEM;
> -		goto unregister;
> -	}
> +	if (!klp_root_kobj)
> +		return -ENOMEM;
>  
>  	return 0;
> -
> -unregister:
> -	unregister_module_notifier(&klp_module_nb);
> -	return ret;
>  }
>  
>  module_init(klp_init);
> diff --git a/kernel/module.c b/kernel/module.c
> index b05d466..71c77ed 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>
> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		mod->exit();
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	klp_module_disable(mod);
>  	ftrace_release_mod(mod);
>  
>  	async_synchronize_full();
> @@ -3297,6 +3299,7 @@ fail:
>  	module_put(mod);
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	klp_module_disable(mod);
>  	ftrace_release_mod(mod);
>  	free_module(mod);
>  	wake_up_all(&module_wq);
> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mutex_unlock(&module_mutex);
>  
>  	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod);
> +	if (err)
> +		goto out;

If you go out here, you need to revert some some operations
that are normally done in the bug_cleanup: goto target
in load_module(). In particular, you need to do:

	/* module_bug_cleanup needs module_mutex protection */
	mutex_lock(&module_mutex);
	module_bug_cleanup(mod);
	mutex_unlock(&module_mutex);

	ftrace_release_mod(mod);

	/* we can't deallocate the module until we clear memory protection */
	module_disable_ro(mod);
	module_disable_nx(mod);


IMHO, it would make sense to somehow split the complete_formation() function
and avoid a code duplication in the error paths.

Best Regards,
Petr