[PATCH v2 0/1] add (un)patch callbacks

[PATCH v2 0/1] add (un)patch callbacks

[Joe Lawrence]

Hi folks,

This is v2 of the livepatch (un)patch hook/notifier/callback/etc code.

The documentation is still a little rough, but I wanted to post up the
code for feedback before getting too far in revising it.

This implements the pre and post handlers as Josh suggested.  See
summary of related changes below...

v2:
- s/hooks/callbacks/g
- implemented pre-(un)patch and post-(un)patch callbacks
  - pre-patch and pre-unpatch callbacks run from callers of
    klp_patch_object() and klp_unpatch_object()
  - post-patch and post-unpatch callbacks run from
    klp_complete_transition() and klp_module_coming/going()
- reduce callbacks from a list to a single per-klp_object instance
- revamp the sample callback demo
- revamp documentation

Feedback appreciated as always.

Joe Lawrence (1):
  livepatch: add (un)patch callbacks

 Documentation/livepatch/callbacks.txt        |  75 +++++++++++
 include/linux/livepatch.h                    |  38 ++++++
 kernel/livepatch/core.c                      |  30 ++++-
 kernel/livepatch/patch.c                     |   5 +-
 kernel/livepatch/transition.c                |  19 ++-
 samples/livepatch/Makefile                   |   2 +
 samples/livepatch/livepatch-callbacks-demo.c | 180 +++++++++++++++++++++++++++
 samples/livepatch/livepatch-callbacks-mod.c  |  53 ++++++++
 8 files changed, 393 insertions(+), 9 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

-- 
1.8.3.1

[PATCH v2 1/1] livepatch: add (un)patch callbacks

[Joe Lawrence]

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        |  75 +++++++++++
 include/linux/livepatch.h                    |  38 ++++++
 kernel/livepatch/core.c                      |  32 ++++-
 kernel/livepatch/patch.c                     |   5 +-
 kernel/livepatch/transition.c                |  19 ++-
 samples/livepatch/Makefile                   |   2 +
 samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
 samples/livepatch/livepatch-callbacks-mod.c  |  53 ++++++++
 8 files changed, 405 insertions(+), 9 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..d78f9ba9f74c
--- /dev/null
+++ b/Documentation/livepatch/callbacks.txt
@@ -0,0 +1,75 @@
+(Un)patching Callbacks
+======================
+
+Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
+to execute callback functions when a kernel object is (un)patched.
+
+These 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
+
+Callbacks are 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..5fb8925fc488 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,28 @@ struct klp_patch {
 	     func->old_name || func->new_func || func->old_sympos; \
 	     func++)
 
+static inline int klp_pre_patch_callback(struct klp_object *obj)
+{
+	if (!obj->patched && obj->callbacks.pre_patch)
+		return (*obj->callbacks.pre_patch)(obj);
+	return 0;
+}
+static inline void klp_post_patch_callback(struct klp_object *obj)
+{
+	if (obj->patched && obj->callbacks.post_patch)
+		(*obj->callbacks.post_patch)(obj);
+}
+static inline void klp_pre_unpatch_callback(struct klp_object *obj)
+{
+	if (obj->patched && obj->callbacks.pre_unpatch)
+		(*obj->callbacks.pre_unpatch)(obj);
+}
+static inline void klp_post_unpatch_callback(struct klp_object *obj)
+{
+	if (!obj->patched && 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..6f205238582b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -388,13 +388,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 +408,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 +881,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 +895,8 @@ int klp_module_coming(struct module *mod)
 				goto err;
 			}
 
+			klp_post_patch_callback(obj);
+
 			break;
 		}
 	}
@@ -929,7 +948,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..fb2a986e66fc 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,22 @@ static void klp_complete_transition(void)
 	}
 
 done:
