From 439e7271dc2b63de379e37971dc2f64d71e24f8a Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Thu, 31 Aug 2017 16:37:41 -0400 Subject: livepatch: introduce shadow variable API Add exported API for livepatch modules: klp_shadow_get() klp_shadow_alloc() klp_shadow_get_or_alloc() klp_shadow_free() klp_shadow_free_all() that implement "shadow" variables, which allow callers to associate new shadow fields to existing data structures. This is intended to be used by livepatch modules seeking to emulate additions to data structure definitions. See Documentation/livepatch/shadow-vars.txt for a summary of the new shadow variable API, including a few common use cases. See samples/livepatch/livepatch-shadow-* for example modules that demonstrate shadow variables. [jkosina@suse.cz: fix __klp_shadow_get_or_alloc() comment as spotted by Josh] Signed-off-by: Joe Lawrence Acked-by: Josh Poimboeuf Acked-by: Miroslav Benes Signed-off-by: Jiri Kosina --- kernel/livepatch/Makefile | 2 +- kernel/livepatch/shadow.c | 277 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 kernel/livepatch/shadow.c (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile index 2b8bdb1925da..b36ceda6488e 100644 --- a/kernel/livepatch/Makefile +++ b/kernel/livepatch/Makefile @@ -1,3 +1,3 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o -livepatch-objs := core.o patch.o transition.o +livepatch-objs := core.o patch.o shadow.o transition.o diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c new file mode 100644 index 000000000000..67e4360521f3 --- /dev/null +++ b/kernel/livepatch/shadow.c @@ -0,0 +1,277 @@ +/* + * shadow.c - Shadow Variables + * + * Copyright (C) 2014 Josh Poimboeuf + * Copyright (C) 2014 Seth Jennings + * Copyright (C) 2017 Joe Lawrence + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see . + */ + +/** + * DOC: Shadow variable API concurrency notes: + * + * The shadow variable API provides a simple relationship between an + * pair and a pointer value. It is the responsibility of the + * caller to provide any mutual exclusion required of the shadow data. + * + * Once a shadow variable is attached to its parent object via the + * klp_shadow_*alloc() API calls, it is considered live: any subsequent + * call to klp_shadow_get() may then return the shadow variable's data + * pointer. Callers of klp_shadow_*alloc() should prepare shadow data + * accordingly. + * + * The klp_shadow_*alloc() API calls may allocate memory for new shadow + * variable structures. Their implementation does not call kmalloc + * inside any spinlocks, but API callers should pass GFP flags according + * to their specific needs. + * + * The klp_shadow_hash is an RCU-enabled hashtable and is safe against + * concurrent klp_shadow_free() and klp_shadow_get() operations. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include +#include +#include + +static DEFINE_HASHTABLE(klp_shadow_hash, 12); + +/* + * klp_shadow_lock provides exclusive access to the klp_shadow_hash and + * the shadow variables it references. + */ +static DEFINE_SPINLOCK(klp_shadow_lock); + +/** + * struct klp_shadow - shadow variable structure + * @node: klp_shadow_hash hash table node + * @rcu_head: RCU is used to safely free this structure + * @obj: pointer to parent object + * @id: data identifier + * @data: data area + */ +struct klp_shadow { + struct hlist_node node; + struct rcu_head rcu_head; + void *obj; + unsigned long id; + char data[]; +}; + +/** + * klp_shadow_match() - verify a shadow variable matches given + * @shadow: shadow variable to match + * @obj: pointer to parent object + * @id: data identifier + * + * Return: true if the shadow variable matches. + */ +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj, + unsigned long id) +{ + return shadow->obj == obj && shadow->id == id; +} + +/** + * klp_shadow_get() - retrieve a shadow variable data pointer + * @obj: pointer to parent object + * @id: data identifier + * + * Return: the shadow variable data element, NULL on failure. + */ +void *klp_shadow_get(void *obj, unsigned long id) +{ + struct klp_shadow *shadow; + + rcu_read_lock(); + + hash_for_each_possible_rcu(klp_shadow_hash, shadow, node, + (unsigned long)obj) { + + if (klp_shadow_match(shadow, obj, id)) { + rcu_read_unlock(); + return shadow->data; + } + } + + rcu_read_unlock(); + + return NULL; +} +EXPORT_SYMBOL_GPL(klp_shadow_get); + +void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data, + size_t size, gfp_t gfp_flags, bool warn_on_exist) +{ + struct klp_shadow *new_shadow; + void *shadow_data; + unsigned long flags; + + /* Check if the shadow variable already exists */ + shadow_data = klp_shadow_get(obj, id); + if (shadow_data) + goto exists; + + /* Allocate a new shadow variable for use inside the lock below */ + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags); + if (!new_shadow) + return NULL; + + new_shadow->obj = obj; + new_shadow->id = id; + + /* Initialize the shadow variable if data provided */ + if (data) + memcpy(new_shadow->data, data, size); + + /* Look for again under the lock */ + spin_lock_irqsave(&klp_shadow_lock, flags); + shadow_data = klp_shadow_get(obj, id); + if (unlikely(shadow_data)) { + /* + * Shadow variable was found, throw away speculative + * allocation. + */ + spin_unlock_irqrestore(&klp_shadow_lock, flags); + kfree(new_shadow); + goto exists; + } + + /* No found, so attach the newly allocated one */ + hash_add_rcu(klp_shadow_hash, &new_shadow->node, + (unsigned long)new_shadow->obj); + spin_unlock_irqrestore(&klp_shadow_lock, flags); + + return new_shadow->data; + +exists: + if (warn_on_exist) { + WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id); + return NULL; + } + + return shadow_data; +} + +/** + * klp_shadow_alloc() - allocate and add a new shadow variable + * @obj: pointer to parent object + * @id: data identifier + * @data: pointer to data to attach to parent + * @size: size of attached data + * @gfp_flags: GFP mask for allocation + * + * Allocates @size bytes for new shadow variable data using @gfp_flags + * and copies @size bytes from @data into the new shadow variable's own + * data space. If @data is NULL, @size bytes are still allocated, but + * no copy is performed. The new shadow variable is then added to the + * global hashtable. + * + * If an existing shadow variable can be found, this routine + * will issue a WARN, exit early and return NULL. + * + * Return: the shadow variable data element, NULL on duplicate or + * failure. + */ +void *klp_shadow_alloc(void *obj, unsigned long id, void *data, + size_t size, gfp_t gfp_flags) +{ + return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, true); +} +EXPORT_SYMBOL_GPL(klp_shadow_alloc); + +/** + * klp_shadow_get_or_alloc() - get existing or allocate a new shadow variable + * @obj: pointer to parent object + * @id: data identifier + * @data: pointer to data to attach to parent + * @size: size of attached data + * @gfp_flags: GFP mask for allocation + * + * Returns a pointer to existing shadow data if an shadow + * variable is already present. Otherwise, it creates a new shadow + * variable like klp_shadow_alloc(). + * + * This function guarantees that only one shadow variable exists with + * the given @id for the given @obj. It also guarantees that the shadow + * variable will be initialized by the given @data only when it did not + * exist before. + * + * Return: the shadow variable data element, NULL on failure. + */ +void *klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data, + size_t size, gfp_t gfp_flags) +{ + return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, false); +} +EXPORT_SYMBOL_GPL(klp_shadow_get_or_alloc); + +/** + * klp_shadow_free() - detach and free a shadow variable + * @obj: pointer to parent object + * @id: data identifier + * + * This function releases the memory for this shadow variable + * instance, callers should stop referencing it accordingly. + */ +void klp_shadow_free(void *obj, unsigned long id) +{ + struct klp_shadow *shadow; + unsigned long flags; + + spin_lock_irqsave(&klp_shadow_lock, flags); + + /* Delete from hash */ + hash_for_each_possible(klp_shadow_hash, shadow, node, + (unsigned long)obj) { + + if (klp_shadow_match(shadow, obj, id)) { + hash_del_rcu(&shadow->node); + kfree_rcu(shadow, rcu_head); + break; + } + } + + spin_unlock_irqrestore(&klp_shadow_lock, flags); +} +EXPORT_SYMBOL_GPL(klp_shadow_free); + +/** + * klp_shadow_free_all() - detach and free all <*, id> shadow variables + * @id: data identifier + * + * This function releases the memory for all <*, id> shadow variable + * instances, callers should stop referencing them accordingly. + */ +void klp_shadow_free_all(unsigned long id) +{ + struct klp_shadow *shadow; + unsigned long flags; + int i; + + spin_lock_irqsave(&klp_shadow_lock, flags); + + /* Delete all <*, id> from hash */ + hash_for_each(klp_shadow_hash, i, shadow, node) { + if (klp_shadow_match(shadow, shadow->obj, id)) { + hash_del_rcu(&shadow->node); + kfree_rcu(shadow, rcu_head); + } + } + + spin_unlock_irqrestore(&klp_shadow_lock, flags); +} +EXPORT_SYMBOL_GPL(klp_shadow_free_all); -- cgit From 5d9da759f7587c87252ef98e70bc0b4a89e4d036 Mon Sep 17 00:00:00 2001 From: Jiri Kosina Date: Thu, 14 Sep 2017 14:15:36 -0700 Subject: livepatch: __klp_shadow_get_or_alloc() is local to shadow.c ... therefore make it static. Fixes: 439e7271dc2 ("livepatch: introduce shadow variable API") Acked-by: Joe Lawrence Signed-off-by: Jiri Kosina --- kernel/livepatch/shadow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c index 67e4360521f3..fdac27588d60 100644 --- a/kernel/livepatch/shadow.c +++ b/kernel/livepatch/shadow.c @@ -113,7 +113,7 @@ void *klp_shadow_get(void *obj, unsigned long id) } EXPORT_SYMBOL_GPL(klp_shadow_get); -void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data, +static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data, size_t size, gfp_t gfp_flags, bool warn_on_exist) { struct klp_shadow *new_shadow; -- cgit From 93862e385ded7c60351e09fcd2a541d273650905 Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Fri, 13 Oct 2017 15:08:41 -0400 Subject: livepatch: add (un)patch callbacks Provide livepatch modules a klp_object (un)patching notification mechanism. Pre and post-(un)patch callbacks allow livepatch modules to setup or synchronize changes that would be difficult to support in only patched-or-unpatched code contexts. Callbacks can be registered for target module or vmlinux klp_objects, but each implementation is klp_object specific. - Pre-(un)patch callbacks run before any (un)patching transition starts. - Post-(un)patch callbacks run once an object has been (un)patched and the klp_patch fully transitioned to its target state. Example use cases include modification of global data and registration of newly available services/handlers. See Documentation/livepatch/callbacks.txt for details and samples/livepatch/ for examples. Signed-off-by: Joe Lawrence Acked-by: Josh Poimboeuf Acked-by: Miroslav Benes Signed-off-by: Jiri Kosina --- kernel/livepatch/core.c | 51 ++++++++++++++++++++++++++++++++++--------- kernel/livepatch/core.h | 38 ++++++++++++++++++++++++++++++++ kernel/livepatch/patch.c | 1 + kernel/livepatch/transition.c | 21 +++++++++++++++--- 4 files changed, 98 insertions(+), 13 deletions(-) (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index b9628e43c78f..cafb5a84417d 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj) return obj->name; } -static bool klp_is_object_loaded(struct klp_object *obj) -{ - return !obj->name || obj->mod; -} - /* sets obj->mod if object is not vmlinux and module is found */ static void klp_find_object_module(struct klp_object *obj) { @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod, static int __klp_disable_patch(struct klp_patch *patch) { + struct klp_object *obj; + if (klp_transition_patch) return -EBUSY; @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch) klp_init_transition(patch, KLP_UNPATCHED); + klp_for_each_object(patch, obj) + if (patch->enabled && obj->patched) + klp_pre_unpatch_callback(obj); + /* * Enforce the order of the func->transition writes in * klp_init_transition() and the TIF_PATCH_PENDING writes in @@ -388,13 +389,18 @@ static int __klp_enable_patch(struct klp_patch *patch) if (!klp_is_object_loaded(obj)) continue; - ret = klp_patch_object(obj); + ret = klp_pre_patch_callback(obj); if (ret) { - pr_warn("failed to enable patch '%s'\n", - patch->mod->name); + pr_warn("pre-patch callback failed for object '%s'\n", + klp_is_module(obj) ? obj->name : "vmlinux"); + goto err; + } - klp_cancel_transition(); - return ret; + ret = klp_patch_object(obj); + if (ret) { + pr_warn("failed to patch object '%s'\n", + klp_is_module(obj) ? obj->name : "vmlinux"); + goto err; } } @@ -403,6 +409,11 @@ static int __klp_enable_patch(struct klp_patch *patch) patch->enabled = true; return 0; +err: + pr_warn("failed to enable patch '%s'\n", patch->mod->name); + + klp_cancel_transition(); + return ret; } /** @@ -871,13 +882,27 @@ int klp_module_coming(struct module *mod) pr_notice("applying patch '%s' to loading module '%s'\n", patch->mod->name, obj->mod->name); + ret = klp_pre_patch_callback(obj); + if (ret) { + pr_warn("pre-patch callback failed for object '%s'\n", + obj->name); + goto err; + } + ret = klp_patch_object(obj); if (ret) { pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", patch->mod->name, obj->mod->name, ret); + + if (patch != klp_transition_patch) + klp_post_unpatch_callback(obj); + goto err; } + if (patch != klp_transition_patch) + klp_post_patch_callback(obj); + break; } } @@ -927,9 +952,15 @@ void klp_module_going(struct module *mod) * is in transition. */ if (patch->enabled || patch == klp_transition_patch) { + + if (patch != klp_transition_patch) + klp_pre_unpatch_callback(obj); + pr_notice("reverting patch '%s' on unloading module '%s'\n", patch->mod->name, obj->mod->name); klp_unpatch_object(obj); + + klp_post_unpatch_callback(obj); } klp_free_object_loaded(obj); diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h index c74f24c47837..6fc907b54e71 100644 --- a/kernel/livepatch/core.h +++ b/kernel/livepatch/core.h @@ -1,6 +1,44 @@ #ifndef _LIVEPATCH_CORE_H #define _LIVEPATCH_CORE_H +#include + extern struct mutex klp_mutex; +static inline bool klp_is_object_loaded(struct klp_object *obj) +{ + return !obj->name || obj->mod; +} + +static inline int klp_pre_patch_callback(struct klp_object *obj) +{ + int ret; + + ret = (obj->callbacks.pre_patch) ? + (*obj->callbacks.pre_patch)(obj) : 0; + + obj->callbacks.post_unpatch_enabled = !ret; + + return ret; +} + +static inline void klp_post_patch_callback(struct klp_object *obj) +{ + if (obj->callbacks.post_patch) + (*obj->callbacks.post_patch)(obj); +} + +static inline void klp_pre_unpatch_callback(struct klp_object *obj) +{ + if (obj->callbacks.pre_unpatch) + (*obj->callbacks.pre_unpatch)(obj); +} + +static inline void klp_post_unpatch_callback(struct klp_object *obj) +{ + if (obj->callbacks.post_unpatch_enabled && + obj->callbacks.post_unpatch) + (*obj->callbacks.post_unpatch)(obj); +} + #endif /* _LIVEPATCH_CORE_H */ diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c index 52c4e907c14b..82d584225dc6 100644 --- a/kernel/livepatch/patch.c +++ b/kernel/livepatch/patch.c @@ -28,6 +28,7 @@ #include #include #include +#include "core.h" #include "patch.h" #include "transition.h" diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index b004a1fb6032..7bf55b7f3687 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -109,9 +109,6 @@ static void klp_complete_transition(void) } } - if (klp_target_state == KLP_UNPATCHED && !immediate_func) - module_put(klp_transition_patch->mod); - /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */ if (klp_target_state == KLP_PATCHED) klp_synchronize_transition(); @@ -130,6 +127,24 @@ static void klp_complete_transition(void) } done: + klp_for_each_object(klp_transition_patch, obj) { + if (!klp_is_object_loaded(obj)) + continue; + if (klp_target_state == KLP_PATCHED) + klp_post_patch_callback(obj); + else if (klp_target_state == KLP_UNPATCHED) + klp_post_unpatch_callback(obj); + } + + /* + * See complementary comment in __klp_enable_patch() for why we + * keep the module reference for immediate patches. + */ + if (!klp_transition_patch->immediate && !immediate_func && + klp_target_state == KLP_UNPATCHED) { + module_put(klp_transition_patch->mod); + } + klp_target_state = KLP_UNDEFINED; klp_transition_patch = NULL; } -- cgit From 6116c3033a761611b1da980ea664c6ddff3eaed6 Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Fri, 13 Oct 2017 15:08:42 -0400 Subject: livepatch: move transition "complete" notice into klp_complete_transition() klp_complete_transition() performs a bit of housework before a transition to KLP_PATCHED or KLP_UNPATCHED is actually completed (including post-(un)patch callbacks). To be consistent, move the transition "complete" kernel log notice out of klp_try_complete_transition() and into klp_complete_transition(). Suggested-by: Josh Poimboeuf Acked-by: Josh Poimboeuf Signed-off-by: Joe Lawrence Acked-by: Miroslav Benes Reviewed-by: Petr Mladek Signed-off-by: Jiri Kosina --- kernel/livepatch/transition.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 7bf55b7f3687..53887f0bca10 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -136,6 +136,9 @@ done: klp_post_unpatch_callback(obj); } + pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name, + klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); + /* * See complementary comment in __klp_enable_patch() for why we * keep the module reference for immediate patches. @@ -423,9 +426,6 @@ void klp_try_complete_transition(void) } success: - pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name, - klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); - /* we're done, now cleanup the data structures */ klp_complete_transition(); } -- cgit From af026796054fb70439e919a925615e61b500ef6b Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Fri, 13 Oct 2017 15:08:43 -0400 Subject: livepatch: add transition notices Log a few kernel debug messages at the beginning of the following livepatch transition functions: klp_complete_transition() klp_cancel_transition() klp_init_transition() klp_reverse_transition() Also update the log notice message in klp_start_transition() for similar verbiage as the above messages. Suggested-by: Josh Poimboeuf Signed-off-by: Joe Lawrence Acked-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- kernel/livepatch/transition.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 53887f0bca10..56add6327736 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -82,6 +82,10 @@ static void klp_complete_transition(void) unsigned int cpu; bool immediate_func = false; + pr_debug("'%s': completing %s transition\n", + klp_transition_patch->mod->name, + klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); + if (klp_target_state == KLP_UNPATCHED) { /* * All tasks have transitioned to KLP_UNPATCHED so we can now @@ -163,6 +167,9 @@ void klp_cancel_transition(void) if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED)) return; + pr_debug("'%s': canceling patching transition, going to unpatch\n", + klp_transition_patch->mod->name); + klp_target_state = KLP_UNPATCHED; klp_complete_transition(); } @@ -441,7 +448,8 @@ void klp_start_transition(void) WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED); - pr_notice("'%s': %s...\n", klp_transition_patch->mod->name, + pr_notice("'%s': starting %s transition\n", + klp_transition_patch->mod->name, klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); /* @@ -497,6 +505,9 @@ void klp_init_transition(struct klp_patch *patch, int state) */ klp_target_state = state; + pr_debug("'%s': initializing %s transition\n", patch->mod->name, + klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); + /* * If the patch can be applied or reverted immediately, skip the * per-task transitions. @@ -562,6 +573,11 @@ void klp_reverse_transition(void) unsigned int cpu; struct task_struct *g, *task; + pr_debug("'%s': reversing transition from %s\n", + klp_transition_patch->mod->name, + klp_target_state == KLP_PATCHED ? "patching to unpatching" : + "unpatching to patching"); + klp_transition_patch->enabled = !klp_transition_patch->enabled; klp_target_state = !klp_target_state; -- cgit From 5aaf1ab55389aeb6ce5527580a1a4d4dbc0f41ff Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Fri, 20 Oct 2017 16:56:50 +0200 Subject: livepatch: Correctly call klp_post_unpatch_callback() in error paths The post_unpatch_enabled flag in struct klp_callbacks is set when a pre-patch callback successfully executes, indicating that we need to call a corresponding post-unpatch callback when the patch is reverted. This is true for ordinary patch disable as well as the error paths of klp_patch_object() callers. As currently coded, we inadvertently execute the post-patch callback twice in klp_module_coming() when klp_patch_object() fails: - We explicitly call klp_post_unpatch_callback() for the failed object - We call it again for the same object (and all the others) via klp_cleanup_module_patches_limited() We should clear the flag in klp_post_unpatch_callback() to make sure that the callback is not called twice. It makes the API more safe. (We could have removed the callback from the former error path as it would be covered by the latter call, but I think that is is cleaner to clear the post_unpatch_enabled after its invoked. For example, someone might later decide to call the callback only when obj->patched flag is set.) There is another mistake in the error path of klp_coming_module() in which it skips the post-unpatch callback for the klp_transition_patch. However, the pre-patch callback was called even for this patch, so be sure to make the corresponding callbacks for all patches. Finally, I used this opportunity to make klp_pre_patch_callback() more readable. [jkosina@suse.cz: incorporate changelog wording changes proposed by Joe Lawrence] Signed-off-by: Petr Mladek Acked-by: Joe Lawrence Signed-off-by: Jiri Kosina --- kernel/livepatch/core.c | 4 +--- kernel/livepatch/core.h | 8 +++++--- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index cafb5a84417d..eb134479c394 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -894,9 +894,7 @@ int klp_module_coming(struct module *mod) pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", patch->mod->name, obj->mod->name, ret); - if (patch != klp_transition_patch) - klp_post_unpatch_callback(obj); - + klp_post_unpatch_callback(obj); goto err; } diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h index 6fc907b54e71..cc3aa708e0b4 100644 --- a/kernel/livepatch/core.h +++ b/kernel/livepatch/core.h @@ -12,10 +12,10 @@ static inline bool klp_is_object_loaded(struct klp_object *obj) static inline int klp_pre_patch_callback(struct klp_object *obj) { - int ret; + int ret = 0; - ret = (obj->callbacks.pre_patch) ? - (*obj->callbacks.pre_patch)(obj) : 0; + if (obj->callbacks.pre_patch) + ret = (*obj->callbacks.pre_patch)(obj); obj->callbacks.post_unpatch_enabled = !ret; @@ -39,6 +39,8 @@ static inline void klp_post_unpatch_callback(struct klp_object *obj) if (obj->callbacks.post_unpatch_enabled && obj->callbacks.post_unpatch) (*obj->callbacks.post_unpatch)(obj); + + obj->callbacks.post_unpatch_enabled = false; } #endif /* _LIVEPATCH_CORE_H */ -- cgit From 89a9a1c1c89cea5f70975c338c011b9f7024dee5 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Fri, 20 Oct 2017 16:56:51 +0200 Subject: livepatch: __klp_disable_patch() should never be called for disabled patches __klp_disable_patch() should never be called when the patch is not enabled. Let's add the same warning that we have in __klp_enable_patch(). This allows to remove the check when calling klp_pre_unpatch_callback(). It was strange anyway because it repeatedly checked per-patch flag for each patched object. Signed-off-by: Petr Mladek Acked-by: Joe Lawrence Signed-off-by: Jiri Kosina --- kernel/livepatch/core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'kernel/livepatch') diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index eb134479c394..287f71e9dbfe 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -282,6 +282,9 @@ static int __klp_disable_patch(struct klp_patch *patch) { struct klp_object *obj; + if (WARN_ON(!patch->enabled)) + return -EINVAL; + if (klp_transition_patch) return -EBUSY; @@ -293,7 +296,7 @@ static int __klp_disable_patch(struct klp_patch *patch) klp_init_transition(patch, KLP_UNPATCHED); klp_for_each_object(patch, obj) - if (patch->enabled && obj->patched) + if (obj->patched) klp_pre_unpatch_callback(obj); /* -- cgit