[PATCH] livepatch hooks, revisted

[PATCH] livepatch hooks, revisted

[Joe Lawrence]

Hi all,

This patch revists the effort that Chris Argus made last year to port
kpatch-style "load hooks" to livepatch:

  https://lkml.org/lkml/2016/8/26/434

Since that discussion, the consistency model has been merged.

This patch locates the hooks in a slightly different place, providing
callbacks to a livepatch module whenever a klp_object is being patched
or unpatched.

A pointer to the current klp_object undergoing (un)patching is also
passed to the hook callback -- with that in hand, the hook
implementation can make decisions based on the current module state
(live, coming, going).

A new Documentation/ file is provided as well as a contrived sample
module and livepatch demo to demonstrate the callbacks.  The example is
about as simple as possible, but could be further embellished to
resemble a real-world livepatch fix if desired.

Thanks,

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

 Documentation/livepatch/hooks.txt        |  98 +++++++++++++++++++++++++
 include/linux/livepatch.h                |  32 ++++++++
 kernel/livepatch/core.c                  |   5 --
 kernel/livepatch/patch.c                 |  35 +++++++++
 samples/livepatch/Makefile               |   2 +
 samples/livepatch/livepatch-hooks-demo.c | 122 +++++++++++++++++++++++++++++++
 samples/livepatch/livepatch-hooks-mod.c  |  38 ++++++++++
 7 files changed, 327 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/livepatch/hooks.txt
 create mode 100644 samples/livepatch/livepatch-hooks-demo.c
 create mode 100644 samples/livepatch/livepatch-hooks-mod.c

-- 
1.8.3.1

[PATCH] livepatch: add (un)patch hooks

[Joe Lawrence]

When the livepatch core executes klp_(un)patch_object, call out to a
livepatch-module specified array of callback hooks.  These hooks provide
a notification mechanism for livepatch modules when klp_objects are
(un)patching. This may be most interesting when another kernel module
is a klp_object target and the livepatch module needs to execute code
after the target is loaded, but before its module_init code is run.

The patch-hook executes right before patching objects and the
unpatch-hook executes right after unpatching objects.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/hooks.txt        |  98 +++++++++++++++++++++++++
 include/linux/livepatch.h                |  32 ++++++++
 kernel/livepatch/core.c                  |   5 --
 kernel/livepatch/patch.c                 |  35 +++++++++
 samples/livepatch/Makefile               |   2 +
 samples/livepatch/livepatch-hooks-demo.c | 122 +++++++++++++++++++++++++++++++
 samples/livepatch/livepatch-hooks-mod.c  |  38 ++++++++++
 7 files changed, 327 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/livepatch/hooks.txt
 create mode 100644 samples/livepatch/livepatch-hooks-demo.c
 create mode 100644 samples/livepatch/livepatch-hooks-mod.c

diff --git a/Documentation/livepatch/hooks.txt b/Documentation/livepatch/hooks.txt
new file mode 100644
index 000000000000..ef18101a3b90
--- /dev/null
+++ b/Documentation/livepatch/hooks.txt
@@ -0,0 +1,98 @@
+(Un)patching Hooks
+==================
+
+Livepatching (un)patch-hooks provide a mechanism to register and execute
+a set of callback functions when the kernel's livepatching core performs
+an (un)patching operation on a given kernel object.
+
+The hooks are provided and registered by a livepatch module as part of
+klp_objects that make up its klp_patch structure.  Both patch and
+unpatch-hook function signatures accept a pointer to a klp_object
+argument and return an integer status, ie:
+
+  static int patch_hook(struct klp_object *obj)
+  {
+  	/* ... */
+  }
+  static int unpatch_hook(struct klp_object *obj)
+  {
+  	/* ... */
+  }
+
+  static struct klp_hook patch_hooks[] = {
+  	{
+  		.hook = patch_hook,
+  	}, { }
+  };
+  static struct klp_hook unpatch_hooks[] = {
+  	{
+  		.hook = unpatch_hook,
+  	}, { }
+  };
+
+  static struct klp_object objs[] = {
+  	{
+  		/* ... */
+  		.patch_hooks = patch_hooks,
+  		.unpatch_hooks = unpatch_hooks,
+  	}, { }
+  };
+
+  static struct klp_patch patch = {
+  	.mod = THIS_MODULE,
+  	.objs = objs,
+  };
+
+If a hook returns non-zero status, the livepatching core will log a
+hook failure warning message.
+
+Multiple (un)patch-hooks may be registered per klp_object.  Each hook
+will execute regardless of any previously executed hook's non-zero
+return status.
+
+Hooks are optional.  The livepatching core will not execute any
+callbacks for an empty klp_hook.hook array or a NULL klp_hook.hook
+value.
+
+
+For module targets
+------------------
+
+In the case of kernel module objects, patch-hooks provide a livepatch
+module opportunity to defer execution until a target module is loaded.
+Similarly, unpatch-hooks only call back into a livepatch module after a
+target module has itself cleaned up.  In these cases, the order of
+execution looks like:
+
+  load kernel module
+  execute all patch_hooks[] for this kernel object
+  livepatch kernel object
+  execute module_init function
+
+  ...
+
+  unload kernel module
+  execute module_exit function
+  livepatch restore kernel object
+  execute all unpatch_hooks[] for this kernel object
+
+On the other hand, if a target kernel module is already present when a
+livepatch is loading, then the corresponding patch hook(s) will execute
+as soon as the livepatching kernel core enables the livepatch.
+
+It may be useful for hooks to inspect the module state of the klp_object
+it is passed (i.e. obj->mod->state).  Patch hooks can expect to see
+modules in MODULE_STATE_LIVE and MODULE_STATE_COMING states.  Unpatch
+hooks can expect modules in MODULE_STATE_LIVE and MODULE_STATE_GOING
+states.
+
+
+For vmlinux target
+------------------
+
+As the kernel is always loaded, patch-hooks for vmlinux will execute as
+soon as the livepatch core enables the livepatch.  Patch-hooks will also
+run if the livepatch is disabled and then re-enabled.
+
+Unpatch-hooks for vmlinux will only execute when the livepatch is
+disabled.
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..d95050386ac7 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -87,10 +87,23 @@ struct klp_func {
 	bool transition;
 };
 
