Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713243 > unrolled thread

[PATCH v3] livepatch: add (un)patch callbacks

Started byJoe Lawrence <joe.lawrence@redhat.com>
First post2017-08-16 21:20 +0200
Last post2017-08-22 11:20 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-16 21:20 +0200
    Re: [PATCH v3] livepatch: add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-16 22:30 +0200
    Re: [PATCH v3] livepatch: add (un)patch callbacks Petr Mladek <pmladek@suse.com> - 2017-08-18 16:00 +0200
      Re: [PATCH v3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-21 23:20 +0200
        Re: [PATCH v3] livepatch: add (un)patch callbacks Petr Mladek <pmladek@suse.com> - 2017-08-22 11:20 +0200

#1713243 — [PATCH v3] livepatch: add (un)patch callbacks

FromJoe Lawrence <joe.lawrence@redhat.com>
Date2017-08-16 21:20 +0200
Subject[PATCH v3] livepatch: add (un)patch callbacks
Message-ID<ufc1b-2Al-1@gated-at.bofh.it>
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 action takes
    place.

  - 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/callback.txt for details.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/callbacks.txt        |  87 ++++++++++++
 include/linux/livepatch.h                    |  81 ++++++++++++
 kernel/livepatch/core.c                      |  37 ++++--
 kernel/livepatch/patch.c                     |   5 +-
 kernel/livepatch/transition.c                |  21 ++-
 samples/livepatch/Makefile                   |   2 +
 samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
 samples/livepatch/livepatch-callbacks-mod.c  |  53 ++++++++
 8 files changed, 462 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/livepatch/callbacks.txt
 create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
 create mode 100644 samples/livepatch/livepatch-callbacks-mod.c

diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
new file mode 100644
index 000000000000..e18f2678f3bb
--- /dev/null
+++ b/Documentation/livepatch/callbacks.txt
@@ -0,0 +1,87 @@
+(Un)patching Callbacks
+======================
+
+Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
+to execute callback functions when a kernel object is (un)patched.  They
+can be considered a "power feature" that extends livepatching abilities
+to include:
+
+  - Safe updates to global data
+
+  - "Patches" to init and probe functions
+
+  - Patching otherwise unpatchable code (i.e. assembly)
+
+In most cases, (un)patch hooks will need to be used in conjunction with
+memory barriers and kernel synchronization primitives, like
+mutexes/spinlocks, or even stop_machine(), to avoid concurrency issues.
+
+Callbacks differ from existing kernel facilities:
+
+  - Module init/exit code doesn't run when disabling and re-enabling a
+    patch.
+
+  - A module notifier can't stop the to-be-patched module from loading.
+
+Callbacks are part of the klp_object structure and their implementation
+is specific to the given object.  Other livepatch objects may or may not
+be patched, irrespective of the target klp_object's current state.
+
+Callbacks can be registered for the following livepatch actions:
+
+  * Pre-patch    - before klp_object is patched
+
+  * Post-patch   - after klp_object has been patched and is active
+                   across all tasks
+
+  * Pre-unpatch  - before klp_object is unpatched, patched code is active
+
+  * Post-unpatch - after klp_object has been patched, all code has been
+		   restored and no tasks are running patched code
+
+A callbacks is only executed if its host klp_object is loaded.  For
+in-kernel vmlinux targets, this means that callbacks will always execute
+when a livepatch is enabled/disabled.
+
+For kernel module targets, callbacks will only execute if the target
+module is loaded.  When a kernel module target is (un)loaded, its
+callbacks will execute only if the livepatch module is enabled.
+
+The pre-patch callback is expected to return a status code (0 for
+success, -ERRNO on error).  An error status code will indicate to the
+livepatching core that patching of the current klp_object is not safe
+and to stop the current patching request.  If the problematic klp_object
+is already loaded (i.e. a livepatch is loaded after target code), the
+kernel's module loader will refuse to load the livepatch.  On the other
+hand, if the problematic klp_object is already in place (i.e. a target
+module is loaded after a livepatch), then the module loader will refuse
+to load the target kernel module.
+
+
+Example Use-cases
+-----------------
+
+1 - Update global data
+
+A pre-patch callback can be useful to update a global variable.  For
+example, 75ff39ccc1bd ("tcp: make challenge acks less predictable")
+changes a global sysctl, as well as patches the tcp_send_challenge_ack()
+function.
+
+In this case, if we're being super paranoid, it might make sense to
+patch the data *after* patching is complete with a post-patch callback,
+so that tcp_send_challenge_ack() could first be changed to read
+sysctl_tcp_challenge_ack_limit with READ_ONCE.
+
+
+2 - Support __init and probe function patches
+
+Although __init and probe functions are not directly livepatch-able, it
+may be possible to implement similar updates via pre/post-patch
+callbacks.
+
+48900cb6af42 ("virtio-net: drop NETIF_F_FRAGLIST") change the way that
+virtnet_probe() initialized its driver's net_device features.  A
+pre/post-patch callback could iterate over all such devices, making a
+similar change to their hw_features value.  (Client functions of the
+value may need to be updated accordingly.)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..500dc9b2b361 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -87,10 +87,25 @@ struct klp_func {
 	bool transition;
 };
 
+struct klp_object;
+
+/**
+ * struct klp_callback - callback structure for live patching
+ * @callback:	function to be executed on callback
+ *
+ */
+struct klp_callbacks {
+	int (*pre_patch)(struct klp_object *obj);
+	void (*post_patch)(struct klp_object *obj);
+	void (*pre_unpatch)(struct klp_object *obj);
+	void (*post_unpatch)(struct klp_object *obj);
+};
+
 /**
  * struct klp_object - kernel object structure for live patching
  * @name:	module name (or NULL for vmlinux)
  * @funcs:	function entries for functions to be patched in the object
+ * @callbacks:	functions to be executed pre/post (un)patching
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
@@ -100,6 +115,7 @@ struct klp_object {
 	/* external */
 	const char *name;
 	struct klp_func *funcs;
+	struct klp_callbacks callbacks;
 
 	/* internal */
 	struct kobject kobj;
@@ -138,6 +154,71 @@ struct klp_patch {
 	     func->old_name || func->new_func || func->old_sympos; \
 	     func++)
 
+/**
+ * klp_is_object_loaded() - is klp_object currently loaded?
+ * @obj:	klp_object pointer
+ *
+ * Return: true if klp_object is loaded (always true for vmlinux)
+ */
+static inline bool klp_is_object_loaded(struct klp_object *obj)
+{
+	return !obj->name || obj->mod;
+}
+
+/**
+ * klp_pre_patch_callback - execute before klp_object is patched
+ * @obj:	invoke callback for this klp_object
+ *
+ * Return: status from callback
+ *
+ * Callers should ensure obj->patched is *not* set.
+ */
+static inline int klp_pre_patch_callback(struct klp_object *obj)
+{
+	if (obj->callbacks.pre_patch)
+		return (*obj->callbacks.pre_patch)(obj);
+	return 0;
+}
+
+/**
+ * klp_post_patch_callback() - execute after klp_object is patched
+ * @obj:	invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is set.
+ */
+static inline void klp_post_patch_callback(struct klp_object *obj)
+{
+	if (obj->callbacks.post_patch)
+		(*obj->callbacks.post_patch)(obj);
+}
+
+/**
+ * klp_pre_unpatch_callback() - execute before klp_object is unpatched
+ *                              and is active across all tasks
+ * @obj:	invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is set.
+ */
+static inline void klp_pre_unpatch_callback(struct klp_object *obj)
+{
+	if (obj->callbacks.pre_unpatch)
+		(*obj->callbacks.pre_unpatch)(obj);
+}
+
+/**
+ * klp_post_unpatch_callback() - execute after klp_object is unpatched,
+ *                               all code has been restored and no tasks
+ *                               are running patched code
+ * @obj:	invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is *not* set.
+ */
+static inline void klp_post_unpatch_callback(struct klp_object *obj)
+{
+	if (obj->callbacks.post_unpatch)
+		(*obj->callbacks.post_unpatch)(obj);
+}
+
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..ddb23e18a357 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)
 {
@@ -388,13 +383,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 +403,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,6 +876,13 @@ 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",
@@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
 				goto err;
 			}
 
