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

Groups > linux.kernel > #1723677 > unrolled thread

[PATCH v2 0/3] livepatch: introduce atomic replace

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] livepatch: introduce atomic replace Jason Baron <jbaron@akamai.com> - 2017-08-30 23:40 +0200
    [PATCH v2 2/3] livepatch: add atomic replace Jason Baron <jbaron@akamai.com> - 2017-08-30 23:40 +0200
    [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators Jason Baron <jbaron@akamai.com> - 2017-08-30 23:40 +0200
      Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func  iterators Petr Mladek <pmladek@suse.com> - 2017-09-07 14:40 +0200
        Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func  iterators Joe Perches <joe@perches.com> - 2017-09-07 19:00 +0200
    [PATCH v2 3/3] livepatch: Add a sysctl livepatch_mode for atomic replace Jason Baron <jbaron@akamai.com> - 2017-08-30 23:50 +0200

#1723677 — [PATCH v2 0/3] livepatch: introduce atomic replace

Hi,

In testing livepatch, I found that when doing cumulative patches, if a patched
function is completed reverted by a subsequent patch (back to its original state)
livepatch does not revert the funtion to its original state. Specifically, if
patch A introduces a change to function 1, and patch B reverts the change to
function 1 and introduces changes to say function 2 and 3 as well, the change
that patch A introducd to function 1 is still present. This could be addressed
by first completely removing patch A (disable and then rmmod) and then inserting
patch B (insmod and enable), but this leaves an unpatched window. In discussing
this issue with Josh on the kpatch mailing list, he mentioned that we could get
'atomic replace working properly', and that is the direction of this patchset:
https://www.redhat.com/archives/kpatch/2017-June/msg00005.html

Patches:

1) livepatch: Add klp_object and klp_func dynamic iterators
A prep patch for the 'atomic replace' feature such that dynamic objects
and functions can be allocated.

2) livepatch: add atomic replace
Core feature.

3) livepatch: Add a sysctl livepatch_mode for atomic replace
Introduces a knob for enabling atomic replace. This patch is really only
meant for testing purposes, and should not be considered for application.
The intention is for the livepatch module generator to set the 'replace'
field in the klp_patch structure such that the kernel knows to do the atomic
replace.

Thanks,

-Jason

v1-v2:                                                                                                          
-removed the func_iter and obj_iter (Petr Mladek)
-initialiing kobject structure for no_op functions using:
 klp_init_object() and klp_init_func()
-added a 'replace' field to klp_patch, similar to the immediate field
-a 'replace' patch now disables all previous patches
-tried to shorten klp_init_patch_no_ops()...
-Simplified logic klp_complete_transition (Petr Mladek)

Jason Baron (3):
  livepatch: Add dynamic klp_object and klp_func iterators
  livepatch: add atomic replace
  livepatch: Add a sysctl livepatch_mode for atomic replace

 include/linux/livepatch.h     | 108 ++++++++++++++++++++++++-
 kernel/livepatch/core.c       | 182 +++++++++++++++++++++++++++++++++++++++---
 kernel/livepatch/core.h       |   5 ++
 kernel/livepatch/patch.c      |  19 +++--
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  47 ++++++++++-
 kernel/sysctl.c               |  12 +++
 7 files changed, 352 insertions(+), 25 deletions(-)

-- 
2.6.1

[toc] | [next] | [standalone]

#1723678 — [PATCH v2 2/3] livepatch: add atomic replace

When doing cumulative patches, if patch A introduces a change to function 1,
and patch B reverts the change to function 1 and introduces changes to say
function 2 and 3 as well, the change that patch A introduced to function 1
is still present.

Introduce atomic replace, by first creating a 'no_op' klp_func for all
the functions that are reverted by patch B. The reason that 'no_op'
klp_funcs are created, instead of just unregistering directly from the ftrace
function hook, is to ensure that the consistency model is properly preserved.
By introducing the 'no_op' functions, 'atomic replace' leverages the existing
consistency model code. Then, after transition to the new code, 'atomic
replace' unregisters the ftrace handlers that are associated with the 'no_op'
klp_funcs, such that that we run the original un-patched function with no
additional no_op function overhead.

