Skip to contentSkip to conversation

Kernel Live Patching

PATCH In review Josh Poimboeuf 2 participants 6 messages Dec 10, 2014 – Dec 13, 2014

Subsystems (3)
Documentation x86 Live patching
lore
Share & tools
lore

Josh Poimboeuf

author
#0 lore raw

v6 cover Kernel Live Patching

Thanks for all the patches and comments for v5. Seth is out this week, so I'm sending this latest patch set in his absence.

Changelog:

changes in v6:
- add empty patch release function
- Petr's patches 2-5:
- do not quietly ignore wrong usage
- find and verify the old address in klp_*_init()
- clean up ordering of functions
- split init and free code that is done only for loaded modules
- minor cleanups

changes in v5:
- TODO: set IPMODIFY
- Mira and Petr patches
- add livepatch subdir to samples Makefile
- kernel-doc for internal struct fields and other cleanups (colons)
- s/klp_find_symbol/klp_find_object_symbol/
- remove redundant error checks
- protect API functions from init failure
- protect API functions from unregistered patches
- return -EBUSY in klp_unregister_patch()
- remove unneeded comments
- propagate error in klp_init_objects()
- s/free/err/ for error handling labels
- fix broken patch release handling
- add asm/livepatch.h and asm/elf.h includes
- add comment for numpages
- handle NULL obj->name in kobject_create_and_add()
- add notrace to ftrace handler
- mutex protect all of klp_init_patch()
- generate compile error when asm/livepatch.h included but disabled
- add WARN_ON checks
- rename struct klp_reloc fields
- make readonly bool and optimize
- smaller cleanups

changes in v4:
- TODO: set IPMODIFY (Masami patch heading to -next)
- kernel-doc for structures
- dynamically allocate ftrace ops (add ktype and release for func)
- add sample live patch module
- add CC_USING_FENTRY check
- s/LPC_/KLP_/
- objname NULL for vmlinux rather than "vmlinux"
- remove module unload code and licensing (since we are builtin now)
- remove code section headers
- narrow mutex hold to only include list_add() in register path
- split WARN_ON with compound condition
- add inline to static function defined in header file

changes in v3:
- TODO: FTRACE_OPS_FL_DYNAMIC? allocate fops in core?
- TODO: kernel-doc for API structs once agreed upon
- merge API and core data structures (Miroslav)
- replace lp_ and lpc_ prefixes with klp_
- add ARCH_HAVE_LIVE_PATCHING
- guard livepatch.h content with IS_ENABLED(CONFIG_LIVE_PATCHING)
- smaller cleanups

changes in v2:
- TODO: kernel-doc for API structs once agreed upon
- rebase to next-20141113
- add copyright/license block to livepatch.h
- add _LINUX prefix to header defines
- replace semaphore with mutex
- add LPC_ prefix to state enum
- convert BUGs to WARNs and handle properly
- change Kconfig default to n
- remove [old|new] attrs from function sysfs dir (KASLR leak, no use)
- disregard user provided old_addr if kernel uses KASLR
- s/out/err for error path labels
- s/unregister/disable for uniform terminology
- s/lp/lpc for module notifier elements
- replace module ref'ing with unload notifier + mutex protection
- adjust notifier priority to run before ftrace
- make LIVE_PATCHING boolean (about to depend on arch stuff)
- move x86-specific reloc code to arch/x86
- s/dynrela/reloc/
- add live patching sysfs documentation
- add API function kernel-doc

Summary:

This patchset implements an ftrace-based mechanism and kernel interface for doing live patching of kernel and kernel module functions. It represents the greatest common functionality set between kpatch [1] and kGraft [2] and can accept patches built using either method. This solution was discussed in the Live Patching Mini-conference at LPC 2014 [3].

The model consists of a live patching "core" that provides an interface for other "patch" kernel modules to register patches with the core.