+			klp_post_patch_callback(obj);
+
 			break;
 		}
 	}
@@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
 			if (patch->enabled || patch == klp_transition_patch) {
 				pr_notice("reverting patch '%s' on unloading module '%s'\n",
 					  patch->mod->name, obj->mod->name);
+
+				klp_pre_unpatch_callback(obj);
 				klp_unpatch_object(obj);
+				klp_post_unpatch_callback(obj);
 			}
 
 			klp_free_object_loaded(obj);
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 52c4e907c14b..0eed0df6e6d9 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
+			klp_pre_unpatch_callback(obj);
 			klp_unpatch_object(obj);
 			return ret;
 		}
@@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
-		if (obj->patched)
+		if (obj->patched) {
+			klp_pre_unpatch_callback(obj);
 			klp_unpatch_object(obj);
+		}
 }
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;
 }
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 10319d7ea0b1..f8e545af5de4 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1 +1,3 @@
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c
new file mode 100644
index 000000000000..a5ae7211f24a
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-callbacks-demo.c - (un)patching callbacks livepatch demo
+ *
+ *
+ * Purpose
+ * -------
+ *
+ * Demonstration of registering livepatch (un)patching callbacks.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * Step 1 - load the simple module
+ *
+ *   insmod samples/livepatch/livepatch-callbacks-mod.ko
+ *
+ *
+ * Step 2 - load the demonstration livepatch (with callbacks)
+ *
+ *   insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ *
+ * Step 3 - cleanup
+ *
+ *   echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ *   rmmod livepatch_callbacks_demo
+ *   rmmod livepatch_callbacks_mod
+ *
+ * Watch dmesg output to see livepatch enablement, callback execution
+ * and patching operations for both vmlinux and module targets.
+ *
+ * NOTE: swap the insmod order of livepatch-callbacks-mod.ko and
+ *       livepatch-callbacks-demo.ko to observe what happens when a
+ *       target module is loaded after a livepatch with callbacks.
+ *
+ * NOTE: 'pre_patch_ret' is a module parameter that sets the pre-patch
+ *       callback return status.  Try setting up a non-zero status
+ *       such as -19 (-ENODEV):
+ *
+ *       # Load demo livepatch, vmlinux is patched
+ *       insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ *       # Setup next pre-patch callback to return -ENODEV
+ *       echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
+ *
+ *       # Module loader refuses to load the target module
+ *       insmod samples/livepatch/livepatch-callbacks-mod.ko
+ *       insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include <linux/stat.h>
+
+static int pre_patch_ret;
+module_param(pre_patch_ret, int, 0644);
+MODULE_PARM_DESC(pre_patch_ret, "pre_patch_ret (default=0)");
+
+const char *module_state[] = {
+	[MODULE_STATE_LIVE]	= "[MODULE_STATE_LIVE] Normal state",
+	[MODULE_STATE_COMING]	= "[MODULE_STATE_COMING] Full formed, running module_init",
+	[MODULE_STATE_GOING]	= "[MODULE_STATE_GOING] Going away",
+	[MODULE_STATE_UNFORMED]	= "[MODULE_STATE_UNFORMED] Still setting it up",
+};
+
+static void callback_info(const char *callback, struct klp_object *obj)
+{
+	if (obj->mod)
+		pr_info("%s: %s -> %s\n", callback, obj->mod->name,
+			module_state[obj->mod->state]);
+	else
+		pr_info("%s: vmlinux\n", callback);
+}
+
+/* Executed on object patching (ie, patch enablement) */
+static int pre_patch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+	return pre_patch_ret;
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void post_patch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void pre_unpatch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void post_unpatch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+}
+
+static struct klp_func funcs[] = {
+	{ }
+};
+
+static struct klp_object objs[] = {
+	{
+		.name = NULL,	/* vmlinux */
+		.funcs = funcs,
+		.callbacks = {
+			.pre_patch = pre_patch_callback,
+			.post_patch = post_patch_callback,
+			.pre_unpatch = pre_unpatch_callback,
+			.post_unpatch = post_unpatch_callback,
+		},
+	},	{
+		.name = "livepatch_callbacks_mod",
+		.funcs = funcs,
+		.callbacks = {
+			.pre_patch = pre_patch_callback,
+			.post_patch = post_patch_callback,
+			.pre_unpatch = pre_unpatch_callback,
+			.post_unpatch = post_unpatch_callback,
+		},
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+static int livepatch_callbacks_demo_init(void)
+{
+	int ret;
+
+	if (!klp_have_reliable_stack() && !patch.immediate) {
+		/*
+		 * WARNING: Be very careful when using 'patch.immediate' in
+		 * your patches.  It's ok to use it for simple patches like
+		 * this, but for more complex patches which change function
+		 * semantics, locking semantics, or data structures, it may not
+		 * be safe.  Use of this option will also prevent removal of
+		 * the patch.
+		 *
+		 * See Documentation/livepatch/livepatch.txt for more details.
+		 */
+		patch.immediate = true;
+		pr_notice("The consistency model isn't supported for your architecture.  Bypassing safety mechanisms and applying the patch immediately.\n");
+	}
+
+	ret = klp_register_patch(&patch);
+	if (ret)
+		return ret;
+	ret = klp_enable_patch(&patch);
+	if (ret) {
+		WARN_ON(klp_unregister_patch(&patch));
+		return ret;
+	}
+	return 0;
+}
+
+static void livepatch_callbacks_demo_exit(void)
+{
+	WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_callbacks_demo_init);
+module_exit(livepatch_callbacks_demo_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-mod.c
new file mode 100644
index 000000000000..d0f8657e19f3
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-mod.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-callbacks-mod.c - (un)patching callbacks demo support module
+ *
+ *
+ * Purpose
+ * -------
+ *
+ * Simple module to demonstrate livepatch (un)patching callbacks.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * This module is not intended to be standalone.  See the "Usage"
+ * section of livepatch-callbacks-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+
+static int livepatch_callbacks_mod_init(void)
+{
+	pr_info("%s\n", __func__);
+	return 0;
+}
+
+static void livepatch_callbacks_mod_exit(void)
+{
+	pr_info("%s\n", __func__);
+}
+
+module_init(livepatch_callbacks_mod_init);
+module_exit(livepatch_callbacks_mod_exit);
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1713304

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-08-16 22:30 +0200
Message-ID<ufd6X-3gr-47@gated-at.bofh.it>
In reply to#1713243
On Wed, Aug 16, 2017 at 03:17:04PM -0400, Joe Lawrence wrote:
> 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 action takes
>     place.
> 
>   - 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/callback.txt for details.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  Documentation/livepatch/callbacks.txt        |  87 ++++++++++++
>  include/linux/livepatch.h                    |  81 ++++++++++++
>  kernel/livepatch/core.c                      |  37 ++++--
>  kernel/livepatch/patch.c                     |   5 +-
>  kernel/livepatch/transition.c                |  21 ++-
>  samples/livepatch/Makefile                   |   2 +
>  samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
>  samples/livepatch/livepatch-callbacks-mod.c  |  53 ++++++++
>  8 files changed, 462 insertions(+), 14 deletions(-)
>  create mode 100644 Documentation/livepatch/callbacks.txt
>  create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
>  create mode 100644 samples/livepatch/livepatch-callbacks-mod.c
> 
> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
> new file mode 100644
> index 000000000000..e18f2678f3bb
> --- /dev/null
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,87 @@
> +(Un)patching Callbacks
> +======================
> +
> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> +to execute callback functions when a kernel object is (un)patched.  They
> +can be considered a "power feature" that extends livepatching abilities
> +to include:
> +
> +  - Safe updates to global data
> +
> +  - "Patches" to init and probe functions
> +
> +  - Patching otherwise unpatchable code (i.e. assembly)
> +
> +In most cases, (un)patch hooks will need to be used in conjunction with

s/hooks/callbacks/

(though I wouldn't be opposed to changing them back to "hooks" anyway as
it's one fewer syllable :-) but no strong opinion either way on that)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1715087

FromPetr Mladek <pmladek@suse.com>
Date2017-08-18 16:00 +0200
Message-ID<ufPYD-4vA-63@gated-at.bofh.it>
In reply to#1713243
On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> 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.
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991ef9347..500dc9b2b361 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -138,6 +154,71 @@ struct klp_patch {
>  	     func->old_name || func->new_func || func->old_sympos; \
>  	     func++)
>  
> +/**
> + * klp_is_object_loaded() - is klp_object currently loaded?
> + * @obj:	klp_object pointer
> + *
> + * Return: true if klp_object is loaded (always true for vmlinux)
> + */
> +static inline bool klp_is_object_loaded(struct klp_object *obj)
> +{
> +	return !obj->name || obj->mod;
> +}
> +
> +/**
> + * klp_pre_patch_callback - execute before klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.pre_patch)
> +		return (*obj->callbacks.pre_patch)(obj);
> +	return 0;
> +}
> +
> +/**
> + * klp_post_patch_callback() - execute after klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.post_patch)
> +		(*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> + *                              and is active across all tasks
> + * @obj:	invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.pre_unpatch)
> +		(*obj->callbacks.pre_unpatch)(obj);
> +}
> +
> +/**
> + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> + *                               all code has been restored and no tasks
> + *                               are running patched code
> + * @obj:	invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.post_unpatch)
> +		(*obj->callbacks.post_unpatch)(obj);
> +}

I guess that we do not want to make these function usable
outside livepatch code. Thefore these inliners should go
to kernel/livepatch/core.h or so.

> +
>  int klp_register_patch(struct klp_patch *);
>  int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..ddb23e18a357 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
>  				goto err;
>  			}
>  
> +			klp_post_patch_callback(obj);

This should be called only if (patch != klp_transition_patch).
Otherwise, it would be called too early.

> +
>  			break;
>  		}
>  	}
> @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
>  			if (patch->enabled || patch == klp_transition_patch) {
>  				pr_notice("reverting patch '%s' on unloading module '%s'\n",
>  					  patch->mod->name, obj->mod->name);
> +
> +				klp_pre_unpatch_callback(obj);

Also the pre_unpatch() callback should be called only
if (patch != klp_transition_patch). Otherwise, it should have
already been called. It is not the current case but see below.


>  				klp_unpatch_object(obj);
> +				klp_post_unpatch_callback(obj);
>  			}
>  
>  			klp_free_object_loaded(obj);
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 52c4e907c14b..0eed0df6e6d9 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
>  	klp_for_each_func(obj, func) {
>  		ret = klp_patch_func(func);
>  		if (ret) {
> +			klp_pre_unpatch_callback(obj);

This looks strange (somehow asymetric). IMHO, it should not be
needed. klp_pre_unpatch_callback() should revert changes done
by klp_post_patch_callback() that has not run yet.

>  			klp_unpatch_object(obj);
>  			return ret;
>  		}
> @@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
>  	struct klp_object *obj;
>  
>  	klp_for_each_object(patch, obj)
> -		if (obj->patched)
> +		if (obj->patched) {
> +			klp_pre_unpatch_callback(obj);

This is even more strange. The corresponding klp_post_patch_callback()
is called at the very end of the transaction when the patch is already
used by the entire system. Therefore the operation should be reverted
before we start disabling the patch.

IMHO, klp_pre_unpatch_callback() should get called in
__klp_disable_patch(). I would put it right after
klp_init_transition(patch, KLP_UNPATCHED);

Another advantage of this logic is that we actually do not need
to care about these callbacks in klp_reverse_transition().
But we should probably mention it in the documentation
how the actions done by the patch and unpatch callbacks
are related.

Otherwise, it looks fine to me.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1716889

FromJoe Lawrence <joe.lawrence@redhat.com>
Date2017-08-21 23:20 +0200
Message-ID<uh2h3-16g-13@gated-at.bofh.it>
In reply to#1715087
On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > 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.
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 194991ef9347..500dc9b2b361 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -138,6 +154,71 @@ struct klp_patch {
> >  	     func->old_name || func->new_func || func->old_sympos; \
> >  	     func++)
> >  
> > +/**
> > + * klp_is_object_loaded() - is klp_object currently loaded?
> > + * @obj:	klp_object pointer
> > + *
> > + * Return: true if klp_object is loaded (always true for vmlinux)
> > + */
> > +static inline bool klp_is_object_loaded(struct klp_object *obj)
> > +{
> > +	return !obj->name || obj->mod;
> > +}
> > +
> > +/**
> > + * klp_pre_patch_callback - execute before klp_object is patched
> > + * @obj:	invoke callback for this klp_object
> > + *
> > + * Return: status from callback
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline int klp_pre_patch_callback(struct klp_object *obj)
> > +{
> > +	if (obj->callbacks.pre_patch)
> > +		return (*obj->callbacks.pre_patch)(obj);
> > +	return 0;
> > +}
> > +
> > +/**
> > + * klp_post_patch_callback() - execute after klp_object is patched
> > + * @obj:	invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_post_patch_callback(struct klp_object *obj)
> > +{
> > +	if (obj->callbacks.post_patch)
> > +		(*obj->callbacks.post_patch)(obj);
> > +}
> > +
> > +/**
> > + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> > + *                              and is active across all tasks
> > + * @obj:	invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> > +{
> > +	if (obj->callbacks.pre_unpatch)
> > +		(*obj->callbacks.pre_unpatch)(obj);
> > +}
> > +
> > +/**
> > + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> > + *                               all code has been restored and no tasks
> > + *                               are running patched code
> > + * @obj:	invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> > +{
> > +	if (obj->callbacks.post_unpatch)
> > +		(*obj->callbacks.post_unpatch)(obj);
> > +}
> 
> I guess that we do not want to make these function usable
> outside livepatch code. Thefore these inliners should go
> to kernel/livepatch/core.h or so.

Okay, I can stash them away in an internal header file like core.h.

> > +
> >  int klp_register_patch(struct klp_patch *);
> >  int klp_unregister_patch(struct klp_patch *);
> >  int klp_enable_patch(struct klp_patch *);
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..ddb23e18a357 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> >  				goto err;
> >  			}
> >  
> > +			klp_post_patch_callback(obj);
> 
> This should be called only if (patch != klp_transition_patch).
> Otherwise, it would be called too early.

Can you elaborate a bit on this scenario?  When would the transition
patch (as I understand it, a livepatch not quite fully (un)patched) hit
the module coming/going notifier?  Is it possible to load or unload a
module like this?  I'd like to add this scenario to my test script if
possible.
 
> > +
> >  			break;
> >  		}
> >  	}
> > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> >  			if (patch->enabled || patch == klp_transition_patch) {
> >  				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> >  					  patch->mod->name, obj->mod->name);
> > +
> > +				klp_pre_unpatch_callback(obj);
> 
> Also the pre_unpatch() callback should be called only
> if (patch != klp_transition_patch). Otherwise, it should have
> already been called. It is not the current case but see below.

Ditto.

> >  				klp_unpatch_object(obj);
> > +				klp_post_unpatch_callback(obj);
> >  			}
> >  
> >  			klp_free_object_loaded(obj);
> > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> > index 52c4e907c14b..0eed0df6e6d9 100644
> > --- a/kernel/livepatch/patch.c
> > +++ b/kernel/livepatch/patch.c
> > @@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
> >  	klp_for_each_func(obj, func) {
> >  		ret = klp_patch_func(func);
> >  		if (ret) {
> > +			klp_pre_unpatch_callback(obj);
> 
> This looks strange (somehow asymetric). IMHO, it should not be
> needed. klp_pre_unpatch_callback() should revert changes done
> by klp_post_patch_callback() that has not run yet.

I was skeptical about this call, too.  After reading your comment, I
realize it shouldn't be needed here.

> >  			klp_unpatch_object(obj);
> >  			return ret;
> >  		}
> > @@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
> >  	struct klp_object *obj;
> >  
> >  	klp_for_each_object(patch, obj)
> > -		if (obj->patched)
> > +		if (obj->patched) {
> > +			klp_pre_unpatch_callback(obj);
> 
> This is even more strange. The corresponding klp_post_patch_callback()
> is called at the very end of the transaction when the patch is already
> used by the entire system. Therefore the operation should be reverted
> before we start disabling the patch.
> 
> IMHO, klp_pre_unpatch_callback() should get called in
> __klp_disable_patch(). I would put it right after
> klp_init_transition(patch, KLP_UNPATCHED);

Okay, looking at the transition code, this makes sense.  I'll move the
pre-(un)patch calls into the __enable_patch() / __disable_patch()
functions after initalizing the transition.  I think that should clean
up some of the strange ordering of kernel log msgs as well.
 
> Another advantage of this logic is that we actually do not need
> to care about these callbacks in klp_reverse_transition().
> But we should probably mention it in the documentation
> how the actions done by the patch and unpatch callbacks
> are related.
> 
> Otherwise, it looks fine to me.

Thanks for reviewing.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1717206

FromPetr Mladek <pmladek@suse.com>
Date2017-08-22 11:20 +0200
Message-ID<uhdvP-jp-7@gated-at.bofh.it>
In reply to#1716889
On Mon 2017-08-21 17:18:58, Joe Lawrence wrote:
> On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> > On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > > 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.
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..ddb23e18a357 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > >  				goto err;
> > >  			}
> > >  
> > > +			klp_post_patch_callback(obj);
> > 
> > This should be called only if (patch != klp_transition_patch).
> > Otherwise, it would be called too early.
> 
> Can you elaborate a bit on this scenario?  When would the transition
> patch (as I understand it, a livepatch not quite fully (un)patched) hit
> the module coming/going notifier?  Is it possible to load or unload a
> module like this?  I'd like to add this scenario to my test script if
> possible.

if (patch == klp_transition_patch) then the transition for this
patch is still running. klp_post_patch_callback() should and is
called at the end of the transition, see klp_complete_transition().
Note that klp_complete_transition() will see the object/module
loaded because obj->mod is set in klp_module_coming().

The (up)patch transition might take a while. It might be even
infinite if a process is blocked in a patched function. klp_mutex
is available most of the time. Therefore it is perfectly fine to
load/remove modules into/from the system and run their
klp_module_coming()/going() callbacks when a livepatch
patch transition is running.


> > > +
> > >  			break;
> > >  		}
> > >  	}
> > > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > >  			if (patch->enabled || patch == klp_transition_patch) {
> > >  				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > >  					  patch->mod->name, obj->mod->name);
> > > +
> > > +				klp_pre_unpatch_callback(obj);
> > 
> > Also the pre_unpatch() callback should be called only
> > if (patch != klp_transition_patch). Otherwise, it should have
> > already been called. It is not the current case but see below.
> 
> Ditto.

This is related to the other comment where I and Josh suggested
to call klp_pre_unpatch_callback() in __klp_disable_patch()
before the transition started. It means that the callback
has already been called for klp_transition_patch.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web