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

Groups > linux.kernel > #1685800 > unrolled thread

[PATCH] livepatch hooks, revisted

Back to article view | Back to linux.kernel

Contents

  [PATCH] livepatch hooks, revisted Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-12 16:20 +0200
    [PATCH] livepatch: add (un)patch hooks Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-12 16:20 +0200
      Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-14 03:50 +0200
        Re: [PATCH] livepatch: add (un)patch hooks Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-14 15:30 +0200
          Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-14 15:50 +0200
      Re: [PATCH] livepatch: add (un)patch hooks Petr Mladek <pmladek@suse.com> - 2017-07-17 18:00 +0200

#1685800 — [PATCH] livepatch hooks, revisted

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

[toc] | [next] | [standalone]

#1685805 — [PATCH] livepatch: add (un)patch hooks

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

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

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

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

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

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

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

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

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web