From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752470AbcBBBST (ORCPT ); Mon, 1 Feb 2016 20:18:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49809 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751734AbcBBBSQ (ORCPT ); Mon, 1 Feb 2016 20:18:16 -0500 From: Jessica Yu To: Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , Rusty Russell , Steven Rostedt , Ingo Molnar Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu Subject: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Date: Mon, 1 Feb 2016 20:17:36 -0500 Message-Id: <1454375856-27757-3-git-send-email-jeyu@redhat.com> In-Reply-To: <1454375856-27757-1-git-send-email-jeyu@redhat.com> References: <1454375856-27757-1-git-send-email-jeyu@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 --- 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. * 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; - 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); + 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 #include #include +#include #include #include #include @@ -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; + blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); return 0; @@ -3531,6 +3538,7 @@ static int load_module(struct load_info *info, const char __user *uargs, blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_disable(mod); /* we can't deallocate the module until we clear memory protection */ module_disable_ro(mod); -- 2.4.3