Patch modules contain the new function code and create an klp_patch structure containing the required data about what functions to patch, where the new code for each patched function resides, and in which kernel object (vmlinux or module) the function to be patch resides. The patch module then invokes the klp_register_patch() function to register with the core, then klp_enable_patch() to have the core redirect the execution paths using ftrace.

The live patching core creates a sysfs hierarchy for user-level access to live patching information. The hierarchy is structured like this:

/sys/kernel/livepatch
/sys/kernel/livepatch/<patch>
/sys/kernel/livepatch/<patch>/enabled
/sys/kernel/livepatch/<patch>/<object>
/sys/kernel/livepatch/<patch>/<object>/<func>

The old function is located using one of two methods: it is either provided by the patch module (only possible for a function in vmlinux) or kallsyms lookup. Symbol ambiguity results in a failure.

The core takes a reference on the patch module itself to keep it from unloading. This is because, without a mechanism to ensure that no thread is currently executing in the patched function, we can not determine whether it is safe to unload the patch module. For this reason, unloading patch modules is currently not allowed.

Disabling patches can be done using the "enabled" attribute of the patch:

echo 0 > /sys/kernel/livepatch/<patch>/enabled

If a patch module contains a patch for a module that is not currently loaded, there is nothing to patch so the core does nothing for that patch object. However, the core registers a module notifier that looks for COMING events so that if the module is ever loaded, it is immediately patched. If a module with patch code is removed, the notifier looks for GOING events and disables any patched functions for that object before it unloads. The notifier has a higher priority than that of the ftrace notifier so that it runs before the ftrace notifier for GOING events and we can cleanly unregister from ftrace.

kpatch and kGraft each have their own mechanisms for ensuring system consistency during the patching process. This first version does not implement any consistency mechanism that ensures that old and new code do not run together. In practice, ~90% of CVEs are safe to apply in this way, since they simply add a conditional check. However, any function change that can not execute safely with the old version of the function can _not_ be safely applied for now.

[1] https://github.com/dynup/kpatch
[2] https://git.kernel.org/cgit/linux/kernel/git/jirislaby/kgraft.git/
[3] https://etherpad.fr/p/LPC2014_LivePatching

Seth Jennings (3):
kernel: add TAINT_LIVEPATCH
kernel: add support for live patching
samples: add sample live patching module

 Documentation/ABI/testing/sysfs-kernel-livepatch |  44 ++
 Documentation/oops-tracing.txt                   |   2 +
 Documentation/sysctl/kernel.txt                  |   1 +
 MAINTAINERS                                      |  14 +
 arch/x86/Kconfig                                 |   3 +
 arch/x86/include/asm/livepatch.h                 |  36 +
 arch/x86/kernel/Makefile                         |   1 +
 arch/x86/kernel/livepatch.c                      |  89 +++
 include/linux/kernel.h                           |   1 +
 include/linux/livepatch.h                        | 132 ++++
 kernel/Makefile                                  |   1 +
 kernel/livepatch/Kconfig                         |  18 +
 kernel/livepatch/Makefile                        |   3 +
 kernel/livepatch/core.c                          | 935 +++++++++++++++++++++++
 kernel/panic.c                                   |   2 +
 samples/Kconfig                                  |   7 +
 samples/Makefile                                 |   2 +-
 samples/livepatch/Makefile                       |   1 +
 samples/livepatch/livepatch-sample.c             |  87 +++
 19 files changed, 1378 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-livepatch
 create mode 100644 arch/x86/include/asm/livepatch.h
 create mode 100644 arch/x86/kernel/livepatch.c
 create mode 100644 include/linux/livepatch.h
 create mode 100644 kernel/livepatch/Kconfig
 create mode 100644 kernel/livepatch/Makefile
 create mode 100644 kernel/livepatch/core.c
 create mode 100644 samples/livepatch/Makefile
 create mode 100644 samples/livepatch/livepatch-sample.c
Signature · 1 line
2.1.0
author
#1 lore raw