+	klp_for_each_object(klp_transition_patch, obj) {
+		if (klp_target_state == KLP_PATCHED)
+			klp_post_patch_callback(obj);
+		else if (klp_target_state == KLP_PATCHED)
+			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) {
+		if (klp_target_state == KLP_UNPATCHED && !immediate_func)
+			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..a91f05c8258c
--- /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

Re: [PATCH v2 1/1] livepatch: add (un)patch callbacks

[Josh Poimboeuf]

On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote:
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,75 @@
> +(Un)patching Callbacks
> +======================
> +
> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> +to execute callback functions when a kernel object is (un)patched.

I think it would be helpful to put a little blurb here about why
callbacks are needed and when they might be used.  Maybe steal some of
the description from the first two bullet points here:

  https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble

Also, I tested stop_machine() in the callbacks and it seemed to work
fine.  It might be worth mentioning in the docs that it's an option.

> +
> +These 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
> +
> +Callbacks are only executed if its host klp_object is loaded.  For

"Callbacks are" -> "A callback is" ?

> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> +	if (!obj->patched && obj->callbacks.pre_patch)
> +		return (*obj->callbacks.pre_patch)(obj);
> +	return 0;
> +}
> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> +	if (obj->patched && obj->callbacks.post_patch)
> +		(*obj->callbacks.post_patch)(obj);
> +}
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> +	if (obj->patched && obj->callbacks.pre_unpatch)
> +		(*obj->callbacks.pre_unpatch)(obj);
> +}
> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> +{
> +	if (!obj->patched && obj->callbacks.post_unpatch)
> +		(*obj->callbacks.post_unpatch)(obj);
> +}
> +

Do these need the obj->patched checks?  As far as I can tell they seem
to be called in the right places and the checks are superfluous.

> --- 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,22 @@ static void klp_complete_transition(void)
>  	}
>  
>  done:
> +	klp_for_each_object(klp_transition_patch, obj) {
> +		if (klp_target_state == KLP_PATCHED)
> +			klp_post_patch_callback(obj);
> +		else if (klp_target_state == KLP_PATCHED)

s/KLP_PATCHED/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) {
> +		if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> +			module_put(klp_transition_patch->mod);
> +	}
> +

Maybe combine these into a single 'if' for clarity:

	if (klp_target_state == KLP_UNPATCHED && !immediate_func &&
	    !klp_transition_patch->immediate)
		module_put(klp_transition_patch->mod);

> + * 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 

Git complained about trailing whitespace here ^

> + *
> + *       # Module loader refuses to load the target module
> + *       insmod samples/livepatch/livepatch-callbacks-mod.ko 

and here ^

> +/* Executed on object unpatching (ie, patch disablement) */
> +static void post_patch_callback(struct klp_object *obj)

s/unpatching/patching/

-- 
Josh

Re: [PATCH v2 1/1] livepatch: add (un)patch callbacks

[Joe Lawrence]

On 08/11/2017 04:44 PM, Josh Poimboeuf wrote:
> On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote:
>> +++ b/Documentation/livepatch/callbacks.txt
>> @@ -0,0 +1,75 @@
>> +(Un)patching Callbacks
>> +======================
>> +
>> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
>> +to execute callback functions when a kernel object is (un)patched.
> 
> I think it would be helpful to put a little blurb here about why
> callbacks are needed and when they might be used.  Maybe steal some of
> the description from the first two bullet points here:
> 
>   https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble

Ok -- btw, can you explain this point: "patching otherwise unpatchable
code (i.e., assembly)".  I wasn't sure if you were referring to the
actual code, or modifying the machine state as setup by some init time
assembly.

> Also, I tested stop_machine() in the callbacks and it seemed to work
> fine.  It might be worth mentioning in the docs that it's an option.

I'll file that under the "you better know what you're doing" section. :)
If your test would be a better use-case example or sample module than
what's currently in the patchset, feel free to send it over and I can
incorporate it.

>> +
>> +These 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
>> +
>> +Callbacks are only executed if its host klp_object is loaded.  For
> 
> "Callbacks are" -> "A callback is" ?

Okay.  What about the preceding plural-case instances?

> 
>> +static inline int klp_pre_patch_callback(struct klp_object *obj)
>> +{
>> +	if (!obj->patched && obj->callbacks.pre_patch)
>> +		return (*obj->callbacks.pre_patch)(obj);
>> +	return 0;
>> +}
>> +static inline void klp_post_patch_callback(struct klp_object *obj)
>> +{
>> +	if (obj->patched && obj->callbacks.post_patch)
>> +		(*obj->callbacks.post_patch)(obj);
>> +}
>> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
>> +{
>> +	if (obj->patched && obj->callbacks.pre_unpatch)
>> +		(*obj->callbacks.pre_unpatch)(obj);
>> +}
>> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
>> +{
>> +	if (!obj->patched && obj->callbacks.post_unpatch)
>> +		(*obj->callbacks.post_unpatch)(obj);
>> +}
>> +
> 
> Do these need the obj->patched checks?  As far as I can tell they seem
> to be called in the right places and the checks are superfluous.