Since 'atomic replace' has completely replaced all previous livepatch modules,
it explicitly disables all previous livepatch modules, in the example- patch A,
such that the livepatch module associated with patch A can be completely removed
(rmmod). Patch A is now in a permanently disabled state. But if it is removed
from the kernel with rmmod, it can be re-inserted (insmod), and act as an atomic
replace on top of patch A.

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h     |   6 ++
 kernel/livepatch/core.c       | 177 +++++++++++++++++++++++++++++++++++++++---
 kernel/livepatch/core.h       |   5 ++
 kernel/livepatch/patch.c      |  19 +++--
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  47 ++++++++++-
 6 files changed, 234 insertions(+), 24 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 8d3df55..ee6d18b 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -50,6 +50,7 @@
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
  * @transition:	the func is currently being applied or reverted
+ * @no_op:	this is a no_op function used to compelete revert a function
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:
@@ -88,6 +89,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool no_op;
 };
 
 /**
@@ -119,10 +121,12 @@ struct klp_object {
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
  * @immediate:  patch all funcs immediately, bypassing safety mechanisms
+ * @replace:	replace all funcs, reverting functions that are no longer patched
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
  * @obj_list:	head of list for dynamically allocated struct klp_object
  * @enabled:	the patch is enabled (but operation may be incomplete)
+ * @replaced:	the patch has been replaced an can not be re-enabled
  * @finish:	for waiting till it is safe to remove the patch module
  */
 struct klp_patch {
@@ -130,12 +134,14 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool enabled;
+	bool replaced;
 	struct completion finish;
 };
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6004be3..21cecc5 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -45,7 +45,7 @@
  */
 DEFINE_MUTEX(klp_mutex);
 
-static LIST_HEAD(klp_patches);
+LIST_HEAD(klp_patches);
 
 static struct kobject *klp_root_kobj;
 
@@ -351,6 +351,9 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (klp_transition_patch)
 		return -EBUSY;
 
+	if (patch->replaced)
+		return -EINVAL;
+
 	if (WARN_ON(patch->enabled))
 		return -EINVAL;
 
@@ -600,13 +603,38 @@ static void klp_free_patch(struct klp_patch *patch)
 		list_del(&patch->list);
 }
 