PATCH v6 1/3 kernel: add TAINT_LIVEPATCH

From: Seth Jennings <[email protected]>

This adds a new taint flag to indicate when the kernel or a kernel module has been live patched. This will provide a clean indication in bug reports that live patching was used.

Additionally, if the crash occurs in a live patched function, the live patch module will appear beside the patched function in the backtrace.

Signed-off-by Seth Jennings <[email protected]>
---
 Documentation/oops-tracing.txt  | 2 ++
 Documentation/sysctl/kernel.txt | 1 +
 include/linux/kernel.h          | 1 +
 kernel/panic.c                  | 2 ++
 4 files changed, 6 insertions(+)
Documentation/oops-tracing.txt +2 -0
index beefb9f..f3ac05c 100644
@@ -270,6 +270,8 @@ characters, each representing a particular tainted value.
 
  15: 'L' if a soft lockup has previously occurred on the system.
 
+ 16: 'K' if the kernel has been live patched.
+
 The primary reason for the 'Tainted: ' string is to tell kernel
 debuggers if this is a clean kernel or if anything unusual has
 occurred.  Tainting is permanent: even if an offending module is
Documentation/sysctl/kernel.txt +1 -0
index 75511ef..83ab256 100644
@@ -843,6 +843,7 @@ can be ORed together:
 8192 - An unsigned module has been loaded in a kernel supporting module
        signature.
 16384 - A soft lockup has previously occurred on the system.
+32768 - The kernel has been live patched.
 
 ==============================================================