That is correct.  I can leave them (defensive coding) or take them out
and perhaps add comments above to explain their use and assumptions.

>> --- 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,22 @@ static void klp_complete_transition(void)
>>  	}
>>  
>>  done:
>> +	klp_for_each_object(klp_transition_patch, obj) {
>> +		if (klp_target_state == KLP_PATCHED)
>> +			klp_post_patch_callback(obj);
>> +		else if (klp_target_state == KLP_PATCHED)
> 
> s/KLP_PATCHED/KLP_UNPATCHED

Ahh, I was so focused on the loadable module cases in
module_coming/going that I botched this case.  Will fix for v3.

>> +			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) {
>> +		if (klp_target_state == KLP_UNPATCHED && !immediate_func)
>> +			module_put(klp_transition_patch->mod);
>> +	}
>> +
> 
> Maybe combine these into a single 'if' for clarity:
> 
> 	if (klp_target_state == KLP_UNPATCHED && !immediate_func &&
> 	    !klp_transition_patch->immediate)
> 		module_put(klp_transition_patch->mod);

How about this arrangement:

if (!klp_transition_patch->immediate &&
    klp_target_state == KLP_UNPATCHED && !immediate_func) {
	module_put(klp_transition_patch->mod);
}

1) It leads with the klp_transition_patch->immediate variable, which the
preceding comment and goto is all about and 2) brackets the multiline
conditional part -- a personal preference, but I could drop for
convention sake.

>> + * 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 
> 
> Git complained about trailing whitespace here ^
> 
>> + *
>> + *       # Module loader refuses to load the target module
>> + *       insmod samples/livepatch/livepatch-callbacks-mod.ko 
> 
> and here ^

Oh hey, look who was too cool to run checkpatch, again.

>> +/* Executed on object unpatching (ie, patch disablement) */
>> +static void post_patch_callback(struct klp_object *obj)
> 
> s/unpatching/patching/
> 

Good catch.

So v2 was a bit rushed to try and get something out there to talk about:

Are the callback locations accurate to your v1 suggestions?

How do you feel about a pre-patch callback potentially preventing the
loading of a kernel module -or- the patch module itself depending on
which is loaded first?

Is the pre-patch return status sufficient? (ie, I couldn't see how
post-patch, pre-unpatch, post-patch callbacks could affect the actions
already set in motion.)

Thanks,

-- Joe

Re: [PATCH v2 1/1] livepatch: add (un)patch callbacks

[Josh Poimboeuf]

On Mon, Aug 14, 2017 at 10:53:08AM -0400, Joe Lawrence wrote:
> On 08/11/2017 04:44 PM, Josh Poimboeuf wrote:
> > On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote:
> >> +++ b/Documentation/livepatch/callbacks.txt
> >> @@ -0,0 +1,75 @@
> >> +(Un)patching Callbacks
> >> +======================
> >> +
> >> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> >> +to execute callback functions when a kernel object is (un)patched.
> > 
> > I think it would be helpful to put a little blurb here about why
> > callbacks are needed and when they might be used.  Maybe steal some of
> > the description from the first two bullet points here:
> > 
> >   https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble
> 
> Ok -- btw, can you explain this point: "patching otherwise unpatchable
> code (i.e., assembly)".  I wasn't sure if you were referring to the
> actual code, or modifying the machine state as setup by some init time
> assembly.

I meant actually patching assembly code.

> > Also, I tested stop_machine() in the callbacks and it seemed to work
> > fine.  It might be worth mentioning in the docs that it's an option.
> 
> I'll file that under the "you better know what you're doing" section. :)
> If your test would be a better use-case example or sample module than
> what's currently in the patchset, feel free to send it over and I can
> incorporate it.

Well, it's questionable whether using stop_machine() is a good idea, and
it's one of those "use as a last resort" things, so maybe we don't need
to add it to the sample module.

> >> +These 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
> >> +
> >> +Callbacks are only executed if its host klp_object is loaded.  For
> > 
> > "Callbacks are" -> "A callback is" ?
> 
> Okay.  What about the preceding plural-case instances?

I think it doesn't matter much, as long as each sentence is
grammatically consistent with itself.