-static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+void klp_patch_free_no_ops(struct klp_patch *patch)
+{
+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);
+		}
+		INIT_LIST_HEAD(&obj->func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kobject_put(&obj->kobj);
+		kfree(obj->name);
+		kfree(obj);
+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+
+static int klp_init_func(struct klp_object *obj, struct klp_func *func,
+			 bool no_op)
 {
-	if (!func->old_name || !func->new_func)
+	if (!func->old_name || (!no_op && !func->new_func))
 		return -EINVAL;
 
-	INIT_LIST_HEAD(&func->stack_node);
 	INIT_LIST_HEAD(&func->func_entry);
+	INIT_LIST_HEAD(&func->stack_node);
 	func->patched = false;
 	func->transition = false;
 
@@ -670,19 +698,23 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	return 0;
 }
 
-static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
+static int klp_init_object(struct klp_patch *patch, struct klp_object *obj,
+			   bool no_op)
 {
 	struct klp_func *func;
 	int ret;
 	const char *name;
 
-	if (!obj->funcs)
+	if (!obj->funcs && !no_op)
 		return -EINVAL;
 
 	obj->patched = false;
 	obj->mod = NULL;
+	INIT_LIST_HEAD(&obj->obj_entry);
+	INIT_LIST_HEAD(&obj->func_list);
 
-	klp_find_object_module(obj);
+	if (!no_op)
+		klp_find_object_module(obj);
 
 	name = klp_is_module(obj) ? obj->name : "vmlinux";
 	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
@@ -690,8 +722,12 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (ret)
 		return ret;
 
+	if (no_op)
+		return 0;
+
 	klp_for_each_func(obj, func) {
-		ret = klp_init_func(obj, func);
+		func->no_op = false;
+		ret = klp_init_func(obj, func, false);
 		if (ret)
 			goto free;
 	}
@@ -710,6 +746,115 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	return ret;
 }
 
+static int klp_init_patch_no_ops(struct klp_patch *prev_patch,
+				 struct klp_patch *patch)
+{
+	struct klp_object *obj, *prev_obj;
+	struct klp_func *prev_func, *func;
+	int ret;
+	bool found, mod;
+
+	klp_for_each_object(prev_patch, prev_obj) {
+		klp_for_each_func(prev_obj, prev_func) {
+next_func:
+			klp_for_each_object(patch, obj) {
+				klp_for_each_func(obj, func) {
+					if ((strcmp(prev_func->old_name,
+						    func->old_name) == 0) &&
+						(prev_func->old_sympos ==
+							func->old_sympos)) {
+						goto next_func;
+					}
+				}
+			}
+			mod = klp_is_module(prev_obj);
+			found = false;
+			klp_for_each_object(patch, obj) {
+				if (mod) {
+					if (klp_is_module(obj) &&
+					    strcmp(prev_obj->name,
+						   obj->name) == 0) {
+						found = true;
+						break;
+					}
+				} else if (!klp_is_module(obj)) {
+					found = true;
+					break;
+				}
+			}
+			if (!found) {
+				obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+				if (!obj)
+					return -ENOMEM;
+				if (prev_obj->name) {
+					obj->name = kstrdup(prev_obj->name,
+							    GFP_KERNEL);
+					if (!obj->name) {
+						kfree(obj);
+						return -ENOMEM;
+					}
+				} else {
+					obj->name = NULL;
+				}
+				obj->funcs = NULL;
+				ret = klp_init_object(patch, obj, true);
+				if (ret) {
+					kfree(obj->name);
+					kfree(obj);
+					return ret;
+				}
+				obj->mod = prev_obj->mod;
+				list_add(&obj->obj_entry, &patch->obj_list);
+			}
+			func = kzalloc(sizeof(*func), GFP_KERNEL);
+			if (!func)
+				return -ENOMEM;
+			if (prev_func->old_name) {
+				func->old_name = kstrdup(prev_func->old_name,
+							 GFP_KERNEL);
+				if (!func->old_name) {
+					kfree(func);
+					return -ENOMEM;
+				}
+			} else {
+				func->old_name = NULL;
+			}
+			func->new_func = NULL;
+			func->old_sympos = prev_func->old_sympos;
+			ret = klp_init_func(obj, func, true);
+			func->immediate = prev_func->immediate;
+			func->old_addr = prev_func->old_addr;
+			func->old_size = prev_func->old_size;
+			func->new_size = 0;
+			func->no_op = true;
+			list_add(&func->func_entry, &obj->func_list);
+		}
+	}
+	return 0;
+}
+
+static int klp_init_no_ops(struct klp_patch *patch)
+{
+	struct klp_patch *prev_patch;
+	int ret = 0;
+
+	if (!patch->replace)
+		return 0;
+
+	prev_patch = patch;
+	while (prev_patch->list.prev != &klp_patches) {
+		prev_patch = list_prev_entry(prev_patch, list);
+
+		ret = klp_init_patch_no_ops(prev_patch, patch);
+		if (ret)
+			return ret;
+
+		if (prev_patch->replace)
+			break;
+	}
+	return 0;
+}
+
 static int klp_init_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -721,6 +866,8 @@ static int klp_init_patch(struct klp_patch *patch)
 	mutex_lock(&klp_mutex);
 
 	patch->enabled = false;
+	patch->replaced = false;
+
 	init_completion(&patch->finish);
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
@@ -732,20 +879,25 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	INIT_LIST_HEAD(&patch->obj_list);
 	klp_for_each_object(patch, obj) {
-		INIT_LIST_HEAD(&obj->obj_entry);
-		INIT_LIST_HEAD(&obj->func_list);
-		ret = klp_init_object(patch, obj);
+		ret = klp_init_object(patch, obj, false);
 		if (ret)
 			goto free;
 	}
 
 	list_add_tail(&patch->list, &klp_patches);
 
+	ret = klp_init_no_ops(patch);
+	if (ret) {
+		list_del(&patch->list);
+		goto free;
+	}
+
 	mutex_unlock(&klp_mutex);
 
 	return 0;
 
 free:
+	klp_patch_free_no_ops(patch);
 	klp_free_objects_limited(patch, obj);
 
 	mutex_unlock(&klp_mutex);
@@ -780,6 +932,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto err;
 	}
 