include/linux/kernel.h +1 -0
index 5449d2f..d03e3de 100644
@@ -471,6 +471,7 @@ extern enum system_states {
 #define TAINT_OOT_MODULE		12
 #define TAINT_UNSIGNED_MODULE		13
 #define TAINT_SOFTLOCKUP		14
+#define TAINT_LIVEPATCH			15
 
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
kernel/panic.c +2 -0
index 4d8d6f9..8136ad7 100644
@@ -226,6 +226,7 @@ static const struct tnt tnts[] = {
 	{ TAINT_OOT_MODULE,		'O', ' ' },
 	{ TAINT_UNSIGNED_MODULE,	'E', ' ' },
 	{ TAINT_SOFTLOCKUP,		'L', ' ' },
+	{ TAINT_LIVEPATCH,		'K', ' ' },
 };
 
 /**

@@ -246,6 +247,7 @@ static const struct tnt tnts[] = {
  *  'O' - Out-of-tree module has been loaded.
  *  'E' - Unsigned module has been loaded.
  *  'L' - A soft lockup has previously occurred.
+ *  'K' - Kernel has been live patched.
  *
  *	The string is overwritten by the next call to print_tainted().
  */
Signature · 1 line
2.1.0
author
#2 lore raw

PATCH v6 2/3 kernel: add support for live patching

In reply to Josh Poimboeuf

From: Seth Jennings <[email protected]>

This commit introduces code for the live patching core. It implements an ftrace-based mechanism and kernel interface for doing live patching of kernel and kernel module functions.

It represents the greatest common functionality set between kpatch and kgraft and can accept patches built using either method.

This first version does not implement any consistency mechanism that ensures that old and new code do not run together. In practice, ~90% of CVEs are safe to apply in this way, since they simply add a conditional check. However, any function change that can not execute safely with the old version of the function can _not_ be safely applied in this version.

Signed-off-by Seth Jennings <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  44 ++
 MAINTAINERS                                      |  13 +
 arch/x86/Kconfig                                 |   3 +
 arch/x86/include/asm/livepatch.h                 |  36 +
 arch/x86/kernel/Makefile                         |   1 +
 arch/x86/kernel/livepatch.c                      |  89 +++
 include/linux/livepatch.h                        | 132 ++++
 kernel/Makefile                                  |   1 +
 kernel/livepatch/Kconfig                         |  18 +
 kernel/livepatch/Makefile                        |   3 +
 kernel/livepatch/core.c                          | 935 +++++++++++++++++++++++
 11 files changed, 1275 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-livepatch
 create mode 100644 arch/x86/include/asm/livepatch.h
 create mode 100644 arch/x86/kernel/livepatch.c
 create mode 100644 include/linux/livepatch.h
 create mode 100644 kernel/livepatch/Kconfig
 create mode 100644 kernel/livepatch/Makefile
 create mode 100644 kernel/livepatch/core.c
Documentation/ABI/testing/sysfs-kernel-livepatch new file +44 -0
new file mode 100644
index 0000000..5bf42a8
@@ -0,0 +1,44 @@
+What:		/sys/kernel/livepatch
+Date:		Nov 2014
+KernelVersion:	3.19.0
+Contact:	[email protected]
+Description:
+		Interface for kernel live patching
+
+		The /sys/kernel/livepatch directory contains subdirectories for
+		each loaded live patch module.
+
+What:		/sys/kernel/livepatch/<patch>
+Date:		Nov 2014
+KernelVersion:	3.19.0
+Contact:	[email protected]
+Description:
+		The patch directory contains subdirectories for each kernel
+		object (vmlinux or a module) in which it patched functions.
+
+What:		/sys/kernel/livepatch/<patch>/enabled
+Date:		Nov 2014
+KernelVersion:	3.19.0
+Contact:	[email protected]
+Description:
+		A writable attribute that indicates whether the patched
+		code is currently applied.  Writing 0 will disable the patch
+		while writing 1 will re-enable the patch.
+
+What:		/sys/kernel/livepatch/<patch>/<object>
+Date:		Nov 2014
+KernelVersion:	3.19.0
+Contact:	[email protected]
+Description:
+		The object directory contains subdirectories for each function
+		that is patched within the object.
+
+What:		/sys/kernel/livepatch/<patch>/<object>/<function>
+Date:		Nov 2014
+KernelVersion:	3.19.0
+Contact:	[email protected]
+Description:
+		The function directory contains attributes regarding the
+		properties and state of the patched function.
+
+		There are currently no such attributes.
MAINTAINERS +13 -0
index 06d5b4b..94f9f69 100644
@@ -5777,6 +5777,19 @@ F:	Documentation/misc-devices/lis3lv02d
 F:	drivers/misc/lis3lv02d/
 F:	drivers/platform/x86/hp_accel.c
 
+LIVE PATCHING
+M:	Josh Poimboeuf <[email protected]>
+M:	Seth Jennings <[email protected]>
+M:	Jiri Kosina <[email protected]>
+M:	Vojtech Pavlik <[email protected]>
+S:	Maintained
+F:	kernel/livepatch/
+F:	include/linux/livepatch.h
+F:	arch/x86/include/asm/livepatch.h
+F:	arch/x86/kernel/livepatch.c
+F:	Documentation/ABI/testing/sysfs-kernel-livepatch
+L:	[email protected]
+
 LLC (802.2)
 M:	Arnaldo Carvalho de Melo <[email protected]>
 S:	Maintained
arch/x86/Kconfig +3 -0
index ce8dcdf..5c57181 100644
@@ -17,6 +17,7 @@ config X86_64
 	depends on 64BIT
 	select X86_DEV_DMA_OPS
 	select ARCH_USE_CMPXCHG_LOCKREF
+	select ARCH_HAVE_LIVE_PATCHING
 
 ### Arch settings
 config X86

@@ -1986,6 +1987,8 @@ config CMDLINE_OVERRIDE
 	  This is used to work around broken boot loaders.  This should
 	  be set to 'N' under normal conditions.
 
+source "kernel/livepatch/Kconfig"
+
 endmenu
 
 config ARCH_ENABLE_MEMORY_HOTPLUG
arch/x86/include/asm/livepatch.h new file +36 -0
new file mode 100644
index 0000000..c2ae592
@@ -0,0 +1,36 @@
+/*
+ * livepatch.h - x86-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * 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/>.
+ */
+
+#ifndef _ASM_X86_LIVEPATCH_H
+#define _ASM_X86_LIVEPATCH_H
+
+#include <linux/module.h>
+
+#ifdef CONFIG_LIVE_PATCHING
+#ifndef CC_USING_FENTRY
+#error Your compiler must support -mfentry for live patching to work
+#endif
+extern int klp_write_module_reloc(struct module *mod, unsigned long type,
+				  unsigned long loc, unsigned long value);
+
+#else
+#error Live patching support is disabled; check CONFIG_LIVE_PATCHING
+#endif
+
+#endif /* _ASM_X86_LIVEPATCH_H */
arch/x86/kernel/Makefile +1 -0
index 5d4502c..316b34e 100644
@@ -63,6 +63,7 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-y				+= apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
+obj-$(CONFIG_LIVE_PATCHING)	+= livepatch.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
 obj-$(CONFIG_X86_TSC)		+= trace_clock.o
arch/x86/kernel/livepatch.c new file +89 -0

89 lines not on this page · raw message

include/linux/livepatch.h new file +132 -0

132 lines not on this page · raw message

kernel/Makefile +1 -0
index a59481a..616994f 100644
@@ -26,6 +26,7 @@ obj-y += power/
 obj-y += printk/
 obj-y += irq/
 obj-y += rcu/
+obj-y += livepatch/
 
 obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
kernel/livepatch/Kconfig new file +18 -0
new file mode 100644
index 0000000..96da00f
@@ -0,0 +1,18 @@
+config ARCH_HAVE_LIVE_PATCHING
+	boolean
+	help
+	  Arch supports kernel live patching
+
+config LIVE_PATCHING
+	boolean "Kernel Live Patching"
+	depends on DYNAMIC_FTRACE_WITH_REGS
+	depends on MODULES
+	depends on SYSFS
+	depends on KALLSYMS_ALL
+	depends on ARCH_HAVE_LIVE_PATCHING
+	help
+	  Say Y here if you want to support kernel live patching.
+	  This option has no runtime impact until a kernel "patch"
+	  module uses the interface provided by this option to register
+	  a patch, causing calls to patched functions to be redirected
+	  to new function code contained in the patch module.
kernel/livepatch/Makefile new file +3 -0
new file mode 100644
index 0000000..7c1f008
@@ -0,0 +1,3 @@
+obj-$(CONFIG_LIVE_PATCHING) += livepatch.o
+
+livepatch-objs := core.o
kernel/livepatch/core.c new file +935 -0

935 lines not on this page · raw message

Signature · 1 line
2.1.0
maintainer
#3 lore raw

Re: [PATCHv6 2/3] kernel: add support for live patching

Hi,

I think we are really close (or I hope so). I found few suspicious things or nitpicks though. They might have applied also to v5, but I didn't manage to look at that. Sorry about that.

kernel/livepatch/core.c line 69

Quoted 2 lines · Josh Poimboeuf
kernel/livepatch/core.c new file +2 -0
+/* klp_mutex must be held by caller */
+static bool klp_patch_is_registered(struct klp_patch *patch)

Maybe klp_is_patch_registered is more appropriate name (consistent with other predicates in the file).

Quoted 9 lines
kernel/livepatch/core.c new file +9 -0
+{
+	struct klp_patch *mypatch;
+
+	list_for_each_entry(mypatch, &klp_patches, list)
+		if (mypatch == patch)
+			return true;
+
+	return false;
+}

[...]

Quoted 44 lines
kernel/livepatch/core.c new file +44 -0
+static int klp_disable_func(struct klp_func *func)
+{
+	int ret;
+
+	if (WARN_ON(func->state != KLP_ENABLED))
+		return -EINVAL;
+
+	if (WARN_ON(!func->old_addr))
+		return -EINVAL;
+
+	ret = unregister_ftrace_function(func->fops);
+	if (ret) {
+		pr_err("failed to unregister ftrace handler for function '%s' (%d)\n",
+		       func->old_name, ret);
+		return ret;
+	}
+
+	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+	if (ret)
+		pr_warn("function unregister succeeded but failed to clear the filter\n");
+
+	func->state = KLP_DISABLED;
+
+	return 0;
+}
+
+static int klp_enable_func(struct klp_func *func)
+{
+	int ret;
+
+	if (WARN_ON(!func->old_addr))
+		return -EINVAL;
+
+	if (WARN_ON(func->state != KLP_DISABLED))
+		return -EINVAL;
+
+	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 0, 0);
+	if (ret) {
+		pr_err("failed to set ftrace filter for function '%s' (%d)\n",
+		       func->old_name, ret);
+		return ret;
+	}
+
+	ret = register_ftrace_function(func->fops);

kernel/livepatch/core.c line 330

Quoted 10 lines
kernel/livepatch/core.c new file +10 -0
+	if (ret) {
+		pr_err("failed to register ftrace handler for function '%s' (%d)\n",
+		       func->old_name, ret);
+		ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+	} else {
+		func->state = KLP_ENABLED;
+	}
+
+	return ret;
+}

