From b24b78a113164a84bf242b08ef2165365445ea2b Mon Sep 17 00:00:00 2001 From: Miroslav Benes Date: Fri, 4 Mar 2016 10:53:39 +0100 Subject: [PATCH 1/6] klp: remove superfluous errors in asm/livepatch.h There is an #error in asm/livepatch.h for both x86 and s390 in !CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by Michael Ellerman. One can happily include asm/livepatch.h with CONFIG_LIVEPATCH. Remove it as useless. Suggested-by: Michael Ellerman Signed-off-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- arch/s390/include/asm/livepatch.h | 2 -- arch/x86/include/asm/livepatch.h | 2 -- 2 files changed, 4 deletions(-) diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h index a52b6cca873d..105074582d86 100644 --- a/arch/s390/include/asm/livepatch.h +++ b/arch/s390/include/asm/livepatch.h @@ -36,8 +36,6 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) { regs->psw.addr = ip; } -#else -#error Include linux/livepatch.h, not asm/livepatch.h #endif #endif diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h index e795f5274217..8acfe798625b 100644 --- a/arch/x86/include/asm/livepatch.h +++ b/arch/x86/include/asm/livepatch.h @@ -40,8 +40,6 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) { regs->ip = ip; } -#else -#error Include linux/livepatch.h, not asm/livepatch.h #endif #endif /* _ASM_X86_LIVEPATCH_H */ From 335e073faacc9d90bee1527fa2550d6df045df63 Mon Sep 17 00:00:00 2001 From: Jiri Kosina Date: Sun, 6 Mar 2016 22:20:25 +0100 Subject: [PATCH 2/6] klp: remove CONFIG_LIVEPATCH dependency from klp headers There is no need for livepatch.h (generic and arch-specific) to depend on CONFIG_LIVEPATCH. Remove that superfluous dependency. Reported-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- arch/s390/include/asm/livepatch.h | 2 -- arch/x86/include/asm/livepatch.h | 2 -- include/linux/livepatch.h | 4 ---- 3 files changed, 8 deletions(-) diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h index 105074582d86..d5427c78b1b3 100644 --- a/arch/s390/include/asm/livepatch.h +++ b/arch/s390/include/asm/livepatch.h @@ -19,7 +19,6 @@ #include -#ifdef CONFIG_LIVEPATCH static inline int klp_check_compiler_support(void) { return 0; @@ -36,6 +35,5 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) { regs->psw.addr = ip; } -#endif #endif diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h index 8acfe798625b..7e68f9558552 100644 --- a/arch/x86/include/asm/livepatch.h +++ b/arch/x86/include/asm/livepatch.h @@ -25,7 +25,6 @@ #include #include -#ifdef CONFIG_LIVEPATCH static inline int klp_check_compiler_support(void) { #ifndef CC_USING_FENTRY @@ -40,6 +39,5 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) { regs->ip = ip; } -#endif #endif /* _ASM_X86_LIVEPATCH_H */ diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index a8828652f794..c05679701e10 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -24,8 +24,6 @@ #include #include -#if IS_ENABLED(CONFIG_LIVEPATCH) - #include enum klp_state { @@ -134,6 +132,4 @@ int klp_unregister_patch(struct klp_patch *); int klp_enable_patch(struct klp_patch *); int klp_disable_patch(struct klp_patch *); -#endif /* CONFIG_LIVEPATCH */ - #endif /* _LINUX_LIVEPATCH_H_ */ From f995b5f720a72dfe7a1b33a43f2841b4e72d53b7 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 9 Mar 2016 15:20:59 +0100 Subject: [PATCH 3/6] livepatch: Fix the error message about unresolvable ambiguity klp_find_callback() stops the search when sympos is not defined and a second symbol of the same name is found. It means that the current error message about the unresolvable ambiguity always prints "(2 matches)". Let's remove this information. The total number of occurrences is not much helpful. The author of the patch still must put a non-trivial effort into searching the right position in the object file. [jkosina@suse.cz: fixed grammar as suggested by Josh] Signed-off-by: Petr Mladek Acked-by: Josh Poimboeuf Acked-by: Chris J Arges Signed-off-by: Jiri Kosina --- kernel/livepatch/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index bc2c85c064c1..780f00cdb7e5 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -190,8 +190,8 @@ static int klp_find_object_symbol(const char *objname, const char *name, if (args.addr == 0) pr_err("symbol '%s' not found in symbol table\n", name); else if (args.count > 1 && sympos == 0) { - pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n", - args.count, name, objname); + pr_err("unresolvable ambiguity for symbol '%s' in object '%s'\n", + name, objname); } else if (sympos != args.count && sympos > 0) { pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n", sympos, name, objname ? objname : "vmlinux"); From 06e1c170c332dca755f389a99671912ff405698c Mon Sep 17 00:00:00 2001 From: Josh Poimboeuf Date: Wed, 16 Mar 2016 10:03:36 -0500 Subject: [PATCH 4/6] livepatch: Update maintainers Seth and Vojtech are no longer active maintainers of livepatch, so remove them in favor of Jessica and Miroslav. Also add Petr as a designated reviewer. [jikos@kernel.org: Petr is the only one affected who hasn't provided his Ack by the time this patch has been applied, as he is offline curently; but I hereby assert that I've talked to him and he's OK with this change] Signed-off-by: Josh Poimboeuf Acked-by: Seth Jennings Acked-by: Jessica Yu Acked-by: Miroslav Benes Acked-by: Vojtech Pavlik Signed-off-by: Jiri Kosina --- MAINTAINERS | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 28eb61bbecf4..62e1b1d3ea9d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6561,9 +6561,10 @@ F: drivers/platform/x86/hp_accel.c LIVE PATCHING M: Josh Poimboeuf -M: Seth Jennings +M: Jessica Yu M: Jiri Kosina -M: Vojtech Pavlik +M: Miroslav Benes +R: Petr Mladek S: Maintained F: kernel/livepatch/ F: include/linux/livepatch.h From 4c973d1620ae08f5cbe27644c5f5b974c8f594ec Mon Sep 17 00:00:00 2001 From: Jessica Yu Date: Wed, 16 Mar 2016 20:55:38 -0400 Subject: [PATCH 5/6] modules: split part of complete_formation() into prepare_coming_module() Put all actions in complete_formation() that are performed after module->state is set to MODULE_STATE_COMING into a separate function prepare_coming_module(). This split prepares for the removal of the livepatch module notifiers in favor of hard-coding function calls to klp_module_{coming,going} in the module loader. The complete_formation -> prepare_coming_module split will also make error handling easier since we can jump to the appropriate error label to do any module GOING cleanup after all the COMING-actions have completed. Signed-off-by: Jessica Yu Reviewed-by: Josh Poimboeuf Reviewed-by: Petr Mladek Acked-by: Rusty Russell Signed-off-by: Jiri Kosina --- kernel/module.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 794ebe8e878d..6dbfad415d51 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3392,9 +3392,6 @@ static int complete_formation(struct module *mod, struct load_info *info) mod->state = MODULE_STATE_COMING; mutex_unlock(&module_mutex); - ftrace_module_enable(mod); - blocking_notifier_call_chain(&module_notify_list, - MODULE_STATE_COMING, mod); return 0; out: @@ -3402,6 +3399,14 @@ out: return err; } +static int prepare_coming_module(struct module *mod) +{ + ftrace_module_enable(mod); + blocking_notifier_call_chain(&module_notify_list, + MODULE_STATE_COMING, mod); + return 0; +} + static int unknown_module_param_cb(char *param, char *val, const char *modname, void *arg) { @@ -3516,13 +3521,17 @@ static int load_module(struct load_info *info, const char __user *uargs, if (err) goto ddebug_cleanup; + err = prepare_coming_module(mod); + if (err) + goto bug_cleanup; + /* Module is ready to execute: parsing args may do that. */ after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, -32768, 32767, mod, unknown_module_param_cb); if (IS_ERR(after_dashes)) { err = PTR_ERR(after_dashes); - goto bug_cleanup; + goto coming_cleanup; } else if (after_dashes) { pr_warn("%s: parameters '%s' after `--' ignored\n", mod->name, after_dashes); @@ -3531,7 +3540,7 @@ static int load_module(struct load_info *info, const char __user *uargs, /* Link in to syfs. */ err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp); if (err < 0) - goto bug_cleanup; + goto coming_cleanup; /* Get rid of temporary copy. */ free_copy(info); @@ -3541,15 +3550,16 @@ static int load_module(struct load_info *info, const char __user *uargs, return do_init_module(mod); + coming_cleanup: + blocking_notifier_call_chain(&module_notify_list, + MODULE_STATE_GOING, mod); + bug_cleanup: /* module_bug_cleanup needs module_mutex protection */ mutex_lock(&module_mutex); module_bug_cleanup(mod); mutex_unlock(&module_mutex); - blocking_notifier_call_chain(&module_notify_list, - MODULE_STATE_GOING, mod); - /* we can't deallocate the module until we clear memory protection */ module_disable_ro(mod); module_disable_nx(mod); From 7e545d6eca20ce8ef7f66a63146cbff82b2ba760 Mon Sep 17 00:00:00 2001 From: Jessica Yu Date: Wed, 16 Mar 2016 20:55:39 -0400 Subject: [PATCH 6/6] livepatch/module: remove livepatch module notifier 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. Signed-off-by: Jessica Yu Reviewed-by: Petr Mladek Acked-by: Josh Poimboeuf Acked-by: Rusty Russell Signed-off-by: Jiri Kosina --- include/linux/livepatch.h | 13 +++ kernel/livepatch/core.c | 167 ++++++++++++++++++-------------------- kernel/module.c | 10 +++ 3 files changed, 104 insertions(+), 86 deletions(-) diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index c05679701e10..bd830d590465 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -24,6 +24,8 @@ #include #include +#if IS_ENABLED(CONFIG_LIVEPATCH) + #include enum klp_state { @@ -132,4 +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 780f00cdb7e5..d68fbf63b083 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -99,12 +99,12 @@ static void klp_find_object_module(struct klp_object *obj) /* * We do not want to block removal of patched modules and therefore * we do not take a reference here. The patches are removed by - * a going module handler instead. + * klp_module_going() instead. */ mod = find_module(obj->name); /* - * Do not mess work of the module coming and going notifiers. - * Note that the patch might still be needed before the going handler + * Do not mess work of klp_module_coming() and klp_module_going(). + * Note that the patch might still be needed before klp_module_going() * is called. 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. @@ -866,88 +866,49 @@ 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) -{ - struct module *pmod = patch->mod; - struct module *mod = obj->mod; - int ret; - - 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) - return 0; - - pr_notice("applying patch '%s' to loading module '%s'\n", - pmod->name, mod->name); - - 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; -} - -static void klp_module_notify_going(struct klp_patch *patch, - struct klp_object *obj) -{ - struct module *pmod = patch->mod; - struct module *mod = obj->mod; - - if (patch->state == KLP_DISABLED) - goto disabled; - - pr_notice("reverting patch '%s' on unloading module '%s'\n", - pmod->name, mod->name); - - klp_disable_object(obj); - -disabled: - klp_free_object_loaded(obj); -} - -static int klp_module_notify(struct notifier_block *nb, unsigned long action, - void *data) +int klp_module_coming(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 (WARN_ON(mod->state != MODULE_STATE_COMING)) + return -EINVAL; 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 klp_module_coming() + * 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 = true; 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); + 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; + } + + if (patch->state == KLP_DISABLED) + break; + + pr_notice("applying patch '%s' to loading module '%s'\n", + patch->mod->name, obj->mod->name); + + 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; } @@ -956,12 +917,56 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, mutex_unlock(&klp_mutex); return 0; + +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); + mod->klp_alive = false; + klp_free_object_loaded(obj); + mutex_unlock(&klp_mutex); + + return ret; } -static struct notifier_block klp_module_nb = { - .notifier_call = klp_module_notify, - .priority = INT_MIN+1, /* called late but before ftrace notifier */ -}; +void klp_module_going(struct module *mod) +{ + struct klp_patch *patch; + struct klp_object *obj; + + if (WARN_ON(mod->state != MODULE_STATE_GOING && + mod->state != MODULE_STATE_COMING)) + return; + + mutex_lock(&klp_mutex); + /* + * Each module has to know that klp_module_going() + * has been called. We never know what module will + * get patched by a new patch. + */ + 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 (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); +} static int __init klp_init(void) { @@ -973,21 +978,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 6dbfad415d51..4b65fbb10bdc 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -53,6 +53,7 @@ #include #include #include +#include #include #include #include @@ -984,6 +985,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_going(mod); ftrace_release_mod(mod); async_synchronize_full(); @@ -3315,6 +3317,7 @@ fail: module_put(mod); blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); ftrace_release_mod(mod); free_module(mod); wake_up_all(&module_wq); @@ -3401,7 +3404,13 @@ out: static int prepare_coming_module(struct module *mod) { + int err; + ftrace_module_enable(mod); + err = klp_module_coming(mod); + if (err) + return err; + blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); return 0; @@ -3553,6 +3562,7 @@ static int load_module(struct load_info *info, const char __user *uargs, coming_cleanup: blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod); + klp_module_going(mod); bug_cleanup: /* module_bug_cleanup needs module_mutex protection */