+	klp_patch_free_no_ops(patch);
 	klp_free_patch(patch);
 
 	mutex_unlock(&klp_mutex);
@@ -933,7 +1086,7 @@ void klp_module_going(struct module *mod)
 			if (patch->enabled || patch == klp_transition_patch) {
 				pr_notice("reverting patch '%s' on unloading module '%s'\n",
 					  patch->mod->name, obj->mod->name);
-				klp_unpatch_object(obj);
+				klp_unpatch_object(obj, false);
 			}
 
 			klp_free_object_loaded(obj);
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index c74f24c..0705ac3 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -1,6 +1,11 @@
 #ifndef _LIVEPATCH_CORE_H
 #define _LIVEPATCH_CORE_H
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
+extern struct list_head klp_patches;
+
+void klp_patch_free_no_ops(struct klp_patch *patch);
 
 #endif /* _LIVEPATCH_CORE_H */
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 52c4e90..10b75e3 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -117,6 +117,8 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	if (func->no_op)
+		goto unlock;
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	preempt_enable_notrace();
@@ -235,15 +237,20 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+void klp_unpatch_object(struct klp_object *obj, bool no_op)
 {
 	struct klp_func *func;
 
-	klp_for_each_func(obj, func)
+	klp_for_each_func(obj, func) {
+		if (no_op && !func->no_op)
+			continue;
+
 		if (func->patched)
 			klp_unpatch_func(func);
+	}
 
-	obj->patched = false;
+	if (!no_op)
+		obj->patched = false;
 }
 
 int klp_patch_object(struct klp_object *obj)
@@ -257,7 +264,7 @@ int klp_patch_object(struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, false);
 			return ret;
 		}
 	}
@@ -266,11 +273,11 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+void klp_unpatch_objects(struct klp_patch *patch, bool no_op)
 {
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, no_op);
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index 0db2271..2e13c50 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -27,7 +27,7 @@ struct klp_ops {
 struct klp_ops *klp_find_ops(unsigned long old_addr);
 
 int klp_patch_object(struct klp_object *obj);
-void klp_unpatch_object(struct klp_object *obj);
-void klp_unpatch_objects(struct klp_patch *patch);
+void klp_unpatch_object(struct klp_object *obj, bool no_op);
+void klp_unpatch_objects(struct klp_patch *patch, bool no_op);
 
 #endif /* _LIVEPATCH_PATCH_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index b004a1f..d5e620e 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -21,6 +21,8 @@
 
 #include <linux/cpu.h>
 #include <linux/stacktrace.h>
+#include <linux/ftrace.h>
+#include <linux/delay.h>
 #include "core.h"
 #include "patch.h"
 #include "transition.h"
@@ -70,6 +72,7 @@ static void klp_synchronize_transition(void)
 	schedule_on_each_cpu(klp_sync);
 }
 
+
 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.
@@ -81,14 +84,39 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	bool no_op = false;
+	struct klp_patch *prev_patch;
+
+	/*
+	 * for replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook. After
+	 * klp_synchronize_transition() is called its safe to free the
+	 * the dynamic no-op functions, done by klp_patch_free_no_ops()
+	 */
+	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
+		prev_patch = klp_transition_patch;
+		while (prev_patch->list.prev != &klp_patches) {
+			prev_patch = list_prev_entry(prev_patch, list);
+			if (prev_patch->enabled) {
+				klp_unpatch_objects(prev_patch, false);
+				prev_patch->enabled = false;
+				prev_patch->replaced = true;
+				module_put(prev_patch->mod);
+			}
+		}
+		klp_unpatch_objects(klp_transition_patch, true);
+		no_op = true;
+	}
 
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*
 		 * All tasks have transitioned to KLP_UNPATCHED so we can now
 		 * remove the new functions from the func_stack.
 		 */