Just to be sure about our policy. We want to be stricter during enabling than in disabling process. Is that correct? Otherwise there is inconsistency in pr_* macros and return values. Also fops could be hypothetically registered back when ftrace_set_filter_ip fails in klp_disable_func. I just want to be sure that we didn't overlook anything...

[...]

kernel/livepatch/core.c line 648

Quoted 10 lines
kernel/livepatch/core.c new file +10 -0
+static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+{
+	struct ftrace_ops *ops;
+	int ret;
+
+	func->state = KLP_DISABLED;
+
+	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		ret = -ENOMEM;

There should be return -ENOMEM.

Quoted 15 lines
kernel/livepatch/core.c new file +15 -0
+
+	ops->private = func;
+	ops->func = klp_ftrace_handler;
+	ops->flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC;
+	func->fops = ops;
+
+	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
+				   obj->kobj, func->old_name);
+	if (ret) {
+		kfree(func->fops);
+		return ret;
+	}
+
+	return 0;
+}

[...]

Quoted 26 lines
kernel/livepatch/core.c new file +26 -0
+static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
+{
+	struct klp_func *func;
+	int ret;
+	const char *name;
+
+	if (!obj->funcs)
+		return -EINVAL;
+
+	obj->state = KLP_DISABLED;
+
+	klp_find_object_module(obj);
+
+	name = klp_is_module(obj) ? obj->name : "vmlinux";
+	obj->kobj = kobject_create_and_add(name, &patch->kobj);
+	if (!obj->kobj)
+		return -ENOMEM;
+
+	for (func = obj->funcs; func->old_name; func++) {
+		ret = klp_init_func(obj, func);
+		if (ret)
+			goto free;
+	}
+
+	if (klp_is_object_loaded(obj)) {
+		ret = klp_init_object_loaded(patch, obj);

kernel/livepatch/core.c line 722

Quoted 10 lines
kernel/livepatch/core.c new file +10 -0
+		if (ret)
+			goto free;
+	}
+
+	return 0;
+
+free:
+	klp_free_funcs_limited(obj, func);
+	return ret;
+}

