On Fri 2016-02-05 22:08:17, 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_coming() during module load, and that klp_module_going() 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 <j...@redhat.com>
> ---
> include/linux/livepatch.h | 9 +++
> kernel/livepatch/core.c | 153
> +++++++++++++++++++++++-----------------------
> kernel/module.c | 20 +++++-
> 3 files changed, 103 insertions(+), 79 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a882865..bd830d5 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 */
> +int klp_module_coming(struct module *mod);
> +void klp_module_going(struct module *mod);
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline int klp_module_coming(struct module *mod) { return 0; }
> +static inline void klp_module_going(struct module *mod) { }
> +
> #endif /* CONFIG_LIVEPATCH */
>
> #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..1d47f96 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> +int klp_module_coming(struct module *mod)
[...]
> + obj->mod = mod;
[...]
> + 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;
> + }
[...]
> + 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;
> + }
[...]
> +
> +err:
> + /*
> + * If a patch is unsuccessfully applied, return
> + * error to the module loader.
> + */
> + pr_warn("patch '%s' failed for module '%s', refusing to load module
> '%s'\n",
> + patch->mod->name, obj->mod->name, obj->mod->name);
> + obj->mod = NULL;
To be on the safe side, we should replace this assignment with:
klp_free_object_loaded(mod);
It clears obj->mod and also all func->old_addr for this module.
By other words, it leaves the structures in the same state as
klp_module_going(). I am sorry that I have missed this before.
Otherwise, it looks fine. So, with the above suggested change:
Reviewed-by: Petr Mladek <pmla...@suse.com>
Best Regards,
Petr