-		klp_unpatch_objects(klp_transition_patch);
+		klp_unpatch_objects(klp_transition_patch, false);
+	}
 
+	if (klp_target_state == KLP_UNPATCHED || no_op) {
 		/*
 		 * Make sure klp_ftrace_handler() can no longer see functions
 		 * from this patch on the ops->func_stack.  Otherwise, after
@@ -130,6 +158,9 @@ static void klp_complete_transition(void)
 	}
 
 done:
+	if (no_op)
+		klp_patch_free_no_ops(klp_transition_patch);
+
 	klp_target_state = KLP_UNDEFINED;
 	klp_transition_patch = NULL;
 }
@@ -202,10 +233,18 @@ static int klp_check_stack_func(struct klp_func *func,
 		if (klp_target_state == KLP_UNPATCHED) {
 			 /*
 			  * Check for the to-be-unpatched function
-			  * (the func itself).
+			  * (the func itself). If we're unpatching
+			  * a no-op, then we're running the original
+			  * function. We never 'patch' a no-op function,
+			  * since we just remove the ftrace hook.
 			  */
-			func_addr = (unsigned long)func->new_func;
-			func_size = func->new_size;
+			if (func->no_op) {
+				func_addr = (unsigned long)func->old_addr;
+				func_size = func->old_size;
+			} else {
+				func_addr = (unsigned long)func->new_func;
+				func_size = func->new_size;
+			}
 		} else {
 			/*
 			 * Check for the to-be-patched function
-- 
2.6.1

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

#1723680 — [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators

In preparation to introducing atomic replace, introduce iterators for klp_func
and klp_object, such that objects and functions can be dynmically allocated
(needed for atomic replace). This patch is intended to effectively be a no-op
until atomic replace is introduced.

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h | 94 +++++++++++++++++++++++++++++++++++++++++++++--
 kernel/livepatch/core.c   |  4 ++
 2 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991e..8d3df55 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/ftrace.h>
 #include <linux/completion.h>
+#include <linux/list.h>
 
 #if IS_ENABLED(CONFIG_LIVEPATCH)
 
@@ -44,6 +45,7 @@
  * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
  * @stack_node:	list node for klp_ops func_stack list
+ * @func_entry: used to link struct klp_func to struct klp_object
  * @old_size:	size of the old function
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
@@ -82,6 +84,7 @@ struct klp_func {
 	unsigned long old_addr;
 	struct kobject kobj;
 	struct list_head stack_node;
+	struct list_head func_entry;
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
@@ -92,6 +95,8 @@ struct klp_func {
  * @name:	module name (or NULL for vmlinux)
  * @funcs:	function entries for functions to be patched in the object
  * @kobj:	kobject for sysfs resources
+ * @func_list:	head of list for dynamically allocate struct klp_func
+ * @obj_entry:	used to link struct klp_object to struct klp_patch
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
  * @patched:	the object's funcs have been added to the klp_ops list
@@ -103,6 +108,8 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head func_list;
+	struct list_head obj_entry;
 	struct module *mod;
 	bool patched;
 };
@@ -114,6 +121,7 @@ struct klp_object {
  * @immediate:  patch all funcs immediately, bypassing safety mechanisms
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
+ * @obj_list:	head of list for dynamically allocated struct klp_object
  * @enabled:	the patch is enabled (but operation may be incomplete)
  * @finish:	for waiting till it is safe to remove the patch module
  */
@@ -126,17 +134,95 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
+static inline struct klp_object *obj_iter_next(struct klp_patch *patch,
+					       struct klp_object *obj)
+{
+	struct klp_object *next_obj = NULL;
+
+	if (list_empty(&obj->obj_entry)) {
+		next_obj = obj + 1;
+		if (next_obj->funcs || next_obj->name)
+			goto out;
+		else
+			next_obj = NULL;
+		if (!list_empty(&patch->obj_list))
+			next_obj = container_of(patch->obj_list.next,
+					struct klp_object,
+					obj_entry);
+		goto out;
+	}
+	if (obj->obj_entry.next != &patch->obj_list)
+		next_obj = container_of(obj->obj_entry.next,
+				struct klp_object,
+				obj_entry);
+out:
+	return next_obj;
+}
+
+static inline struct klp_object *obj_iter_init(struct klp_patch *patch)
+{
+	if (patch->objs->funcs || patch->objs->name)
+		return patch->objs;
+	else
+		return NULL;
+}
+
 #define klp_for_each_object(patch, obj) \
-	for (obj = patch->objs; obj->funcs || obj->name; obj++)
+	for (obj = obj_iter_init(patch); obj; obj = obj_iter_next(patch, obj))
+
+static inline struct klp_func *func_iter_next(struct klp_object *obj,
+					      struct klp_func *func)
+{
+	struct klp_func *next_func = NULL;
+
+	if (list_empty(&func->func_entry)) {
+		next_func = func + 1;
+		if (next_func->old_name || next_func->new_func ||
+		    next_func->old_sympos)
+			goto out;
+		else
+			next_func = NULL;
+		if (!list_empty(&obj->func_list))
+			next_func = container_of(obj->func_list.next,
+					struct klp_func,
+					func_entry);
+		goto out;
+	}
+	if (func->func_entry.next != &obj->func_list)
+		next_func = container_of(func->func_entry.next,
+					 struct klp_func,
+					 func_entry);
+out:
+	return next_func;
+}
+
+static inline struct klp_func *func_iter_init(struct klp_object *obj)
+{
+	/* statically allocated */
+	if (list_empty(&obj->obj_entry)) {
+		if (obj->funcs->old_name || obj->funcs->new_func ||
+		    obj->funcs->old_sympos)
+			return obj->funcs;
+		else
+			return NULL;
+	} else {
+		if (!list_empty(obj->func_list.next))
+			return container_of(obj->func_list.next,
+					    struct klp_func,
+					    func_entry);
+		else
+			return NULL;
+	}
+}
 
 #define klp_for_each_func(obj, func) \
-	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
-	     func++)
+	for (func = func_iter_init(obj); func; \
+	     func = func_iter_next(obj, func))
 
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e4..6004be3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -606,6 +606,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 		return -EINVAL;
 
 	INIT_LIST_HEAD(&func->stack_node);