Shouldn't we call kobject_put(obj->kobj) in free branch? If I am not wrong it is not freed anywhere else. We free only already initialized functions and already initialized objects later in klp_init_patch, but not the kobject of the currently failing object.

Quoted 26 lines
kernel/livepatch/core.c new file +26 -0
+static int klp_init_patch(struct klp_patch *patch)
+{
+	struct klp_object *obj;
+	int ret;
+
+	if (!patch->objs)
+		return -EINVAL;
+
+	mutex_lock(&klp_mutex);
+
+	patch->state = KLP_DISABLED;
+
+	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
+				   klp_root_kobj, patch->mod->name);
+	if (ret)
+		goto unlock;
+
+	for (obj = patch->objs; obj->funcs; obj++) {
+		ret = klp_init_object(patch, obj);
+		if (ret)
+			goto free;
+	}
+
+	list_add(&patch->list, &klp_patches);
+
+	mutex_unlock(&klp_mutex);

kernel/livepatch/core.c line 759

Quoted 10 lines
kernel/livepatch/core.c new file +10 -0
+
+	return 0;
+
+free:
+	klp_free_objects_limited(patch, obj);
+	kobject_put(&patch->kobj);
+unlock:
+	mutex_unlock(&klp_mutex);
+	return ret;
+}