> >> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> >> +{
> >> +	if (!obj->patched && obj->callbacks.pre_patch)
> >> +		return (*obj->callbacks.pre_patch)(obj);
> >> +	return 0;
> >> +}
> >> +static inline void klp_post_patch_callback(struct klp_object *obj)
> >> +{
> >> +	if (obj->patched && obj->callbacks.post_patch)
> >> +		(*obj->callbacks.post_patch)(obj);
> >> +}
> >> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> >> +{
> >> +	if (obj->patched && obj->callbacks.pre_unpatch)
> >> +		(*obj->callbacks.pre_unpatch)(obj);
> >> +}
> >> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> >> +{
> >> +	if (!obj->patched && obj->callbacks.post_unpatch)
> >> +		(*obj->callbacks.post_unpatch)(obj);
> >> +}
> >> +
> > 
> > Do these need the obj->patched checks?  As far as I can tell they seem
> > to be called in the right places and the checks are superfluous.
> 
> That is correct.  I can leave them (defensive coding) or take them out
> and perhaps add comments above to explain their use and assumptions.

Personally I'd rather get rid of the checks as I found them confusing.

If we really wanted to get defensive, we could add some WARNs, but that
might be overkill.

> >> --- 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,22 @@ static void klp_complete_transition(void)
> >>  	}
> >>  
> >>  done:
> >> +	klp_for_each_object(klp_transition_patch, obj) {
> >> +		if (klp_target_state == KLP_PATCHED)
> >> +			klp_post_patch_callback(obj);
> >> +		else if (klp_target_state == KLP_PATCHED)
> > 
> > s/KLP_PATCHED/KLP_UNPATCHED
> 
> Ahh, I was so focused on the loadable module cases in
> module_coming/going that I botched this case.  Will fix for v3.
> 
> >> +			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) {
> >> +		if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> >> +			module_put(klp_transition_patch->mod);
> >> +	}
> >> +
> > 
> > Maybe combine these into a single 'if' for clarity:
> > 
> > 	if (klp_target_state == KLP_UNPATCHED && !immediate_func &&
> > 	    !klp_transition_patch->immediate)
> > 		module_put(klp_transition_patch->mod);
> 
> How about this arrangement:
> 
> if (!klp_transition_patch->immediate &&
>     klp_target_state == KLP_UNPATCHED && !immediate_func) {
> 	module_put(klp_transition_patch->mod);
> }
> 
> 1) It leads with the klp_transition_patch->immediate variable, which the
> preceding comment and goto is all about and 2) brackets the multiline
> conditional part -- a personal preference, but I could drop for
> convention sake.

The bracket's fine with me.  Personally I think it makes more sense to
bunch the immediate checks together.

> >> + * 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 
> > 
> > Git complained about trailing whitespace here ^
> > 
> >> + *
> >> + *       # Module loader refuses to load the target module
> >> + *       insmod samples/livepatch/livepatch-callbacks-mod.ko 
> > 
> > and here ^
> 
> Oh hey, look who was too cool to run checkpatch, again.
> 
> >> +/* Executed on object unpatching (ie, patch disablement) */
> >> +static void post_patch_callback(struct klp_object *obj)
> > 
> > s/unpatching/patching/
> > 
> 
> Good catch.
> 
> So v2 was a bit rushed to try and get something out there to talk about:
> 
> Are the callback locations accurate to your v1 suggestions?

Yeah, it all looked good to me.

> How do you feel about a pre-patch callback potentially preventing the
> loading of a kernel module -or- the patch module itself depending on
> which is loaded first?

I think that makes sense.

> Is the pre-patch return status sufficient? (ie, I couldn't see how
> post-patch, pre-unpatch, post-patch callbacks could affect the actions
> already set in motion.)

I think so.

-- 
Josh

Re: [PATCH v2 0/1] add (un)patch callbacks

[Miroslav Benes]

On Tue, 8 Aug 2017, Joe Lawrence wrote:

> Hi folks,
> 
> This is v2 of the livepatch (un)patch hook/notifier/callback/etc code.
> 
> The documentation is still a little rough, but I wanted to post up the
> code for feedback before getting too far in revising it.
> 
> This implements the pre and post handlers as Josh suggested.  See
> summary of related changes below...
> 
> v2:
> - s/hooks/callbacks/g
> - implemented pre-(un)patch and post-(un)patch callbacks
>   - pre-patch and pre-unpatch callbacks run from callers of
>     klp_patch_object() and klp_unpatch_object()
>   - post-patch and post-unpatch callbacks run from
>     klp_complete_transition() and klp_module_coming/going()
> - reduce callbacks from a list to a single per-klp_object instance
> - revamp the sample callback demo
> - revamp documentation
> 
> Feedback appreciated as always.

Looks almost good to me. v3 with Josh's review should be ok.

Miroslav