+	INIT_LIST_HEAD(&func->func_entry);
 	func->patched = false;
 	func->transition = false;
 
@@ -729,7 +730,10 @@ static int klp_init_patch(struct klp_patch *patch)
 		return ret;
 	}
 
+	INIT_LIST_HEAD(&patch->obj_list);
 	klp_for_each_object(patch, obj) {
+		INIT_LIST_HEAD(&obj->obj_entry);
+		INIT_LIST_HEAD(&obj->func_list);
 		ret = klp_init_object(patch, obj);
 		if (ret)
 			goto free;
-- 
2.6.1

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

#1728199 — Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators

On Wed 2017-08-30 17:38:43, Jason Baron wrote:
> In preparation to introducing atomic replace, introduce iterators for klp_func


> and klp_object, such that objects and functions can be dynmically allocated
> (needed for atomic replace). This patch is intended to effectively be a no-op

./scripts/checkpatch.pl reports that these lines are too long.
It prefers a maximum 75 chars per line in the commit message ;-)

> until atomic replace is introduced.
> 
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/ftrace.h>
>  #include <linux/completion.h>
> +#include <linux/list.h>
>  
>  #if IS_ENABLED(CONFIG_LIVEPATCH)
>  
> @@ -44,6 +45,7 @@
>   * @old_addr:	the address of the function being patched
>   * @kobj:	kobject for sysfs resources
>   * @stack_node:	list node for klp_ops func_stack list
> + * @func_entry: used to link struct klp_func to struct klp_object

Please, make it clear that only dynamically allocated structures
are linked. Same for the other entries.

It might look superfluous when you read this patch. But it
will help a lot when you read the code one year from now.


>   * @old_size:	size of the old function
>   * @new_size:	size of the new function
>   * @patched:	the func has been added to the klp_ops list