And that is everything. I like it, it has improved a lot. I hope that there are no other problems. I am getting blind looking at it all the time :)

Thank you
Mira

author
#4 lore raw

Re: [PATCHv6 2/3] kernel: add support for live patching

Quoted 7 lines · Miroslav Benes

Hi,

I think we are really close (or I hope so). I found few suspicious things or nitpicks though. They might have applied also to v5, but I didn't manage to look at that. Sorry about that.

Quoted 7 lines

kernel/livepatch/core.c line 69

Quoted 2 lines · Josh Poimboeuf
kernel/livepatch/core.c new file +2 -0
+/* klp_mutex must be held by caller */
+static bool klp_patch_is_registered(struct klp_patch *patch)

Maybe klp_is_patch_registered is more appropriate name (consistent with other predicates in the file).

Ok.

Quoted 53 lines
Quoted 53 lines
kernel/livepatch/core.c new file +53 -0
+static int klp_disable_func(struct klp_func *func)
+{
+	int ret;
+
+	if (WARN_ON(func->state != KLP_ENABLED))
+		return -EINVAL;
+
+	if (WARN_ON(!func->old_addr))
+		return -EINVAL;
+
+	ret = unregister_ftrace_function(func->fops);
+	if (ret) {
+		pr_err("failed to unregister ftrace handler for function '%s' (%d)\n",
+		       func->old_name, ret);
+		return ret;
+	}
+
+	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+	if (ret)
+		pr_warn("function unregister succeeded but failed to clear the filter\n");
+
+	func->state = KLP_DISABLED;
+
+	return 0;
+}
+
+static int klp_enable_func(struct klp_func *func)
+{
+	int ret;
+
+	if (WARN_ON(!func->old_addr))
+		return -EINVAL;
+
+	if (WARN_ON(func->state != KLP_DISABLED))
+		return -EINVAL;
+
+	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 0, 0);
+	if (ret) {
+		pr_err("failed to set ftrace filter for function '%s' (%d)\n",
+		       func->old_name, ret);
+		return ret;
+	}
+
+	ret = register_ftrace_function(func->fops);
+	if (ret) {
+		pr_err("failed to register ftrace handler for function '%s' (%d)\n",
+		       func->old_name, ret);
+		ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+	} else {
+		func->state = KLP_ENABLED;
+	}
+
+	return ret;
Quoted 8 lines
Quoted 1 line

+}

Just to be sure about our policy. We want to be stricter during enabling than in disabling process. Is that correct? Otherwise there is inconsistency in pr_* macros and return values. Also fops could be hypothetically registered back when ftrace_set_filter_ip fails in klp_disable_func. I just want to be sure that we didn't overlook anything...

The asymmetry in the enable/disable error handling is intentional. In klp_disable_func(), a ftrace_set_filter_ip() failure isn't a fatal condition because we've already unregistered the fops and thus removed the patch.