+struct klp_object;
+
+/**
+ * struct klp_hook - hook structure for live patching
+ * @hook:	function to be executed on hook
+ *
+ */
+struct klp_hook {
+	int (*hook)(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
+ * @patch_hooks:	functions to be executed on patching
+ * @unpatch_hooks:	functions to be executed on unpatching
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
@@ -100,6 +113,8 @@ struct klp_object {
 	/* external */
 	const char *name;
 	struct klp_func *funcs;
+	struct klp_hook *patch_hooks;
+	struct klp_hook *unpatch_hooks;
 
 	/* internal */
 	struct kobject kobj;
@@ -108,6 +123,17 @@ struct klp_object {
 };
 
 /**
+ * klp_is_module() - is klp_object a module?
+ * @obj:	klp_object pointer
+ *
+ * Return: true if klp_object is a loadable module
+ */
+static inline bool klp_is_module(struct klp_object *obj)
+{
+	return obj->name;
+}
+
+/**
  * struct klp_patch - patch structure for live patching
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
@@ -138,6 +164,12 @@ struct klp_patch {
 	     func->old_name || func->new_func || func->old_sympos; \
 	     func++)
 
+#define klp_for_each_patch_hook(obj, hook) \
+	for (hook = obj->patch_hooks; hook && hook->hook; hook++)
+
+#define klp_for_each_unpatch_hook(obj, hook) \
+	for (hook = obj->unpatch_hooks; hook && hook->hook; hook++)
+
 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..ff3685470057 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,11 +49,6 @@
 
 static struct kobject *klp_root_kobj;
 
-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;
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 52c4e907c14b..c8084a18ddb7 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -235,25 +235,60 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
+/**
+ * klp_run_hook - execute a given klp_hook callback
+ * @hook:	callback hook
+ * @obj:	kernel object that has been hooked
+ *
+ * Return: return value from hook, or 0 if none is currently associated
+ */
+static int klp_run_hook(struct klp_hook *hook, struct klp_object *obj)
+{
+	if (hook && hook->hook)
+		return (*hook->hook)(obj);
+
+	return 0;
+}
+
 void klp_unpatch_object(struct klp_object *obj)
 {
 	struct klp_func *func;
+	struct klp_hook *hook;
+	int ret;
 
 	klp_for_each_func(obj, func)
 		if (func->patched)
 			klp_unpatch_func(func);
 
 	obj->patched = false;
+
+	klp_for_each_unpatch_hook(obj, hook) {
+		ret = klp_run_hook(hook, obj);
+		if (ret) {
+			pr_warn("unpatch hook '%p' failed for object '%s'\n",
+				hook, klp_is_module(obj) ? obj->name : "vmlinux");
+		}
+	}
+
 }
 
 int klp_patch_object(struct klp_object *obj)
 {
 	struct klp_func *func;
+	struct klp_hook *hook;
 	int ret;
 
 	if (WARN_ON(obj->patched))
 		return -EINVAL;
 
+	klp_for_each_patch_hook(obj, hook) {
+		ret = klp_run_hook(hook, obj);
+		if (ret) {
+			pr_warn("patch hook '%p' failed for object '%s'\n",
+				hook, klp_is_module(obj) ? obj->name : "vmlinux");
+		}
+	}
+
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 10319d7ea0b1..2568a56ba8f3 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1 +1,3 @@
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-hooks-demo.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-hooks-mod.o
diff --git a/samples/livepatch/livepatch-hooks-mod.c b/samples/livepatch/livepatch-hooks-mod.c
new file mode 100644
index 000000000000..f4ec09a5fc53
--- /dev/null
+++ b/samples/livepatch/livepatch-hooks-mod.c
@@ -0,0 +1,38 @@
+/*
+ * livepatch-hooks-mod.c - (un)patching hooks demo support module
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+
+static int livepatch_hooks_mod_init(void)
+{
+	pr_info("%s\n", __func__);
+	return 0;
+}
+
+static void livepatch_hooks_mod_exit(void)
+{
+	pr_info("%s\n", __func__);
+}
+
+module_init(livepatch_hooks_mod_init);
+module_exit(livepatch_hooks_mod_exit);
+MODULE_LICENSE("GPL");
diff --git a/samples/livepatch/livepatch-hooks-demo.c b/samples/livepatch/livepatch-hooks-demo.c
new file mode 100644
index 000000000000..672a749a0549
--- /dev/null
+++ b/samples/livepatch/livepatch-hooks-demo.c
@@ -0,0 +1,122 @@
+/*
+ * livepatch-hooks-demo.c - (un)patching hooks livepatch demo
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+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 hook_info(const char *hook, struct klp_object *obj)
+{
+	if (klp_is_module(obj))
+		pr_info("%s: %s\n", hook, module_state[obj->mod->state]);
+	else
+		pr_info("%s: vmlinux\n", hook);
+}
+
+/* Executed on object patching (ie, patch enablement) */
+static int patch_hook(struct klp_object *obj)
+{
+	hook_info(__func__, obj);
+	return 0;
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static int unpatch_hook(struct klp_object *obj)
+{
+	hook_info(__func__, obj);
+	return 0;
+}
+
+static struct klp_func funcs[] = {
+	{ }
+};
+
+static struct klp_hook patch_hooks[] = {
+	{
+		.hook = patch_hook,
+	}, { }
+};
+static struct klp_hook unpatch_hooks[] = {
+	{
+		.hook = unpatch_hook,
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		.name = "livepatch_hooks_mod",
+		.funcs = funcs,
+		.patch_hooks = patch_hooks,
+		.unpatch_hooks = unpatch_hooks,
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+static int livepatch_hooks_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_hooks_demo_exit(void)
+{
+	WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_hooks_demo_init);
+module_exit(livepatch_hooks_demo_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
-- 
1.8.3.1

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> When the livepatch core executes klp_(un)patch_object, call out to a
> livepatch-module specified array of callback hooks.  These hooks provide
> a notification mechanism for livepatch modules when klp_objects are
> (un)patching. This may be most interesting when another kernel module
> is a klp_object target and the livepatch module needs to execute code
> after the target is loaded, but before its module_init code is run.

And it's also useful for vmlinux.  Patch module load/unload is separate
from enable/disable, so the module init/exit functions can't be used for
patch-specific changes (e.g., global data changes).

> The patch-hook executes right before patching objects and the
> unpatch-hook executes right after unpatching objects.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Thanks for posting it.  We found this to be a useful feature in the
past, not quite as useful as shadow data, but still good to have for
certain cases.

Instead of "load hooks" I think it would be more accurate to call them
"enable/disable hooks".  (Maybe "callbacks" would be better than
"hooks"?  Not sure...)

Even better, we might want to be specific: "pre enable hooks" and "post
disable hooks".  (Or "pre patch hooks" and "post unpatch hooks"?)
Because we might eventually decide we need the corresponding "post
enable hooks" and "pre disable hooks" as well.

For the enable case, I think it would be a nice feature if we checked
the return code and aborted the patching operation on error.  I think
that should be easy enough.

For the unload case, it's too late to do anything, so I'd say a void
return code would be better.  Otherwise it implies that we actually do
something about it.  Maybe in that case we can leave it up to the user
to decide whether to print an error or WARN() or whatever.

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Joe Lawrence]

On Thu, Jul 13, 2017 at 08:46:40PM -0500, Josh Poimboeuf wrote:
> Date: Thu, 13 Jul 2017 20:46:40 -0500
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> To: Joe Lawrence <joe.lawrence@redhat.com>
> Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu
>  <jeyu@kernel.org>, Jiri Kosina <jikos@kernel.org>, Miroslav Benes
>  <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>, Chris J Arges
>  <chris.j.arges@canonical.com>
> Subject: Re: [PATCH] livepatch: add (un)patch hooks
> User-Agent: Mutt/1.6.0.1 (2016-04-01)
> 
> On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> > When the livepatch core executes klp_(un)patch_object, call out to a
> > livepatch-module specified array of callback hooks.  These hooks provide
> > a notification mechanism for livepatch modules when klp_objects are
> > (un)patching. This may be most interesting when another kernel module
> > is a klp_object target and the livepatch module needs to execute code
> > after the target is loaded, but before its module_init code is run.
> 
> And it's also useful for vmlinux.  Patch module load/unload is separate
> from enable/disable, so the module init/exit functions can't be used for
> patch-specific changes (e.g., global data changes).
> 
> > The patch-hook executes right before patching objects and the
> > unpatch-hook executes right after unpatching objects.
> > 
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> 
> Thanks for posting it.  We found this to be a useful feature in the
> past, not quite as useful as shadow data, but still good to have for
> certain cases.
> 
> Instead of "load hooks" I think it would be more accurate to call them
> "enable/disable hooks".  (Maybe "callbacks" would be better than
> "hooks"?  Not sure...)

Hi Josh,

I hesitataed in calling them "enable/disable" hooks as I associated
those terms at the patch level -- a livepatch might be enabled, but
callbacks for a module may not occur until its actually loaded.  (I'm
fine with whatever is most intuitive to the livepatching collective :)

"Callbacks" vs. "hooks" is a good point though, as the latter has
negative connotations, especially when callers of this facility will be
mostly out of tree.

> Even better, we might want to be specific: "pre enable hooks" and "post
> disable hooks".  (Or "pre patch hooks" and "post unpatch hooks"?)
> Because we might eventually decide we need the corresponding "post
> enable hooks" and "pre disable hooks" as well.

"Pre-patch" and "post-unpatch" are a bit wordy, but a good description.
I already felt it was important enough to document the order of
operations in the doc file and commit msg, so I like this idea. 

> For the enable case, I think it would be a nice feature if we checked
> the return code and aborted the patching operation on error.  I think
> that should be easy enough.

Yeah, that should be easy.  To be specific, you're only talking about
the patching operation on the associated klp_object, not the entire
klp_patch right?

> For the unload case, it's too late to do anything, so I'd say a void
> return code would be better.  Otherwise it implies that we actually do
> something about it.  Maybe in that case we can leave it up to the user
> to decide whether to print an error or WARN() or whatever.

Good point.  I can change that in v2.

Thanks,

-- Joe

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Fri, Jul 14, 2017 at 09:23:29AM -0400, Joe Lawrence wrote:
> > On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> > > When the livepatch core executes klp_(un)patch_object, call out to a
> > > livepatch-module specified array of callback hooks.  These hooks provide
> > > a notification mechanism for livepatch modules when klp_objects are
> > > (un)patching. This may be most interesting when another kernel module
> > > is a klp_object target and the livepatch module needs to execute code
> > > after the target is loaded, but before its module_init code is run.
> > 
> > And it's also useful for vmlinux.  Patch module load/unload is separate
> > from enable/disable, so the module init/exit functions can't be used for
> > patch-specific changes (e.g., global data changes).
> > 
> > > The patch-hook executes right before patching objects and the
> > > unpatch-hook executes right after unpatching objects.
> > > 
> > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > 
> > Thanks for posting it.  We found this to be a useful feature in the
> > past, not quite as useful as shadow data, but still good to have for
> > certain cases.
> > 
> > Instead of "load hooks" I think it would be more accurate to call them
> > "enable/disable hooks".  (Maybe "callbacks" would be better than
> > "hooks"?  Not sure...)
> 
> Hi Josh,
> 
> I hesitataed in calling them "enable/disable" hooks as I associated
> those terms at the patch level -- a livepatch might be enabled, but
> callbacks for a module may not occur until its actually loaded.  (I'm
> fine with whatever is most intuitive to the livepatching collective :)

Yeah, "enable/disable" isn't quite right.

But also I think "load" is a bit confusing because it sounds (to me)
like the hooks are called when the *patch* module is loaded.  And in the
case where the hooks are for the vmlinux object, "load" doesn't make
sense.

I think "patch/unpatch hooks" (or "callbacks") would be better.  That
matches our current terminology (and is validated by the fact that the
hooks are applied in klp_{patch,unpatch}_object().

> "Callbacks" vs. "hooks" is a good point though, as the latter has
> negative connotations, especially when callers of this facility will be
> mostly out of tree.
> 
> > Even better, we might want to be specific: "pre enable hooks" and "post
> > disable hooks".  (Or "pre patch hooks" and "post unpatch hooks"?)
> > Because we might eventually decide we need the corresponding "post
> > enable hooks" and "pre disable hooks" as well.
> 
> "Pre-patch" and "post-unpatch" are a bit wordy, but a good description.
> I already felt it was important enough to document the order of
> operations in the doc file and commit msg, so I like this idea. 
> 
> > For the enable case, I think it would be a nice feature if we checked
> > the return code and aborted the patching operation on error.  I think
> > that should be easy enough.
> 
> Yeah, that should be easy.  To be specific, you're only talking about
> the patching operation on the associated klp_object, not the entire
> klp_patch right?

Oh, right, I forgot about modules.  We can't stop the module from
loading, so forget that.  Maybe the load hook should just return void.

> > For the unload case, it's too late to do anything, so I'd say a void
> > return code would be better.  Otherwise it implies that we actually do
> > something about it.  Maybe in that case we can leave it up to the user
> > to decide whether to print an error or WARN() or whatever.
> 
> Good point.  I can change that in v2.

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Petr Mladek]

On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
> When the livepatch core executes klp_(un)patch_object, call out to a
> livepatch-module specified array of callback hooks.  These hooks provide
> a notification mechanism for livepatch modules when klp_objects are
> (un)patching. This may be most interesting when another kernel module
> is a klp_object target and the livepatch module needs to execute code
> after the target is loaded, but before its module_init code is run.
> 
> The patch-hook executes right before patching objects and the
> unpatch-hook executes right after unpatching objects.
> 
> diff --git a/Documentation/livepatch/hooks.txt b/Documentation/livepatch/hooks.txt
> new file mode 100644
> index 000000000000..ef18101a3b90
> --- /dev/null
> +++ b/Documentation/livepatch/hooks.txt
> @@ -0,0 +1,98 @@
> +(Un)patching Hooks
> +==================
> +
> +Livepatching (un)patch-hooks provide a mechanism to register and execute
> +a set of callback functions when the kernel's livepatching core performs
> +an (un)patching operation on a given kernel object.

The above is correct but it is a bit hard to understand what it really
means. Josh's discussion about the naming suggests that I am not the only
one who is confused ;-)

We need to make it clear that there are 4 basic situations
where these hooks are called:

  + patch hook is called when:

	1. livepatch is being enabled and object is loaded
	2. livepatch is enabled and object is being loaded

  + unpatch hook is called when

	3. livepatch is enabled and object is being removed
	4. livepatch is being disabled and object is loaded

Note that this document mostly talks only about the two situations
when the livepatch is enabled and the patched object is being
loaded or removed.

But it is still quite tricky to understand what can be modified
a safe way. We need to be careful about different things
in the different situations.

If the patched object is beeing added/removed, we know that its
code is not being used but the code from the rest of the patch
is already in use. The module is not yet or not longer properly
initialized. Therefore it might be too early or too late to
register or unregister any of its services in the rest of
the system. Basically it limits the changes only to
to the object (module) itself.

If the patch is being enabled, it is another story. The object
is already initialized and its old code is used but the new
code from the patch is not yet or not longer used. It suggests
that it might be safe to do some changes related to the
new code in the patch. But we need to be careful because
the system is using the old code.


But there are actually 4 more situations. If we use the consistency
model, different parts of the system might use different code.
I mean that:

  + patch hook is called also when:

     + livepatch is being enabled and object is being loaded
     + livepatch is being disabled and object is being loaded

  + unpatch hook is called when:

     + livepatch is being enabled and object is being removed
     + livepatch is being disabled and object is being removed


It is a bit easier if you run the hook for vmlinux
because it is always running.

I am sorry for the long mail. But I have really troubles to
understand and describe what can be done with these hooks
a safe way.

It might help if you share some real-life examples.


> +The hooks are provided and registered by a livepatch module as part of
> +klp_objects that make up its klp_patch structure.  Both patch and
> +unpatch-hook function signatures accept a pointer to a klp_object
> +argument and return an integer status, ie:

I would put this into separate section and make it clear
that it is a sample code.

> +  static int patch_hook(struct klp_object *obj)
> +  {
> +  	/* ... */
> +  }
> +  static int unpatch_hook(struct klp_object *obj)
> +  {
> +  	/* ... */
> +  }
> +
> +  static struct klp_hook patch_hooks[] = {
> +  	{
> +  		.hook = patch_hook,
> +  	}, { }
> +  };
> +  static struct klp_hook unpatch_hooks[] = {
> +  	{
> +  		.hook = unpatch_hook,
> +  	}, { }
> +  };
> +
> +  static struct klp_object objs[] = {
> +  	{
> +  		/* ... */
> +  		.patch_hooks = patch_hooks,
> +  		.unpatch_hooks = unpatch_hooks,
> +  	}, { }
> +  };
> +
> +  static struct klp_patch patch = {
> +  	.mod = THIS_MODULE,
> +  	.objs = objs,
> +  };
> +
> +If a hook returns non-zero status, the livepatching core will log a
> +hook failure warning message.

> +Multiple (un)patch-hooks may be registered per klp_object.  Each hook
> +will execute regardless of any previously executed hook's non-zero
> +return status.

We should pass the error down the stack. If will prevent either the
patch or the patched module of being loaded. Of course, we could
not do much if the patch or the patched object is being removed.


> +Hooks are optional.  The livepatching core will not execute any
> +callbacks for an empty klp_hook.hook array or a NULL klp_hook.hook
> +value.
> +
> +
> +For module targets
> +------------------
> +
> +On the other hand, if a target kernel module is already present when a
> +livepatch is loading, then the corresponding patch hook(s) will execute
> +as soon as the livepatching kernel core enables the livepatch.
>
> +It may be useful for hooks to inspect the module state of the klp_object
> +it is passed (i.e. obj->mod->state).  Patch hooks can expect to see
> +modules in MODULE_STATE_LIVE and MODULE_STATE_COMING states.  Unpatch
> +hooks can expect modules in MODULE_STATE_LIVE and MODULE_STATE_GOING
> +states.

This actualy talks about the other situations but it is well hidden
and kind of cryptic.


> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..ff3685470057 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -49,11 +49,6 @@
>  
>  static struct kobject *klp_root_kobj;
>  
> -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;
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 52c4e907c14b..c8084a18ddb7 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -235,25 +235,60 @@ static int klp_patch_func(struct klp_func *func)
>  	return ret;
>  }
>  
> +/**
> + * klp_run_hook - execute a given klp_hook callback
> + * @hook:	callback hook
> + * @obj:	kernel object that has been hooked
> + *
> + * Return: return value from hook, or 0 if none is currently associated
> + */
> +static int klp_run_hook(struct klp_hook *hook, struct klp_object *obj)
> +{
> +	if (hook && hook->hook)
> +		return (*hook->hook)(obj);
> +
> +	return 0;
> +}
> +
>  void klp_unpatch_object(struct klp_object *obj)
>  {
>  	struct klp_func *func;
> +	struct klp_hook *hook;
> +	int ret;
>  
>  	klp_for_each_func(obj, func)
>  		if (func->patched)
>  			klp_unpatch_func(func);
>  
>  	obj->patched = false;
>
> +
> +	klp_for_each_unpatch_hook(obj, hook) {
> +		ret = klp_run_hook(hook, obj);
> +		if (ret) {
> +			pr_warn("unpatch hook '%p' failed for object '%s'\n",
> +				hook, klp_is_module(obj) ? obj->name : "vmlinux");
> +		}
> +	}
> +

It probably does not matter but I would move

	obj->patched = false;

here. Otherwise, the hooks will see "false" in both cases. In each,
case it looks more symetric.

Or is there any reason behind the given order?

>  }
>  
>  int klp_patch_object(struct klp_object *obj)
>  {
>  	struct klp_func *func;
> +	struct klp_hook *hook;
>  	int ret;
>  
>  	if (WARN_ON(obj->patched))
>  		return -EINVAL;
>  
> +	klp_for_each_patch_hook(obj, hook) {
> +		ret = klp_run_hook(hook, obj);
> +		if (ret) {
> +			pr_warn("patch hook '%p' failed for object '%s'\n",
> +				hook, klp_is_module(obj) ? obj->name : "vmlinux");
> +		}
> +	}
> +
> +
>  	klp_for_each_func(obj, func) {
>  		ret = klp_patch_func(func);
>  		if (ret) {


Thaks a lot for looking at this. I guess that it is useful. But
it is also pretty dangerous at the same moment. I would like to
understand all consequences/usecases before we add this API.

Best Regards,
Petr

Re: [PATCH] livepatch: add (un)patch hooks

[Miroslav Benes]

> > > For the enable case, I think it would be a nice feature if we checked
> > > the return code and aborted the patching operation on error.  I think
> > > that should be easy enough.
> > 
> > Yeah, that should be easy.  To be specific, you're only talking about
> > the patching operation on the associated klp_object, not the entire
> > klp_patch right?
> 
> Oh, right, I forgot about modules.  We can't stop the module from
> loading, so forget that.  Maybe the load hook should just return void.

We can. We can stop a patch module from being loaded if there is an error 
coming from a hook, or we can stop a patched module (patch module is 
loaded) from being loaded. The latter case is not very user-friendly. We 
have force_load_module sysfs attribute for exactly these cases (if an 
admin is sure he can safely do that). It is not nice though.
 
> > > For the unload case, it's too late to do anything, so I'd say a void
> > > return code would be better.  Otherwise it implies that we actually do
> > > something about it.  Maybe in that case we can leave it up to the user
> > > to decide whether to print an error or WARN() or whatever.
> > 
> > Good point.  I can change that in v2.

I agree with this point.

Miroslav

Re: [PATCH] livepatch: add (un)patch hooks

[Miroslav Benes]

On Thu, 13 Jul 2017, Josh Poimboeuf wrote:

> On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> > When the livepatch core executes klp_(un)patch_object, call out to a
> > livepatch-module specified array of callback hooks.  These hooks provide
> > a notification mechanism for livepatch modules when klp_objects are
> > (un)patching. This may be most interesting when another kernel module
> > is a klp_object target and the livepatch module needs to execute code
> > after the target is loaded, but before its module_init code is run.
> 
> And it's also useful for vmlinux.  Patch module load/unload is separate
> from enable/disable, so the module init/exit functions can't be used for
> patch-specific changes (e.g., global data changes).

I admit that I don't understand this, which is probably the reason for my 
question. Why do we need it when we have module notifiers and module 
init/exit functions in the kernel? Petr described different possible 
scenarios and they can be solved either in init/exit function of a patch 
module or in a module notifier which the patch module can register.

If there is a difference, it should be mentioned in the documentation and 
in the changelog.
 
> > The patch-hook executes right before patching objects and the
> > unpatch-hook executes right after unpatching objects.
> > 
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> 
> Thanks for posting it.  We found this to be a useful feature in the
> past, not quite as useful as shadow data, but still good to have for
> certain cases.
> 
> Instead of "load hooks" I think it would be more accurate to call them
> "enable/disable hooks".  (Maybe "callbacks" would be better than
> "hooks"?  Not sure...)
> 
> Even better, we might want to be specific: "pre enable hooks" and "post
> disable hooks".  (Or "pre patch hooks" and "post unpatch hooks"?)
> Because we might eventually decide we need the corresponding "post
> enable hooks" and "pre disable hooks" as well.

And this is what I'm worried about. I think we don't want to have hooks 
sprinkled here and there in the code.

Thanks,
Miroslav

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Tue, Jul 18, 2017 at 01:15:16PM +0200, Miroslav Benes wrote:
> On Thu, 13 Jul 2017, Josh Poimboeuf wrote:
> 
> > On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> > > When the livepatch core executes klp_(un)patch_object, call out to a
> > > livepatch-module specified array of callback hooks.  These hooks provide
> > > a notification mechanism for livepatch modules when klp_objects are
> > > (un)patching. This may be most interesting when another kernel module
> > > is a klp_object target and the livepatch module needs to execute code
> > > after the target is loaded, but before its module_init code is run.
> > 
> > And it's also useful for vmlinux.  Patch module load/unload is separate
> > from enable/disable, so the module init/exit functions can't be used for
> > patch-specific changes (e.g., global data changes).
> 
> I admit that I don't understand this, which is probably the reason for my 
> question. Why do we need it when we have module notifiers and module 
> init/exit functions in the kernel? Petr described different possible 
> scenarios and they can be solved either in init/exit function of a patch 
> module or in a module notifier which the patch module can register.
> 
> If there is a difference, it should be mentioned in the documentation and 
> in the changelog.

Some differences:

- The patch module init/exit code doesn't run when disabling and
  re-enabling a patch.

- The module notifier can't stop the to-be-patched module from loading.

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Petr Mladek]

On Tue 2017-07-18 21:08:57, Josh Poimboeuf wrote:
> On Tue, Jul 18, 2017 at 01:15:16PM +0200, Miroslav Benes wrote:
> > On Thu, 13 Jul 2017, Josh Poimboeuf wrote:
> > 
> > > On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> > > > When the livepatch core executes klp_(un)patch_object, call out to a
> > > > livepatch-module specified array of callback hooks.  These hooks provide
> > > > a notification mechanism for livepatch modules when klp_objects are
> > > > (un)patching. This may be most interesting when another kernel module
> > > > is a klp_object target and the livepatch module needs to execute code
> > > > after the target is loaded, but before its module_init code is run.
> > > 
> > > And it's also useful for vmlinux.  Patch module load/unload is separate
> > > from enable/disable, so the module init/exit functions can't be used for
> > > patch-specific changes (e.g., global data changes).
> > 
> > I admit that I don't understand this, which is probably the reason for my 
> > question. Why do we need it when we have module notifiers and module 
> > init/exit functions in the kernel? Petr described different possible 
> > scenarios and they can be solved either in init/exit function of a patch 
> > module or in a module notifier which the patch module can register.
> > 
> > If there is a difference, it should be mentioned in the documentation and 
> > in the changelog.
> 
> Some differences:
> 
> - The patch module init/exit code doesn't run when disabling and
>   re-enabling a patch.

True. Well, I would still like to see some real life examples
where this can be used.

My problem with the callbacks is that the same code runs in too
many situations and each situation has different constrains.
IMHO, it is much more complicated to say what is safe and what
is not. See
https://lkml.kernel.org/r/20170717155144.GF32632@pathway.suse.cz
for more details.


> - The module notifier can't stop the to-be-patched module from loading.

Good point.

Best Regards,
Petr

Re: [PATCH] livepatch: add (un)patch hooks

[Joe Lawrence]

On 07/17/2017 11:51 AM, Petr Mladek wrote:
> On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
>> When the livepatch core executes klp_(un)patch_object, call out to a
>> livepatch-module specified array of callback hooks.  These hooks provide
>> a notification mechanism for livepatch modules when klp_objects are
>> (un)patching. This may be most interesting when another kernel module
>> is a klp_object target and the livepatch module needs to execute code
>> after the target is loaded, but before its module_init code is run.
>>
>> The patch-hook executes right before patching objects and the
>> unpatch-hook executes right after unpatching objects.
>>
>> diff --git a/Documentation/livepatch/hooks.txt b/Documentation/livepatch/hooks.txt
>> new file mode 100644
>> index 000000000000..ef18101a3b90
>> --- /dev/null
>> +++ b/Documentation/livepatch/hooks.txt
>> @@ -0,0 +1,98 @@
>> +(Un)patching Hooks
>> +==================
>> +
>> +Livepatching (un)patch-hooks provide a mechanism to register and execute
>> +a set of callback functions when the kernel's livepatching core performs
>> +an (un)patching operation on a given kernel object.
> 
> The above is correct but it is a bit hard to understand what it really
> means. Josh's discussion about the naming suggests that I am not the only
> one who is confused ;-)
> 
> We need to make it clear that there are 4 basic situations
> where these hooks are called:
> 
>   + patch hook is called when:
> 
> 	1. livepatch is being enabled and object is loaded
> 	2. livepatch is enabled and object is being loaded
> 
>   + unpatch hook is called when
> 
> 	3. livepatch is enabled and object is being removed
> 	4. livepatch is being disabled and object is loaded
> 
> Note that this document mostly talks only about the two situations
> when the livepatch is enabled and the patched object is being
> loaded or removed.
> 
> But it is still quite tricky to understand what can be modified
> a safe way. We need to be careful about different things
> in the different situations.

Indeed, this is tricky.

> If the patched object is beeing added/removed, we know that its
> code is not being used but the code from the rest of the patch
> is already in use. The module is not yet or not longer properly
> initialized. Therefore it might be too early or too late to
> register or unregister any of its services in the rest of
> the system. Basically it limits the changes only to
> to the object (module) itself.
> 
> If the patch is being enabled, it is another story. The object
> is already initialized and its old code is used but the new
> code from the patch is not yet or not longer used. It suggests
> that it might be safe to do some changes related to the
> new code in the patch. But we need to be careful because
> the system is using the old code.
> 
> 
> But there are actually 4 more situations. If we use the consistency
> model, different parts of the system might use different code.
> I mean that:
> 
>   + patch hook is called also when:
> 
>      + livepatch is being enabled and object is being loaded
>      + livepatch is being disabled and object is being loaded
> 
>   + unpatch hook is called when:
> 
>      + livepatch is being enabled and object is being removed
>      + livepatch is being disabled and object is being removed
> 
> 
> It is a bit easier if you run the hook for vmlinux
> because it is always running.
> 
> I am sorry for the long mail. But I have really troubles to
> understand and describe what can be done with these hooks
> a safe way.

If you look at it from the inside out, the concept can be boiled down to
"kernel object patching notifiers".  Essentially whenever a klp_object
is being patched or unpatched, the callbacks are executed.

In this implementation, the patch-hook provides the callback a context
of "your object's patched code is loaded, but not active yet".
Likewise, the unpatch-hook would be "your patched code is no longer
active, but not unloaded (yet)".

I agree that explaining the patch-at-large context quickly gets
complicated.  IMHO, I think some of that discussion belongs in
Documentation/livepatch/livepatch.txt, in a section that answers, "just
when does code get patched?".  In my mind, these notifiers build on top
of the answer to that question.

That said, some kind of guidance about the various contexts in which
these hooks are executed is definitely warranted.  Perhaps some kind of
table or list can summarize things, let me think more on that.

> It might help if you share some real-life examples.

Well, kpatch utilizes "load hooks" and their application are somewhat
specific to that system.  Livepatch (and the consistency model)
introduce a more complicated world -- as you pointed out, for example,
old code and new code may be simultaneously running.

The traditional kpatch load hook example given [1] is updating some data
structure:

[1]
https://github.com/dynup/kpatch/blob/master/doc/patch-author-guide.md#use-a-kpatch-load-hook

however, I think a livepatch counterpart needs to consider a bit more:
locking, unpatched/patched code access, etc.

> 
>> +The hooks are provided and registered by a livepatch module as part of
>> +klp_objects that make up its klp_patch structure.  Both patch and
>> +unpatch-hook function signatures accept a pointer to a klp_object
>> +argument and return an integer status, ie:
> 
> I would put this into separate section and make it clear
> that it is a sample code.

It wasn't exactly /example/ code, it just seemed more succinct to
provide some /pseudo/ code to explain the data structure relationships.

If it is redundant then I can remove from v2 ... if it were any more
"working" code, then it would be nearly as elaborate as the provided
sample module. :)  Does it deserve its own "Data structure relationship
/ pseudo code" section?

>> +  static int patch_hook(struct klp_object *obj)
>> +  {
>> +  	/* ... */
>> +  }
>> +  static int unpatch_hook(struct klp_object *obj)
>> +  {
>> +  	/* ... */
>> +  }
>> +
>> +  static struct klp_hook patch_hooks[] = {
>> +  	{
>> +  		.hook = patch_hook,
>> +  	}, { }
>> +  };
>> +  static struct klp_hook unpatch_hooks[] = {
>> +  	{
>> +  		.hook = unpatch_hook,
>> +  	}, { }
>> +  };
>> +
>> +  static struct klp_object objs[] = {
>> +  	{
>> +  		/* ... */
>> +  		.patch_hooks = patch_hooks,
>> +  		.unpatch_hooks = unpatch_hooks,
>> +  	}, { }
>> +  };
>> +
>> +  static struct klp_patch patch = {
>> +  	.mod = THIS_MODULE,
>> +  	.objs = objs,
>> +  };
>> +
>> +If a hook returns non-zero status, the livepatching core will log a
>> +hook failure warning message.
> 
>> +Multiple (un)patch-hooks may be registered per klp_object.  Each hook
>> +will execute regardless of any previously executed hook's non-zero
>> +return status.
> 
> We should pass the error down the stack. If will prevent either the
> patch or the patched module of being loaded. Of course, we could
> not do much if the patch or the patched object is being removed.

I know Josh and Miroslav pointed this out elsewhere in the thread,
but I don't know how I feel about using the hooks to affect the
patch(ed) module loading.  Making these "void" feels simpler IMHO.

>> +Hooks are optional.  The livepatching core will not execute any
>> +callbacks for an empty klp_hook.hook array or a NULL klp_hook.hook
>> +value.
>> +
>> +
>> +For module targets
>> +------------------
>> +
>> +On the other hand, if a target kernel module is already present when a
>> +livepatch is loading, then the corresponding patch hook(s) will execute
>> +as soon as the livepatching kernel core enables the livepatch.
>>
>> +It may be useful for hooks to inspect the module state of the klp_object
>> +it is passed (i.e. obj->mod->state).  Patch hooks can expect to see
>> +modules in MODULE_STATE_LIVE and MODULE_STATE_COMING states.  Unpatch
>> +hooks can expect modules in MODULE_STATE_LIVE and MODULE_STATE_GOING
>> +states.
> 
> This actualy talks about the other situations but it is well hidden
> and kind of cryptic.

Its placement seemed logical in the progression from intro -> easy cases
-> complicated cases.  The ramifications of what it means to be called
when the klp_object->mod is MODULE_STATE_FOO is definitely understated
and could probably use more attention.

I think this goes back to creating some kind of table of scenarios.  I
can try to compile this for v2.

> 
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index b9628e43c78f..ff3685470057 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -49,11 +49,6 @@
>>  
>>  static struct kobject *klp_root_kobj;
>>  
>> -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;
>> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
>> index 52c4e907c14b..c8084a18ddb7 100644
>> --- a/kernel/livepatch/patch.c
>> +++ b/kernel/livepatch/patch.c
>> @@ -235,25 +235,60 @@ static int klp_patch_func(struct klp_func *func)
>>  	return ret;
>>  }
>>  
>> +/**
>> + * klp_run_hook - execute a given klp_hook callback
>> + * @hook:	callback hook
>> + * @obj:	kernel object that has been hooked
>> + *
>> + * Return: return value from hook, or 0 if none is currently associated
>> + */
>> +static int klp_run_hook(struct klp_hook *hook, struct klp_object *obj)
>> +{
>> +	if (hook && hook->hook)
>> +		return (*hook->hook)(obj);
>> +
>> +	return 0;
>> +}
>> +
>>  void klp_unpatch_object(struct klp_object *obj)
>>  {
>>  	struct klp_func *func;
>> +	struct klp_hook *hook;
>> +	int ret;
>>  
>>  	klp_for_each_func(obj, func)
>>  		if (func->patched)
>>  			klp_unpatch_func(func);
>>  
>>  	obj->patched = false;
>>
>> +
>> +	klp_for_each_unpatch_hook(obj, hook) {
>> +		ret = klp_run_hook(hook, obj);
>> +		if (ret) {
>> +			pr_warn("unpatch hook '%p' failed for object '%s'\n",
>> +				hook, klp_is_module(obj) ? obj->name : "vmlinux");
>> +		}
>> +	}
>> +
> 
> It probably does not matter but I would move
> 
> 	obj->patched = false;
> 
> here. Otherwise, the hooks will see "false" in both cases. In each,
> case it looks more symetric.
> 
> Or is there any reason behind the given order?

klp_unpatch_func() was just executed for all klp_object functions, so
IMHO a value of obj->patched = false reflects reality.  Is this not
symmetric:

klp_patch_object()
  klp_for_each_patch_hook     |
    klp_run_hook              | A - hooks

  klp_for_each_func           |
    klp_patch_func            |
  obj->patched = true         | B - patching

klp_unpatch_object()
  klp_for_each_func           |
    klp_unpatch_func          |
  obj->patched = false        | B' - unpatching

  klp_for_each_unpatch_hook   |
    klp_run_hook              | A' - hooks

I didn't really have a strong preference here... only for what would be
most intuitive.

>>  }
>>  
>>  int klp_patch_object(struct klp_object *obj)
>>  {
>>  	struct klp_func *func;
>> +	struct klp_hook *hook;
>>  	int ret;
>>  
>>  	if (WARN_ON(obj->patched))
>>  		return -EINVAL;
>>  
>> +	klp_for_each_patch_hook(obj, hook) {
>> +		ret = klp_run_hook(hook, obj);
>> +		if (ret) {
>> +			pr_warn("patch hook '%p' failed for object '%s'\n",
>> +				hook, klp_is_module(obj) ? obj->name : "vmlinux");
>> +		}
>> +	}
>> +
>> +
>>  	klp_for_each_func(obj, func) {
>>  		ret = klp_patch_func(func);
>>  		if (ret) {
> 
> 
> Thaks a lot for looking at this. I guess that it is useful. But
> it is also pretty dangerous at the same moment. I would like to
> understand all consequences/usecases before we add this API.

Thanks for reviewing and I appreciate the feedback, especially the dark
corners of enabling vs. loading and what would be safe when.

Regards,

-- Joe

Re: [PATCH] livepatch: add (un)patch hooks

[Miroslav Benes]

On Tue, 18 Jul 2017, Josh Poimboeuf wrote:

> On Tue, Jul 18, 2017 at 01:15:16PM +0200, Miroslav Benes wrote:
> > On Thu, 13 Jul 2017, Josh Poimboeuf wrote:
> > 
> > > On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote:
> > > > When the livepatch core executes klp_(un)patch_object, call out to a
> > > > livepatch-module specified array of callback hooks.  These hooks provide
> > > > a notification mechanism for livepatch modules when klp_objects are
> > > > (un)patching. This may be most interesting when another kernel module
> > > > is a klp_object target and the livepatch module needs to execute code
> > > > after the target is loaded, but before its module_init code is run.
> > > 
> > > And it's also useful for vmlinux.  Patch module load/unload is separate
> > > from enable/disable, so the module init/exit functions can't be used for
> > > patch-specific changes (e.g., global data changes).
> > 
> > I admit that I don't understand this, which is probably the reason for my 
> > question. Why do we need it when we have module notifiers and module 
> > init/exit functions in the kernel? Petr described different possible 
> > scenarios and they can be solved either in init/exit function of a patch 
> > module or in a module notifier which the patch module can register.
> > 
> > If there is a difference, it should be mentioned in the documentation and 
> > in the changelog.
> 
> Some differences:
> 
> - The patch module init/exit code doesn't run when disabling and
>   re-enabling a patch.
> 
> - The module notifier can't stop the to-be-patched module from loading.

Ah, right. Thanks.

Joe, could you add both points to the changelog and the documentation, 
please?

Miroslav

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Mon, Jul 17, 2017 at 05:51:44PM +0200, Petr Mladek wrote:
> On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
> > When the livepatch core executes klp_(un)patch_object, call out to a
> > livepatch-module specified array of callback hooks.  These hooks provide
> > a notification mechanism for livepatch modules when klp_objects are
> > (un)patching. This may be most interesting when another kernel module
> > is a klp_object target and the livepatch module needs to execute code
> > after the target is loaded, but before its module_init code is run.
> > 
> > The patch-hook executes right before patching objects and the
> > unpatch-hook executes right after unpatching objects.
> > 
> > diff --git a/Documentation/livepatch/hooks.txt b/Documentation/livepatch/hooks.txt
> > new file mode 100644
> > index 000000000000..ef18101a3b90
> > --- /dev/null
> > +++ b/Documentation/livepatch/hooks.txt
> > @@ -0,0 +1,98 @@
> > +(Un)patching Hooks
> > +==================
> > +
> > +Livepatching (un)patch-hooks provide a mechanism to register and execute
> > +a set of callback functions when the kernel's livepatching core performs
> > +an (un)patching operation on a given kernel object.
> 
> The above is correct but it is a bit hard to understand what it really
> means. Josh's discussion about the naming suggests that I am not the only
> one who is confused ;-)
> 
> We need to make it clear that there are 4 basic situations
> where these hooks are called:
> 
>   + patch hook is called when:
> 
> 	1. livepatch is being enabled and object is loaded
> 	2. livepatch is enabled and object is being loaded

These could be stated much simpler:

	1. the object is about to be patched

>   + unpatch hook is called when
> 
> 	3. livepatch is enabled and object is being removed
> 	4. livepatch is being disabled and object is loaded

And this one too:

	2. the object was just unpatched

The rest are just unnecessary details IMO.  When writing a hook for a
particular object, the patch author shouldn't need to care about the
other objects and whether they're patched or unpatched.

Maybe we just need a warning/reminder in the documentation that this
hook is specific to the given object, and other objects could be patched
or unpatched irrespective of the target object's state.

> Note that this document mostly talks only about the two situations
> when the livepatch is enabled and the patched object is being
> loaded or removed.
> 
> But it is still quite tricky to understand what can be modified
> a safe way. We need to be careful about different things
> in the different situations.
> 
> If the patched object is beeing added/removed, we know that its
> code is not being used but the code from the rest of the patch
> is already in use. The module is not yet or not longer properly
> initialized. Therefore it might be too early or too late to
> register or unregister any of its services in the rest of
> the system. Basically it limits the changes only to
> to the object (module) itself.
> 
> If the patch is being enabled, it is another story. The object
> is already initialized and its old code is used but the new
> code from the patch is not yet or not longer used. It suggests
> that it might be safe to do some changes related to the
> new code in the patch. But we need to be careful because
> the system is using the old code.
> 
> 
> But there are actually 4 more situations. If we use the consistency
> model, different parts of the system might use different code.
> I mean that:
> 
>   + patch hook is called also when:
> 
>      + livepatch is being enabled and object is being loaded
>      + livepatch is being disabled and object is being loaded
> 
>   + unpatch hook is called when:
> 
>      + livepatch is being enabled and object is being removed
>      + livepatch is being disabled and object is being removed
> 
> 
> It is a bit easier if you run the hook for vmlinux
> because it is always running.

Again I think this is all overthinking it.  The patch hook should be
specific to the object.  It shouldn't make assumptions about other
objects.

> I am sorry for the long mail. But I have really troubles to
> understand and describe what can be done with these hooks
> a safe way.
> 
> It might help if you share some real-life examples.

Agreed, we should share some real world examples.  For a few cases, load
hooks were extremely useful.  But most of our experience has been with
the kpatch consistency model, so we need to revisit our past findings
and view them through the livepatch lens.

One crazy -- but potentially very useful -- idea would be if the user
were allowed to run stop_machine() from the load hook.  If possible,
that would help prevent a lot of race conditions.

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Wed, Jul 19, 2017 at 03:49:52PM -0500, Josh Poimboeuf wrote:
> > I am sorry for the long mail. But I have really troubles to
> > understand and describe what can be done with these hooks
> > a safe way.
> > 
> > It might help if you share some real-life examples.
> 
> Agreed, we should share some real world examples.  For a few cases, load
> hooks were extremely useful.  But most of our experience has been with
> the kpatch consistency model, so we need to revisit our past findings
> and view them through the livepatch lens.
> 
> One crazy -- but potentially very useful -- idea would be if the user
> were allowed to run stop_machine() from the load hook.  If possible,
> that would help prevent a lot of race conditions.

To try to give us all a better idea of what's needed, here are some of
the patches that Joe and I looked at before which seem to need load
hooks.  A few of these are ones we actually delivered to customers with
kpatch.  I've tried to re-analyze them in light of the livepatch CM.


First, a few observations:

- Load hooks are a power feature.  They're live patching in "hard mode".
  But they're also very powerful.  Luckily I think they're only needed
  in a few cases, probably < 5% of patches.

- In general, the hooks seem to be useful for cases like:

  - global data updates

  - "patches" to __init and probe functions

  - patching otherwise unpatchable code (i.e., assembly)

  In many/most cases, it seems like stop_machine() would be very useful
  to avoid concurrency issues.

- The more examples I look at, the more I'm thinking we will need both
  pre-patch and post-patch hooks, as well as pre-unpatch and
  post-unpatch hooks.

- The pre-patch and pre-unpatch hooks can be run before the
  patching/unpatching process begins.

- The post-patch and post-unpatch hooks will need to be run from either
  klp_complete_transition() or klp_module_coming/going(), depending on
  whether the to-be-patched module is already loaded or is being
  loaded/unloaded.


Here's a simple example:

  75ff39ccc1bd ("tcp: make challenge acks less predictable")

It involves changing a global sysctl, as well as a patch to the
tcp_send_challenge_ack() function.  We used load hooks to change the
sysctl.

In this case, if we're being super paranoid, it might make sense to
patch the data *after* patching is complete (i.e., a post-patch hook),
so that tcp_send_challenge_ack() could first be changed to read
sysctl_tcp_challenge_ack_limit with READ_ONCE.  But I think the race is
harmless (and such a race already exists in that function with respect
to sysctl writes anyway).

Another way of dealing with concurrency would be to use stop_machine()
in the load hook.


Another example:

  48900cb6af42 ("virtio-net: drop NETIF_F_FRAGLIST")

That changes the net_device features in a driver probe function.  I
don't know exactly how to patch it, but if it's possible, I'm pretty
sure load hooks is the way to do it :-)


Another one:

  54a20552e1ea ("KVM: x86: work around infinite loop in microcode when #AC is delivered")

Again, I have no idea how to do it, but I'd bet that load hooks are
involved.


This one was interesting:

  6f442be2fb22 ("x86_64, traps: Stop using IST for #SS")

A livepatch patch for it is below.  We had something similar for kpatch.
The below patch is completely untested because we don't have
kpatch-build tooling support for livepatch hooks yet.

Note that the load hook would need to run *after* the patch has been
applied and the transition has completed.  And also, it would need to
run inside stop_machine().  I didn't put that in the patch yet.  But it
should at least give you an idea.


diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 819662746e23..68fe9d5f1c22 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -236,6 +236,7 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
 #ifdef CONFIG_X86_32
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
 #endif
+DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment_v2)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
 
 #ifdef CONFIG_X86_64
diff --git a/fs/proc/cmdline.c b/fs/proc/cmdline.c
index cbd82dff7e81..504b01dea937 100644
--- a/fs/proc/cmdline.c
+++ b/fs/proc/cmdline.c
@@ -27,3 +27,42 @@ static int __init proc_cmdline_init(void)
 	return 0;
 }
 fs_initcall(proc_cmdline_init);
+
+
+#include "kpatch-macros.h"
+#include <asm/traps.h>
+#include <asm/desc.h>
+#include <asm/cacheflush.h>
+#define trace_stack_segment_v2 stack_segment_v2
+
+static void swapgs_load_hook(void)
+{
+	/* bug doesn't exist on xen */
+	if (paravirt_enabled() && strcmp(pv_info.name, "KVM"))
+		return;
+
+	write_cr0(read_cr0() & ~X86_CR0_WP);
+	barrier();
+
+	/* disable IST for #SS */
+	set_intr_gate(X86_TRAP_SS, stack_segment_v2);
+
+	barrier();
+	write_cr0(read_cr0() | X86_CR0_WP);
+}
+KLP_LOAD_HOOK(swapgs_load_hook);
+
+static void swapgs_unload_hook(void)
+{
+	if (paravirt_enabled() && strcmp(pv_info.name, "KVM"))
+		return;
+
+	write_cr0(read_cr0() & ~X86_CR0_WP);
+	barrier();
+
+	set_intr_gate_ist(X86_TRAP_SS, stack_segment_v2, STACKFAULT_STACK);
+
+	barrier();
+	write_cr0(read_cr0() | X86_CR0_WP);
+}
+KLP_UNLOAD_HOOK(swapgs_unload_hook);

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Wed, Jul 19, 2017 at 11:17:23PM -0500, Josh Poimboeuf wrote:
> +static void swapgs_unload_hook(void)
> +{
> +	if (paravirt_enabled() && strcmp(pv_info.name, "KVM"))
> +		return;
> +
> +	write_cr0(read_cr0() & ~X86_CR0_WP);
> +	barrier();
> +
> +	set_intr_gate_ist(X86_TRAP_SS, stack_segment_v2, STACKFAULT_STACK);

s/stack_segment_v2/stack_segment/ for the unload hook

> +
> +	barrier();
> +	write_cr0(read_cr0() | X86_CR0_WP);
> +}
> +KLP_UNLOAD_HOOK(swapgs_unload_hook);

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Wed, Jul 19, 2017 at 11:17:23PM -0500, Josh Poimboeuf wrote:
> This one was interesting:
> 
>   6f442be2fb22 ("x86_64, traps: Stop using IST for #SS")
> 
> A livepatch patch for it is below.  We had something similar for kpatch.
> The below patch is completely untested because we don't have
> kpatch-build tooling support for livepatch hooks yet.
> 
> Note that the load hook would need to run *after* the patch has been
> applied and the transition has completed.  And also, it would need to
> run inside stop_machine().  I didn't put that in the patch yet.  But it
> should at least give you an idea.

Actually the statement that it needs to run after the patch is applied
isn't accurate.  It was true with an earlier revision of the patch which
modified the do_stack_segment() function.  But this version only adds
code.

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Petr Mladek]

On Wed 2017-07-19 14:59:48, Joe Lawrence wrote:
> On 07/17/2017 11:51 AM, Petr Mladek wrote:
> > On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
> > We need to make it clear that there are 4 basic situations
> > where these hooks are called:
> > 
> >   + patch hook is called when:
> > 
> > 	1. livepatch is being enabled and object is loaded
> > 	2. livepatch is enabled and object is being loaded
> > 
> >   + unpatch hook is called when
> > 
> > 	3. livepatch is enabled and object is being removed
> > 	4. livepatch is being disabled and object is loaded
> > 
> If you look at it from the inside out, the concept can be boiled down to
> "kernel object patching notifiers".  Essentially whenever a klp_object
> is being patched or unpatched, the callbacks are executed.

I see your point. I somewhat got the impression that feature
description primary talked about situations when the affected
object/module was loaded later than the livepatch. We could
change things more easily in this situations. It rang the bells
that the hooks would run also in other situations where we
would need to take more care.

I do not know if it is my personal problem or we might just
need to improve the documentation a bit. I wonder if the
section "For module targets" consfused me. There are 16
lines about one situations and 3 lines about the other.
Also the indented lines are more eye catching than the
rest.


> however, I think a livepatch counterpart needs to consider a bit more:
> locking, unpatched/patched code access, etc.
> 
> > 
> >> +The hooks are provided and registered by a livepatch module as part of
> >> +klp_objects that make up its klp_patch structure.  Both patch and
> >> +unpatch-hook function signatures accept a pointer to a klp_object
> >> +argument and return an integer status, ie:
> > 
> > I would put this into separate section and make it clear
> > that it is a sample code.
> 
> It wasn't exactly /example/ code, it just seemed more succinct to
> provide some /pseudo/ code to explain the data structure relationships.
> 
> If it is redundant then I can remove from v2

I think that it is fine. It is much easier to understand that
hundred of words.

I do not know. Maybe, it was just mentioned to early. Maybe,
I expected more details about the feature usefulness before
"implementation" details.

Maybe we could forget about this problem. I wrote about it
because I got a bit confused when I read the patch for
the first time. But it is a minor problem.


> >> +  static int patch_hook(struct klp_object *obj)
> >> +  {
> >> +  	/* ... */
> >> +  }
> >> +  static int unpatch_hook(struct klp_object *obj)
> >> +  {
> >> +  	/* ... */
> >> +  }
> >> +
> >> +  static struct klp_hook patch_hooks[] = {
> >> +  	{
> >> +  		.hook = patch_hook,
> >> +  	}, { }
> >> +  };
> >> +  static struct klp_hook unpatch_hooks[] = {
> >> +  	{
> >> +  		.hook = unpatch_hook,
> >> +  	}, { }
> >> +  };
> >> +
> >> +  static struct klp_object objs[] = {
> >> +  	{
> >> +  		/* ... */
> >> +  		.patch_hooks = patch_hooks,
> >> +  		.unpatch_hooks = unpatch_hooks,
> >> +  	}, { }
> >> +  };
> >> +
> >> +  static struct klp_patch patch = {
> >> +  	.mod = THIS_MODULE,
> >> +  	.objs = objs,
> >> +  };
> >> +
> >> +If a hook returns non-zero status, the livepatching core will log a
> >> +hook failure warning message.
> > 
> >> +Multiple (un)patch-hooks may be registered per klp_object.  Each hook
> >> +will execute regardless of any previously executed hook's non-zero
> >> +return status.
> > 
> > We should pass the error down the stack. If will prevent either the
> > patch or the patched module of being loaded. Of course, we could
> > not do much if the patch or the patched object is being removed.
> 
> I know Josh and Miroslav pointed this out elsewhere in the thread,
> but I don't know how I feel about using the hooks to affect the
> patch(ed) module loading.  Making these "void" feels simpler IMHO.

I think that the system safety is much more important than a
simplicity.

People use livepatches in situatution when the system restart is
too expensive. If we detect a problem and could stop the operation
before it causes some damage, it is very valuable. IMHO, this is
the case here.


> >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >> index b9628e43c78f..ff3685470057 100644
> >> --- a/kernel/livepatch/core.c
> >> +++ b/kernel/livepatch/core.c
> >> @@ -49,11 +49,6 @@
> >>  
> >>  static struct kobject *klp_root_kobj;
> >>  
> >> -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;
> >> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> >> index 52c4e907c14b..c8084a18ddb7 100644
> >> --- a/kernel/livepatch/patch.c
> >> +++ b/kernel/livepatch/patch.c
> >> @@ -235,25 +235,60 @@ static int klp_patch_func(struct klp_func *func)
> >>  	return ret;
> >>  }
> >>  
> >> +/**
> >> + * klp_run_hook - execute a given klp_hook callback
> >> + * @hook:	callback hook
> >> + * @obj:	kernel object that has been hooked
> >> + *
> >> + * Return: return value from hook, or 0 if none is currently associated
> >> + */
> >> +static int klp_run_hook(struct klp_hook *hook, struct klp_object *obj)
> >> +{
> >> +	if (hook && hook->hook)
> >> +		return (*hook->hook)(obj);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  void klp_unpatch_object(struct klp_object *obj)
> >>  {
> >>  	struct klp_func *func;
> >> +	struct klp_hook *hook;
> >> +	int ret;
> >>  
> >>  	klp_for_each_func(obj, func)
> >>  		if (func->patched)
> >>  			klp_unpatch_func(func);
> >>  
> >>  	obj->patched = false;
> >>
> >> +
> >> +	klp_for_each_unpatch_hook(obj, hook) {
> >> +		ret = klp_run_hook(hook, obj);
> >> +		if (ret) {
> >> +			pr_warn("unpatch hook '%p' failed for object '%s'\n",
> >> +				hook, klp_is_module(obj) ? obj->name : "vmlinux");
> >> +		}
> >> +	}
> >> +
> > 
> > It probably does not matter but I would move
> > 
> > 	obj->patched = false;
> > 
> > here. Otherwise, the hooks will see "false" in both cases. In each,
> > case it looks more symetric.
> > 
> > Or is there any reason behind the given order?
> 
> klp_unpatch_func() was just executed for all klp_object functions, so
> IMHO a value of obj->patched = false reflects reality.  Is this not
> symmetric:
> 
> klp_patch_object()
>   klp_for_each_patch_hook     |
>     klp_run_hook              | A - hooks
> 
>   klp_for_each_func           |
>     klp_patch_func            |
>   obj->patched = true         | B - patching
> 
> klp_unpatch_object()
>   klp_for_each_func           |
>     klp_unpatch_func          |
>   obj->patched = false        | B' - unpatching
> 
>   klp_for_each_unpatch_hook   |
>     klp_run_hook              | A' - hooks
> 
> I didn't really have a strong preference here... only for what would be
> most intuitive.

My feeling is that running the hooks is part of the patch
enabling/disabling process. They do thinks that cannot be
done by "simply" registering/unregistering the ftrace handlers.
Therefore the obj->patched state should reflect the state
of the hooks as well.

But again. This is a minor problem. The order is not really
important because everything is synchronized using the klp_mutex.

Best Regards,
Petr

Re: [PATCH] livepatch: add (un)patch hooks

[Petr Mladek]

On Wed 2017-07-19 23:17:23, Josh Poimboeuf wrote:
> On Wed, Jul 19, 2017 at 03:49:52PM -0500, Josh Poimboeuf wrote:
> > > I am sorry for the long mail. But I have really troubles to
> > > understand and describe what can be done with these hooks
> > > a safe way.
> > > 
> > > It might help if you share some real-life examples.
> > 
> > Agreed, we should share some real world examples.  For a few cases, load
> > hooks were extremely useful.  But most of our experience has been with
> > the kpatch consistency model, so we need to revisit our past findings
> > and view them through the livepatch lens.
> > 
> > One crazy -- but potentially very useful -- idea would be if the user
> > were allowed to run stop_machine() from the load hook.  If possible,
> > that would help prevent a lot of race conditions.
> 
> To try to give us all a better idea of what's needed, here are some of
> the patches that Joe and I looked at before which seem to need load
> hooks.  A few of these are ones we actually delivered to customers with
> kpatch.  I've tried to re-analyze them in light of the livepatch CM.
> 
> 
> First, a few observations:
> 
> - Load hooks are a power feature.  They're live patching
>   in "hard mode".

I really like this description of the feature!

The original description made me feel that it was something easy
to use and rather safe because the changes were done before
the object was patched or even loaded.

>From my point of view, creating a classic livepatch is relatively
easy. The old code is redirected to a fixed one. Where the fixed
one is more or less the same as the new "upstream" fixed code.

Of course, there are some catches, for example, the inline functions,
compiler optimization, semantic changes, functions that cannot be traced.
But all this is kind of discovered and described.

But the hooks allows to do anything. They require writing a custom
code that will never be used anywhere else. It usually has very
limited review if any.

The things gets more complicated when the new code provided by
the livepatch somehow depends on the effect of the hooks and vice
versa. Then the authors must know something about the consistency
models. They might deal with many problems (races) that we were
dealing when implementing the consistency models. And the livepatch
framework does not help much here. In fact, the hooks work in
the immediate mode even when the rest of the patch is applied
using the more careful consistency model.


> - In general, the hooks seem to be useful for cases like:
> 
>   - global data updates
> 
>   - "patches" to __init and probe functions

I think that this is similar to global data updates. The variables
are only harder to find.


>   - patching otherwise unpatchable code (i.e., assembly)
> 
>   In many/most cases, it seems like stop_machine() would be very useful
>   to avoid concurrency issues.

I am not sure if stop_machine() would help here. It would make sense
in kPatch where also the ftrace handlers are added during
stop_machine(). Then it is possible to synchronize both operations
(hooks, enabling ftrace handlers) and do everything "atomically".

IMHO, the big advantage of livepatch framework is that stop_machine()
is not needed. I hope that it will stay this way.

Also it might need some additional support. You would want to stop
the machine to make sure that it is safe to do a change. Then
we might need to check stacks, ...


> - The more examples I look at, the more I'm thinking we will need both
>   pre-patch and post-patch hooks, as well as pre-unpatch and
>   post-unpatch hooks.

Yup.


> - The pre-patch and pre-unpatch hooks can be run before the
>   patching/unpatching process begins.
> 
> - The post-patch and post-unpatch hooks will need to be run from either
>   klp_complete_transition() or klp_module_coming/going(), depending on
>   whether the to-be-patched module is already loaded or is being
>   loaded/unloaded.

Makes sense.

> 
> Here's a simple example:

Thanks a lot for the examples. I have got the idea.

I would formulate it the way that the hooks will allow to
patch/unpatch things that cannot be done by simply using
fixed code instead of the old one.

The use case is:

   + modification of global variables (even more instances)

   + registering newly available services/handlers
     (always in post handlers?)

People need to be careful:

   + synchronize the changes with the rest of the system

   + be aware when the new code from the livepatch is active
     (state of the transition, state of the object(module)


The advantage is that they:

   + allow to enable/disable the changes together with the patch
     (using immediate consistency model)

   + allow to handle both patch enable/disable and object
     load/unload in one place

   + they eventually allow to reject loading the patch
     or the affected object (module) in case of error.


Best Regards,
Petr

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Thu, Jul 20, 2017 at 05:50:04PM +0200, Petr Mladek wrote:
> On Wed 2017-07-19 23:17:23, Josh Poimboeuf wrote:
> >   - patching otherwise unpatchable code (i.e., assembly)
> > 
> >   In many/most cases, it seems like stop_machine() would be very useful
> >   to avoid concurrency issues.
> 
> I am not sure if stop_machine() would help here. It would make sense
> in kPatch where also the ftrace handlers are added during
> stop_machine(). Then it is possible to synchronize both operations
> (hooks, enabling ftrace handlers) and do everything "atomically".
> 
> IMHO, the big advantage of livepatch framework is that stop_machine()
> is not needed. I hope that it will stay this way.
> 
> Also it might need some additional support. You would want to stop
> the machine to make sure that it is safe to do a change. Then
> we might need to check stacks, ...

Don't worry.  I much prefer our current consistency model to kpatch, and
I have no intention of changing it :-)

That said, for the hooks, I still think stop_machine() will be helpful
in some cases where you need to ensure no other code is running.  Like
the stack_segment patch I posted, for example.

Anyway I'm not suggesting we do the stop_machine() in livepatch code
itself.  I'm just hoping it will work from a hook, in case the patch
author needs to do it as a last resort.

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Joe Lawrence]

On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
> - The pre-patch and pre-unpatch hooks can be run before the
>   patching/unpatching process begins.

Hi Josh,

By "(un)patching process" are you referring to the klp_patch at large or
each klp_object?  ie, would all klp_objects execute their hooks before
anything is (un)patched?  Just trying to clarify.

> - The post-patch and post-unpatch hooks will need to be run from either
>   klp_complete_transition() or klp_module_coming/going(), depending on
>   whether the to-be-patched module is already loaded or is being
>   loaded/unloaded.

You're suggesting that post-(un)patch-hooks:

  1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes

and for subsequently loaded klp_objects (ie modules):

  2 - On load - notify it with current KLP_(UN)PATCHED state,
      Steady state - same as (1) above.

Thanks,

-- Joe

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Thu, Jul 27, 2017 at 04:43:58PM -0400, Joe Lawrence wrote:
> On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
> > - The pre-patch and pre-unpatch hooks can be run before the
> >   patching/unpatching process begins.
> 
> Hi Josh,
> 
> By "(un)patching process" are you referring to the klp_patch at large or
> each klp_object?  ie, would all klp_objects execute their hooks before
> anything is (un)patched?  Just trying to clarify.

I think they can be interleaved: call pre-patch hook for object A, patch
object A, call pre-patch hook for object B, patch object B, etc.  We'll
just have to document that each hook is specific to its own object, and
no assumptions can be made about the state of other objects.

So maybe the pre-patch hooks could be called from klp_patch_object() and
pre-unpatch hooks could be called from klp_unpatch_object().  That way
they're called from both patch enable/disable and module coming/going
contexts.

Or maybe it would be cleaner to call the hooks from the *callers* of
klp_patch/unpatch_object().  That would mean a couple of more call
sites, but at least the locations of the pre- hooks would be more
symmetrical with the post- hook locations.  For example,
klp_module_coming() would have both pre- and post- hook calls.

> > - The post-patch and post-unpatch hooks will need to be run from either
> >   klp_complete_transition() or klp_module_coming/going(), depending on
> >   whether the to-be-patched module is already loaded or is being
> >   loaded/unloaded.
> 
> You're suggesting that post-(un)patch-hooks:
> 
>   1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes

Right. (From klp_complete_transition())

> and for subsequently loaded klp_objects (ie modules):
> 
>   2 - On load - notify it with current KLP_(UN)PATCHED state,
>       Steady state - same as (1) above.

Right. (From klp_module_coming/going())

-- 
Josh

Re: [PATCH] livepatch: add (un)patch hooks

[Joe Lawrence]

On 07/27/2017 05:36 PM, Josh Poimboeuf wrote:
> On Thu, Jul 27, 2017 at 04:43:58PM -0400, Joe Lawrence wrote:
>> On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
>>> - The post-patch and post-unpatch hooks will need to be run from either
>>>   klp_complete_transition() or klp_module_coming/going(), depending on
>>>   whether the to-be-patched module is already loaded or is being
>>>   loaded/unloaded.
>>
>> You're suggesting that post-(un)patch-hooks:
>>
>>   1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes
> 
> Right. (From klp_complete_transition())

We should be careful to only call hooks for those klp_objects that were
actually (un)patched.  I don't think there is such state that makes it
all the way out to klp_complete_transition(), but since both the
completion and module_coming/going code both operate under the
klp_mutex, perhaps klp_is_object_loaded() is a sufficient check?

>> and for subsequently loaded klp_objects (ie modules):
>>
>>   2 - On load - notify it with current KLP_(UN)PATCHED state,
>>       Steady state - same as (1) above.
> 
> Right. (From klp_module_coming/going())
> 

At least this should be easier to implement since we know what the story
is for the klp_object in hand.

-- Joe

Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Fri, Jul 28, 2017 at 02:08:33PM -0400, Joe Lawrence wrote:
> On 07/27/2017 05:36 PM, Josh Poimboeuf wrote:
> > On Thu, Jul 27, 2017 at 04:43:58PM -0400, Joe Lawrence wrote:
> >> On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
> >>> - The post-patch and post-unpatch hooks will need to be run from either
> >>>   klp_complete_transition() or klp_module_coming/going(), depending on
> >>>   whether the to-be-patched module is already loaded or is being
> >>>   loaded/unloaded.
> >>
> >> You're suggesting that post-(un)patch-hooks:
> >>
> >>   1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes
> > 
> > Right. (From klp_complete_transition())
> 
> We should be careful to only call hooks for those klp_objects that were
> actually (un)patched.  I don't think there is such state that makes it
> all the way out to klp_complete_transition(), but since both the
> completion and module_coming/going code both operate under the
> klp_mutex, perhaps klp_is_object_loaded() is a sufficient check?

Yeah, the mutex makes it safe to just check klp_is_object_loaded().

-- 
Josh