[...]

> @@ -126,17 +134,95 @@ struct klp_patch {
>  	/* internal */
>  	struct list_head list;
>  	struct kobject kobj;
> +	struct list_head obj_list;
>  	bool enabled;
>  	struct completion finish;
>  };
>  
> +static inline struct klp_object *obj_iter_next(struct klp_patch *patch,
> +					       struct klp_object *obj)

The function is far from trivial. I wonder if it is still a good
candidate for inlining.

Also it should get prefixed by klp_ because it is in a header
that can be included anywhere.

Next I still think that it will be easier to understand when
we make it more clear that only the dymanically allocated
objects are in the list. I mean renaming:

  obj_entry -> dyn_obj_entry
  obj_list -> dyn_obj_list

> +{
> +	struct klp_object *next_obj = NULL;
> +

The code quite tricky. IMHO, it would deserve a comment.

	/*
	 * Statically defined objects are in NULL-ended array.
	 * Only dynamic ones are in the obj_list.
	 */
> +	if (list_empty(&obj->obj_entry)) {
> +		next_obj = obj + 1;
> +		if (next_obj->funcs || next_obj->name)
> +			goto out;
> +		else
> +			next_obj = NULL;

Please, add an empty line here to make it better readable.

> +		if (!list_empty(&patch->obj_list))
> +			next_obj = container_of(patch->obj_list.next,
> +					struct klp_object,
> +					obj_entry);
> +		goto out;
> +	}

Also here an empty line.

> +	if (obj->obj_entry.next != &patch->obj_list)
> +		next_obj = container_of(obj->obj_entry.next,
> +				struct klp_object,
> +				obj_entry);
> +out:
> +	return next_obj;
> +}

> +static inline struct klp_object *obj_iter_init(struct klp_patch *patch)
> +{
> +	if (patch->objs->funcs || patch->objs->name)

I would do something like

#define klp_is_null_obj(obj) (!obj->funcs && !obj->name)

Then it can be used here an in klp_obj_iter_next().
This will be even more useful in the func iterator
where the check is even more complicated.


> +		return patch->objs;
> +	else
> +		return NULL;
> +}
> +
>  #define klp_for_each_object(patch, obj) \
> -	for (obj = patch->objs; obj->funcs || obj->name; obj++)
> +	for (obj = obj_iter_init(patch); obj; obj = obj_iter_next(patch, obj))
> +
> +static inline struct klp_func *func_iter_next(struct klp_object *obj,
> +					      struct klp_func *func)
> +{
> +	struct klp_func *next_func = NULL;
> +
> +	if (list_empty(&func->func_entry)) {
> +		next_func = func + 1;
> +		if (next_func->old_name || next_func->new_func ||
> +		    next_func->old_sympos)
> +			goto out;
> +		else
> +			next_func = NULL;
> +		if (!list_empty(&obj->func_list))
> +			next_func = container_of(obj->func_list.next,
> +					struct klp_func,
> +					func_entry);

I have just realized that a practice is to use list_entry() instead
of container_of() for list entries. It probably makes the code better
readable for a trained eye.

> +		goto out;
> +	}
> +	if (func->func_entry.next != &obj->func_list)
> +		next_func = container_of(func->func_entry.next,
> +					 struct klp_func,
> +					 func_entry);
> +out:
> +	return next_func;
> +}

[...]

>  #define klp_for_each_func(obj, func) \
> -	for (func = obj->funcs; \
> -	     func->old_name || func->new_func || func->old_sympos; \
> -	     func++)
> +	for (func = func_iter_init(obj); func; \
> +	     func = func_iter_next(obj, func))

Otherwise, I have basically the same comments about iter_func
like for iter_obj.


>  int klp_register_patch(struct klp_patch *);
>  int klp_unregister_patch(struct klp_patch *);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e4..6004be3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -729,7 +730,10 @@ static int klp_init_patch(struct klp_patch *patch)
>  		return ret;
>  	}
>  
> +	INIT_LIST_HEAD(&patch->obj_list);

Please, do this together with the other trivial initizalizations.
I mean to do it in the same place like in the other init functions:

	patch->enabled = false;
	patch->replaced = false;