Quoted 4 lines
Quoted 4 lines
kernel/livepatch/core.c new file +4 -0
+static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+{
+	struct ftrace_ops *ops;
+	int ret;
Quoted 8 lines

kernel/livepatch/core.c line 648

Quoted 6 lines
kernel/livepatch/core.c new file +6 -0
+
+	func->state = KLP_DISABLED;
+
+	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		ret = -ENOMEM;

There should be return -ENOMEM.

Agreed.

Quoted 33 lines
Quoted 33 lines
kernel/livepatch/core.c new file +33 -0
+static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
+{
+	struct klp_func *func;
+	int ret;
+	const char *name;
+
+	if (!obj->funcs)
+		return -EINVAL;
+
+	obj->state = KLP_DISABLED;
+
+	klp_find_object_module(obj);
+
+	name = klp_is_module(obj) ? obj->name : "vmlinux";
+	obj->kobj = kobject_create_and_add(name, &patch->kobj);
+	if (!obj->kobj)
+		return -ENOMEM;
+
+	for (func = obj->funcs; func->old_name; func++) {
+		ret = klp_init_func(obj, func);
+		if (ret)
+			goto free;
+	}
+
+	if (klp_is_object_loaded(obj)) {
+		ret = klp_init_object_loaded(patch, obj);
+		if (ret)
+			goto free;
+	}
+
+	return 0;
+
+free:
Quoted 8 lines

kernel/livepatch/core.c line 722

Quoted 3 lines
kernel/livepatch/core.c new file +3 -0
+	klp_free_funcs_limited(obj, func);
+	return ret;
+}

Shouldn't we call kobject_put(obj->kobj) in free branch? If I am not wrong it is not freed anywhere else. We free only already initialized functions and already initialized objects later in klp_init_patch, but not the kobject of the currently failing object.

Agreed.

Quoted 3 lines

And that is everything. I like it, it has improved a lot. I hope that there are no other problems. I am getting blind looking at it all the time :)

Thanks! I'll send out the next patch set soon, maybe Monday.

Signature · 1 line
Josh
author
#5 lore raw

PATCH v6 3/3 samples: add sample live patching module

In reply to Josh Poimboeuf

From: Seth Jennings <[email protected]>

Add a sample live patching module.

Signed-off-by Seth Jennings <[email protected]>
---
 MAINTAINERS                          |  1 +
 samples/Kconfig                      |  7 +++
 samples/Makefile                     |  2 +-
 samples/livepatch/Makefile           |  1 +
 samples/livepatch/livepatch-sample.c | 87 ++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 samples/livepatch/Makefile
 create mode 100644 samples/livepatch/livepatch-sample.c
MAINTAINERS +1 -0
index 94f9f69..15c0929d 100644
@@ -5788,6 +5788,7 @@ F:	include/linux/livepatch.h
 F:	arch/x86/include/asm/livepatch.h
 F:	arch/x86/kernel/livepatch.c
 F:	Documentation/ABI/testing/sysfs-kernel-livepatch
+F:	samples/livepatch/
 L:	[email protected]
 
 LLC (802.2)
samples/Kconfig +7 -0
index 6181c2c..0aed20d 100644
@@ -63,4 +63,11 @@ config SAMPLE_RPMSG_CLIENT
 	  to communicate with an AMP-configured remote processor over
 	  the rpmsg bus.
 
+config SAMPLE_LIVE_PATCHING
+	tristate "Build live patching sample -- loadable modules only"
+	depends on LIVE_PATCHING && m
+	help
+	  Builds a sample live patch that replaces the procfs handler
+	  for /proc/cmdline to print "this has been live patched".
+
 endif # SAMPLES
samples/Makefile +1 -1
index 1a60c62..f00257b 100644
@@ -1,4 +1,4 @@
 # Makefile for Linux samples code
 
-obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ trace_events/ \
+obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ trace_events/ livepatch/ \
 			   hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
samples/livepatch/Makefile new file +1 -0
new file mode 100644
index 0000000..7f1cdc1
@@ -0,0 +1 @@
+obj-$(CONFIG_SAMPLE_LIVE_PATCHING) += livepatch-sample.o
samples/livepatch/livepatch-sample.c new file +87 -0

87 lines not on this page · raw message

Signature · 1 line
2.1.0