+	INIT_LIST_HEAD(&patch->obj_list);


>  	klp_for_each_object(patch, obj) {
> +		INIT_LIST_HEAD(&obj->obj_entry);
> +		INIT_LIST_HEAD(&obj->func_list);

These two should be done in klp_init_object(). You move it there
in the second patch anyway.

>  		ret = klp_init_object(patch, obj);
>  		if (ret)
>  			goto free;

Best Regards,
Petr

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

#1728315 — Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators

On Thu, 2017-09-07 at 14:34 +0200, Petr Mladek wrote:
> On Wed 2017-08-30 17:38:43, Jason Baron wrote:
[]
> > +	if (list_empty(&obj->obj_entry)) {
> > +		next_obj = obj + 1;
> > +		if (next_obj->funcs || next_obj->name)
> > +			goto out;
> > +		else
> > +			next_obj = NULL;
> 
> Please, add an empty line here to make it better readable.

and/or just get rid of the else

> > +static inline struct klp_func *func_iter_next(struct klp_object *obj,
> > +					      struct klp_func *func)
> > +{
> > +	struct klp_func *next_func = NULL;
> > +
> > +	if (list_empty(&func->func_entry)) {
> > +		next_func = func + 1;
> > +		if (next_func->old_name || next_func->new_func ||
> > +		    next_func->old_sympos)
> > +			goto out;
> > +		else
> > +			next_func = NULL;

here too

> > +		if (!list_empty(&obj->func_list))
> > +			next_func = container_of(obj->func_list.next,
> > +					struct klp_func,
> > +					func_entry);
> 
> I have just realized that a practice is to use list_entry() instead
> of container_of() for list entries. It probably makes the code better
> readable for a trained eye.

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

#1723684 — [PATCH v2 3/3] livepatch: Add a sysctl livepatch_mode for atomic replace

Introduce a sysctl knob such that by default livepatch is not in
'atomic replace' mode. A '0' in /proc/sys/kernel/livepatch_mode means the
current default mode, while a '1' means do atomic replace.

This patch is not meant to be applied and is for testing purposes only. The
intent is for the tool that creates the livepatch modules to set the 'replace'
field in struct klp_patch to 1, to indicate that atomic replace mode is
being requested, 0 otherwise.

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h |  8 ++++++++
 kernel/livepatch/core.c   |  7 +++++++
 kernel/sysctl.c           | 12 ++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index ee6d18b..3c4df79 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -35,6 +35,14 @@
 #define KLP_UNPATCHED	 0
 #define KLP_PATCHED	 1
 
+/* livepatch mode */
+
+extern int sysctl_livepatch_mode;
+enum {
+	LIVEPATCH_MODE_DEFAULT,
+	LIVEPATCH_MODE_REPLACE,
+};
+
 /**
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 21cecc5..cdd89a4 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,6 +49,8 @@ LIST_HEAD(klp_patches);
 
 static struct kobject *klp_root_kobj;
 
+int sysctl_livepatch_mode;
+
 static bool klp_is_module(struct klp_object *obj)
 {
 	return obj->name;
@@ -868,6 +870,11 @@ static int klp_init_patch(struct klp_patch *patch)
 	patch->enabled = false;
 	patch->replaced = false;
 
+	if (sysctl_livepatch_mode == LIVEPATCH_MODE_REPLACE)
+		patch->replace = true;
+	else
+		patch->replace = false;
+
 	init_completion(&patch->finish);
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4dfba1a..3a0a1f6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -67,6 +67,7 @@
 #include <linux/kexec.h>
 #include <linux/bpf.h>
 #include <linux/mount.h>
+#include <linux/livepatch.h>
 
 #include <linux/uaccess.h>
 #include <asm/processor.h>
@@ -1203,6 +1204,17 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &one,
 	},
 #endif
+#ifdef CONFIG_LIVEPATCH
+	{
+		.procname	= "livepatch_mode",
+		.data		= &sysctl_livepatch_mode,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
+#endif
 	{ }
 };
 
-- 
2.6.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web