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

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

[Jason Baron]

Hi,
 
While using 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 introduced 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

Thanks,

-Jason

v3-v4:
-add static patch, objects, funcs to linked lists to simplify iterator
-break-out pure function movement as patch 2/3
 
v2-v3:
-refactor how the dynamic nops are calculated (Petr Mladek)
-move the creation of dynamic nops to enable/disable paths
-add klp_replaced_patches list to indicate patches that can be re-enabled
-dropped 'replaced' field
-renamed dynamic fields in klp_func, object and patch
-moved iterator implementation to kernel/livepatch/core.c
-'inherit' nop immediate flag
-update kobject_put free'ing logic (Petr Mladek)

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: use lists to manage patches, objects and functions
  livepatch: shuffle core.c function order
  livepatch: add atomic replace

 include/linux/livepatch.h     |  23 +-
 kernel/livepatch/core.c       | 663 ++++++++++++++++++++++++++++++------------
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 +-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 +++-
 6 files changed, 569 insertions(+), 199 deletions(-)

-- 
2.6.1

[PATCH v4 1/3] livepatch: use lists to manage patches, objects and functions

[Jason Baron]

Currently klp_patch contains a pointer to a statically allocated array of
struct klp_object and struct klp_objects contains a pointer to a statically
allocated array of klp_func. In order to allow for the dynamic allocation
of objects and functions, link klp_patch, klp_object, and klp_func together
via linked lists. This allows us to more easily allocate new objects and
functions, while having the iterator be a simple linked list walk.

This patch is intended to be a no-op until atomic replace is introduced by
a subsequent patch.

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 | 19 +++++++++++++++++--
 kernel/livepatch/core.c   |  8 ++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991e..482f90b 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:	links 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;
@@ -94,6 +97,8 @@ struct klp_func {
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
+ * @func_list:	head of list for struct klp_func
+ * @obj_entry:	links struct klp_object to struct klp_patch
  * @patched:	the object's funcs have been added to the klp_ops list
  */
 struct klp_object {
@@ -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 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,18 +134,25 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
-#define klp_for_each_object(patch, obj) \
+#define klp_for_each_object_static(patch, obj) \
 	for (obj = patch->objs; obj->funcs || obj->name; obj++)
 
-#define klp_for_each_func(obj, func) \
+#define klp_for_each_object(patch, obj)	\
+	list_for_each_entry(obj, &patch->obj_list, obj_entry)
+
+#define klp_for_each_func_static(obj, func) \
 	for (func = obj->funcs; \
 	     func->old_name || func->new_func || func->old_sympos; \
 	     func++)
 
+#define klp_for_each_func(obj, func)	\
+	list_for_each_entry(func, &obj->func_list, func_entry)
+
 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 b9628e4..b7f77be 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -608,6 +608,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 	INIT_LIST_HEAD(&func->stack_node);
 	func->patched = false;
 	func->transition = false;
+	list_add(&func->func_entry, &obj->func_list);
 
 	/* The format for the sysfs directory is <function,sympos> where sympos
 	 * is the nth occurrence of this symbol in kallsyms for the patched
@@ -689,7 +690,9 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (ret)
 		return ret;
 
-	klp_for_each_func(obj, func) {
+	list_add(&obj->obj_entry, &patch->obj_list);
+	INIT_LIST_HEAD(&obj->func_list);
+	klp_for_each_func_static(obj, func) {
 		ret = klp_init_func(obj, func);
 		if (ret)
 			goto free;
@@ -729,7 +732,8 @@ static int klp_init_patch(struct klp_patch *patch)
 		return ret;
 	}
 
-	klp_for_each_object(patch, obj) {
+	INIT_LIST_HEAD(&patch->obj_list);
+	klp_for_each_object_static(patch, obj) {
 		ret = klp_init_object(patch, obj);
 		if (ret)
 			goto free;
-- 
2.6.1

[PATCH v4 2/3] livepatch: shuffle core.c function order

[Jason Baron]

In preparation for __klp_enable_patch() to call a number of 'static'
functions, in a subsequent patch, move them earlier in core.c. This patch
should be a nop from a functional pov.

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>
---
 kernel/livepatch/core.c | 349 ++++++++++++++++++++++++------------------------
 1 file changed, 173 insertions(+), 176 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b7f77be..f53eed5 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -283,6 +283,179 @@ static int klp_write_object_relocations(struct module *pmod,
 	return ret;
 }
 
+static void klp_kobj_release_object(struct kobject *kobj)
+{
+}
+
+static struct kobj_type klp_ktype_object = {
+	.release = klp_kobj_release_object,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+static void klp_kobj_release_func(struct kobject *kobj)
+{
+}
+
+static struct kobj_type klp_ktype_func = {
+	.release = klp_kobj_release_func,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+/*
+ * Free all functions' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void klp_free_funcs_limited(struct klp_object *obj,
+				   struct klp_func *limit)
+{
+	struct klp_func *func;
+
+	for (func = obj->funcs; func->old_name && func != limit; func++)
+		kobject_put(&func->kobj);
+}
+
+/* Clean up when a patched object is unloaded */
+static void klp_free_object_loaded(struct klp_object *obj)
+{
+	struct klp_func *func;
+
+	obj->mod = NULL;
+
+	klp_for_each_func(obj, func)
+		func->old_addr = 0;
+}
+
+/*
+ * Free all objects' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void klp_free_objects_limited(struct klp_patch *patch,
+				     struct klp_object *limit)
+{
+	struct klp_object *obj;
+
+	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
+		klp_free_funcs_limited(obj, NULL);
+		kobject_put(&obj->kobj);
+	}
+}
+
+static void klp_free_patch(struct klp_patch *patch)
+{
+	klp_free_objects_limited(patch, NULL);
+	if (!list_empty(&patch->list))
+		list_del(&patch->list);
+}
+
+static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+{
+	if (!func->old_name || !func->new_func)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&func->stack_node);
+	func->patched = false;
+	func->transition = false;
+
+	/* The format for the sysfs directory is <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. If the user selects 0 for old_sympos, then 1 will be used
+	 * since a unique symbol will be the first occurrence.
+	 */
+	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
+				    &obj->kobj, "%s,%lu", func->old_name,
+				    func->old_sympos ? func->old_sympos : 1);
+}
+
+/* Arches may override this to finish any remaining arch-specific tasks */
+void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
+					struct klp_object *obj)
+{
+}
+
+/* parts of the initialization that is done only when the object is loaded */
+static int klp_init_object_loaded(struct klp_patch *patch,
+				  struct klp_object *obj)
+{
+	struct klp_func *func;
+	int ret;
+
+	module_disable_ro(patch->mod);
+	ret = klp_write_object_relocations(patch->mod, obj);
+	if (ret) {
+		module_enable_ro(patch->mod, true);
+		return ret;
+	}
+
+	arch_klp_init_object_loaded(patch, obj);
+	module_enable_ro(patch->mod, true);
+
+	klp_for_each_func(obj, func) {
+		ret = klp_find_object_symbol(obj->name, func->old_name,
+					     func->old_sympos,
+					     &func->old_addr);
+		if (ret)
+			return ret;
+
+		ret = kallsyms_lookup_size_offset(func->old_addr,
+						  &func->old_size, NULL);
+		if (!ret) {
+			pr_err("kallsyms size lookup failed for '%s'\n",
+			       func->old_name);
+			return -ENOENT;
+		}
+
+		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
+						  &func->new_size, NULL);
+		if (!ret) {
+			pr_err("kallsyms size lookup failed for '%s' replacement\n",
+			       func->old_name);
+			return -ENOENT;
+		}
+	}
+
+	return 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->patched = false;
+	obj->mod = NULL;
+
+	klp_find_object_module(obj);
+
+	name = klp_is_module(obj) ? obj->name : "vmlinux";
+	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
+				   &patch->kobj, "%s", name);
+	if (ret)
+		return ret;
+
+	klp_for_each_func(obj, 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:
+	klp_free_funcs_limited(obj, func);
+	kobject_put(&obj->kobj);
+	return ret;
+}
+
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	if (klp_transition_patch)
@@ -536,182 +709,6 @@ static struct kobj_type klp_ktype_patch = {
 	.default_attrs = klp_patch_attrs,
 };
 
-static void klp_kobj_release_object(struct kobject *kobj)
-{
-}
-
-static struct kobj_type klp_ktype_object = {
-	.release = klp_kobj_release_object,
-	.sysfs_ops = &kobj_sysfs_ops,
-};
-
-static void klp_kobj_release_func(struct kobject *kobj)
-{
-}
-
-static struct kobj_type klp_ktype_func = {
-	.release = klp_kobj_release_func,
-	.sysfs_ops = &kobj_sysfs_ops,
-};
-
-/*
- * Free all functions' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
- */
-static void klp_free_funcs_limited(struct klp_object *obj,
-				   struct klp_func *limit)
-{
-	struct klp_func *func;
-
-	for (func = obj->funcs; func->old_name && func != limit; func++)
-		kobject_put(&func->kobj);
-}
-
-/* Clean up when a patched object is unloaded */
-static void klp_free_object_loaded(struct klp_object *obj)
-{
-	struct klp_func *func;
-
-	obj->mod = NULL;
-
-	klp_for_each_func(obj, func)
-		func->old_addr = 0;
-}
-
-/*
- * Free all objects' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
- */
-static void klp_free_objects_limited(struct klp_patch *patch,
-				     struct klp_object *limit)
-{
-	struct klp_object *obj;
-
-	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
-		klp_free_funcs_limited(obj, NULL);
-		kobject_put(&obj->kobj);
-	}
-}
-
-static void klp_free_patch(struct klp_patch *patch)
-{
-	klp_free_objects_limited(patch, NULL);
-	if (!list_empty(&patch->list))
-		list_del(&patch->list);
-}
-
-static int klp_init_func(struct klp_object *obj, struct klp_func *func)
-{
-	if (!func->old_name || !func->new_func)
-		return -EINVAL;
-
-	INIT_LIST_HEAD(&func->stack_node);
-	func->patched = false;
-	func->transition = false;
-	list_add(&func->func_entry, &obj->func_list);
-
-	/* The format for the sysfs directory is <function,sympos> where sympos
-	 * is the nth occurrence of this symbol in kallsyms for the patched
-	 * object. If the user selects 0 for old_sympos, then 1 will be used
-	 * since a unique symbol will be the first occurrence.
-	 */
-	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
-				    &obj->kobj, "%s,%lu", func->old_name,
-				    func->old_sympos ? func->old_sympos : 1);
-}
-
-/* Arches may override this to finish any remaining arch-specific tasks */
-void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
-					struct klp_object *obj)
-{
-}
-
-/* parts of the initialization that is done only when the object is loaded */
-static int klp_init_object_loaded(struct klp_patch *patch,
-				  struct klp_object *obj)
-{
-	struct klp_func *func;
-	int ret;
-
-	module_disable_ro(patch->mod);
-	ret = klp_write_object_relocations(patch->mod, obj);
-	if (ret) {
-		module_enable_ro(patch->mod, true);
-		return ret;
-	}
-
-	arch_klp_init_object_loaded(patch, obj);
-	module_enable_ro(patch->mod, true);
-
-	klp_for_each_func(obj, func) {
-		ret = klp_find_object_symbol(obj->name, func->old_name,
-					     func->old_sympos,
-					     &func->old_addr);
-		if (ret)
-			return ret;
-
-		ret = kallsyms_lookup_size_offset(func->old_addr,
-						  &func->old_size, NULL);
-		if (!ret) {
-			pr_err("kallsyms size lookup failed for '%s'\n",
-			       func->old_name);
-			return -ENOENT;
-		}
-
-		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
-						  &func->new_size, NULL);
-		if (!ret) {
-			pr_err("kallsyms size lookup failed for '%s' replacement\n",
-			       func->old_name);
-			return -ENOENT;
-		}
-	}
-
-	return 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->patched = false;
-	obj->mod = NULL;
-
-	klp_find_object_module(obj);
-
-	name = klp_is_module(obj) ? obj->name : "vmlinux";
-	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
-				   &patch->kobj, "%s", name);
-	if (ret)
-		return ret;
-
-	list_add(&obj->obj_entry, &patch->obj_list);
-	INIT_LIST_HEAD(&obj->func_list);
-	klp_for_each_func_static(obj, 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:
-	klp_free_funcs_limited(obj, func);
-	kobject_put(&obj->kobj);
-	return ret;
-}
-
 static int klp_init_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
-- 
2.6.1

[PATCH v4 3/3] livepatch: add atomic replace

[Jason Baron]

Sometimes we would like to revert a particular fix. This is currently
This is not easy because we want to keep all other fixes active and we
could revert only the last applied patch.

One solution would be to apply new patch that implemented all
the reverted functions like in the original code. It would work
as expected but there will be unnecessary redirections. In addition,
it would also require knowing which functions need to be reverted at
build time.

A better solution would be to say that a new patch replaces
an older one. This might be complicated if we want to replace
a particular patch. But it is rather easy when a new cummulative
patch replaces all others.

Add a new "replace" flag to struct klp_patch. When it is enabled, a set of
nop' klp_func will be dynamically created for all functions that are
already being patched but that will not longer be modified by the new
patch. They are temporarily used as a new target during the patch
transition.

Since 'atomic replace' has completely replaced all previous livepatch
modules, it explicitly disables all previous livepatch modules. However,
previous livepatch modules that have been replaced, can be re-enabled
if they have the 'replace' flag set. In this case the set of 'nop'
functions is re-calculated, such that it replaces all others.

For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set
then:

# insmod kpatch-a.ko
# insmod kpatch-b.ko

At this point we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
0

# cat /sys/kernel/livepatch/kpatch-b/enabled
1

To revert to the kpatch-a state we can do:

echo 1 > /sys/kernel/livepatch/kpatch-a/enabled

And now we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
1

# cat /sys/kernel/livepatch/kpatch-b/enabled
0

Note that it may be possible to unload (rmmod) replaced patches in some
cases based on the consistency model, when we know that all the functions
that are contained in the patch may no longer be in used, however its
left as future work, if this functionality is desired.

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     |   4 +
 kernel/livepatch/core.c       | 326 +++++++++++++++++++++++++++++++++++++++---
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 ++-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 ++++++-
 6 files changed, 383 insertions(+), 29 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 482f90b..d471ddb 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
+ * @nop:	used to revert a previously patched 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 nop;
 };
 
 /**
@@ -119,6 +121,7 @@ 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:	revert previously patched functions not included in the patch
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
  * @obj_list:	head of list for struct klp_object
@@ -130,6 +133,7 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index f53eed5..d1c7a06 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -45,7 +45,14 @@
  */
 DEFINE_MUTEX(klp_mutex);
 
-static LIST_HEAD(klp_patches);
+LIST_HEAD(klp_patches);
+
+/*
+ * List of 'replaced' patches that have been replaced by a patch that has the
+ * 'replace' bit set. When they are added to this list, they are disabled but
+ * they are elligible for being re-enabled.
+ */
+LIST_HEAD(klp_replaced_patches);
 
 static struct kobject *klp_root_kobj;
 
@@ -87,17 +94,28 @@ static void klp_find_object_module(struct klp_object *obj)
 	mutex_unlock(&module_mutex);
 }
 
-static bool klp_is_patch_registered(struct klp_patch *patch)
+static bool klp_is_patch_in_list(struct klp_patch *patch,
+				 struct list_head *head)
 {
 	struct klp_patch *mypatch;
 
-	list_for_each_entry(mypatch, &klp_patches, list)
+	list_for_each_entry(mypatch, head, list)
 		if (mypatch == patch)
 			return true;
 
 	return false;
 }
 
+static bool klp_is_patch_registered(struct klp_patch *patch)
+{
+	return klp_is_patch_in_list(patch, &klp_patches);
+}
+
+static bool klp_is_patch_replaced(struct klp_patch *patch)
+{
+	return klp_is_patch_in_list(patch, &klp_replaced_patches);
+}
+
 static bool klp_initialized(void)
 {
 	return !!klp_root_kobj;
@@ -283,8 +301,21 @@ static int klp_write_object_relocations(struct module *pmod,
 	return ret;
 }
 
+atomic_t klp_nop_release_count;
+static DECLARE_WAIT_QUEUE_HEAD(klp_nop_release_wait);
+
 static void klp_kobj_release_object(struct kobject *kobj)
 {
+	struct klp_object *obj;
+
+	obj = container_of(kobj, struct klp_object, kobj);
+	/* Free dynamically allocated object */
+	if (!obj->funcs) {
+		kfree(obj->name);
+		kfree(obj);
+		atomic_dec(&klp_nop_release_count);
+		wake_up(&klp_nop_release_wait);
+	}
 }
 
 static struct kobj_type klp_ktype_object = {
@@ -294,6 +325,16 @@ static struct kobj_type klp_ktype_object = {
 
 static void klp_kobj_release_func(struct kobject *kobj)
 {
+	struct klp_func *func;
+
+	func = container_of(kobj, struct klp_func, kobj);
+	/* Free dynamically allocated functions */
+	if (!func->new_func) {
+		kfree(func->old_name);
+		kfree(func);
+		atomic_dec(&klp_nop_release_count);
+		wake_up(&klp_nop_release_wait);
+	}
 }
 
 static struct kobj_type klp_ktype_func = {
@@ -347,14 +388,16 @@ 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)
+static int klp_init_func(struct klp_object *obj, struct klp_func *func,
+			 bool nop)
 {
-	if (!func->old_name || !func->new_func)
+	if (!func->old_name || (!nop && !func->new_func))
 		return -EINVAL;
 
 	INIT_LIST_HEAD(&func->stack_node);
 	func->patched = false;
 	func->transition = false;
+	list_add(&func->func_entry, &obj->func_list);
 
 	/* The format for the sysfs directory is <function,sympos> where sympos
 	 * is the nth occurrence of this symbol in kallsyms for the patched
@@ -416,13 +459,14 @@ 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 nop)
 {
 	struct klp_func *func;
 	int ret;
 	const char *name;
 
-	if (!obj->funcs)
+	if (!obj->funcs && !nop)
 		return -EINVAL;
 
 	obj->patched = false;
@@ -436,8 +480,14 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (ret)
 		return ret;
 
-	klp_for_each_func(obj, func) {
-		ret = klp_init_func(obj, func);
+	list_add(&obj->obj_entry, &patch->obj_list);
+	INIT_LIST_HEAD(&obj->func_list);
+
+	if (nop)
+		return 0;
+
+	klp_for_each_func_static(obj, func) {
+		ret = klp_init_func(obj, func, false);
 		if (ret)
 			goto free;
 	}
@@ -456,6 +506,226 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	return ret;
 }
 
+static struct klp_func *klp_find_func(struct klp_object *obj,
+				      struct klp_func *old_func)
+{
+	struct klp_func *func;
+
+	klp_for_each_func(obj, func) {
+		if ((strcmp(old_func->old_name, func->old_name) == 0) &&
+		    (old_func->old_sympos == func->old_sympos)) {
+			return func;
+		}
+	}
+
+	return NULL;
+}
+
+static struct klp_object *klp_find_object(struct klp_patch *patch,
+					  struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	bool mod = klp_is_module(old_obj);
+
+	klp_for_each_object(patch, obj) {
+		if (mod) {
+			if (klp_is_module(obj) &&
+			    strcmp(old_obj->name, obj->name) == 0) {
+				return obj;
+			}
+		} else if (!klp_is_module(obj)) {
+			return obj;
+		}
+	}
+
+	return NULL;
+}
+
+static struct klp_func *klp_alloc_nop_func(struct klp_func *old_func,
+					   struct klp_object *obj)
+{
+	struct klp_func *func;
+	int err;
+
+	func = kzalloc(sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&func->func_entry);
+	if (old_func->old_name) {
+		func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
+		if (!func->old_name) {
+			kfree(func);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+	func->old_sympos = old_func->old_sympos;
+	err = klp_init_func(obj, func, true);
+	if (err) {
+		if (!list_empty(&func->func_entry))
+			list_del(&func->func_entry);
+		kfree(func->old_name);
+		kfree(func);
+		return ERR_PTR(err);
+	}
+	func->immediate = old_func->immediate;
+	func->old_addr = old_func->old_addr;
+	func->old_size = old_func->old_size;
+	func->nop = true;
+
+	return func;
+}
+
+static struct klp_object *klp_alloc_nop_object(const char *name,
+					       struct klp_patch *patch)
+{
+	struct klp_object *obj;
+	int err;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	if (name) {
+		obj->name = kstrdup(name, GFP_KERNEL);
+		if (!obj->name) {
+			kfree(obj);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+	err = klp_init_object(patch, obj, true);
+	if (err) {
+		kfree(obj->name);
+		kfree(obj);
+		return ERR_PTR(err);
+	}
+
+	return obj;
+}
+
+static int klp_add_nop_func(struct klp_object *obj,
+			    struct klp_func *old_func)
+{
+	struct klp_func *func;
+
+	func = klp_find_func(obj, old_func);
+	if (func) {
+		/*
+		 * If any of the previous functions that we are
+		 * reverting don't have the immediate set, then
+		 * we want to set it here. The idea is to use
+		 * the most restrictive case, although its unlikely
+		 * for previous patches to set the immediate flag
+		 * differently for the same function. Note
+		 * if patch->immediate is set this field wouldn't
+		 * be consulted.
+		 */
+		if (func->immediate && !old_func->immediate)
+			func->immediate = false;
+
+		return 0;
+	}
+	func = klp_alloc_nop_func(old_func, obj);
+	if (IS_ERR(func))
+		return PTR_ERR(func);
+
+	return 0;
+}
+
+static int klp_add_nop_object(struct klp_patch *patch,
+			      struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	struct klp_func *old_func;
+	int err = 0;
+
+	obj = klp_find_object(patch, old_obj);
+	if (!obj) {
+		obj = klp_alloc_nop_object(old_obj->name, patch);
+
+		if (IS_ERR(obj))
+			return PTR_ERR(obj);
+	}
+	klp_for_each_func(old_obj, old_func) {
+		err = klp_add_nop_func(obj, old_func);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static bool klp_is_obj_dyn(struct klp_object *obj)
+{
+	struct klp_func *func;
+	bool ret = true;
+
+	klp_for_each_func(obj, func) {
+		if (!func->nop) {
+			ret = false;
+			break;
+		}
+	}
+	return ret;
+}
+
+void klp_remove_nops(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) {
+			if (func->nop) {
+				list_del(&func->func_entry);
+				atomic_inc(&klp_nop_release_count);
+				kobject_put(&func->kobj);
+			}
+		}
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list,
+				 obj_entry) {
+		if (klp_is_obj_dyn(obj)) {
+			list_del(&obj->obj_entry);
+			atomic_inc(&klp_nop_release_count);
+			kobject_put(&obj->kobj);
+		}
+	}
+
+	wait_event(klp_nop_release_wait,
+		   atomic_read(&klp_nop_release_count) == 0);
+}
+
+/* Add 'nop' functions which simply return to the caller to run
+ * the original function. The 'nop' functions are added to a
+ * patch to facilitate a 'replace' mode
+ */
+static int klp_add_nops(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+	struct klp_object *old_obj;
+	int err = 0;
+
+	if (!patch->replace)
+		return 0;
+
+	list_for_each_entry(old_patch, &klp_patches, list) {
+		if (old_patch == patch)
+			break;
+
+		klp_for_each_object(old_patch, old_obj) {
+			err = klp_add_nop_object(patch, old_obj);
+			if (err) {
+				klp_remove_nops(patch);
+				return err;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	if (klp_transition_patch)
@@ -520,6 +790,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
 	int ret;
+	bool replaced = false;
 
 	if (klp_transition_patch)
 		return -EBUSY;
@@ -527,10 +798,22 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (WARN_ON(patch->enabled))
 		return -EINVAL;
 
+	if (klp_is_patch_replaced(patch)) {
+		list_move_tail(&patch->list, &klp_patches);
+		replaced = true;
+	}
+
 	/* enforce stacking: only the first disabled patch can be enabled */
 	if (patch->list.prev != &klp_patches &&
-	    !list_prev_entry(patch, list)->enabled)
-		return -EBUSY;
+	    !list_prev_entry(patch, list)->enabled) {
+		ret = -EBUSY;
+		goto cleanup_list;
+	}
+
+	/* setup nops */
+	ret = klp_add_nops(patch);
+	if (ret)
+		goto cleanup_list;
 
 	/*
 	 * A reference is taken on the patch module to prevent it from being
@@ -541,8 +824,10 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	 * determine if a thread is still running in the patched code contained
 	 * in the patch module once the ftrace registration is successful.
 	 */
-	if (!try_module_get(patch->mod))
-		return -ENODEV;
+	if (!try_module_get(patch->mod)) {
+		ret = -ENODEV;
+		goto cleanup_list;
+	}
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
@@ -576,6 +861,12 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	patch->enabled = true;
 
 	return 0;
+
+cleanup_list:
+	if (replaced)
+		list_move(&patch->list, &klp_replaced_patches);
+
+	return ret;
 }
 
 /**
@@ -593,7 +884,7 @@ int klp_enable_patch(struct klp_patch *patch)
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_registered(patch) && !klp_is_patch_replaced(patch)) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -632,7 +923,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_registered(patch) && !klp_is_patch_replaced(patch)) {
 		/*
 		 * Module with the patch could either disappear meanwhile or is
 		 * not properly initialized yet.
@@ -731,7 +1022,7 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	INIT_LIST_HEAD(&patch->obj_list);
 	klp_for_each_object_static(patch, obj) {
-		ret = klp_init_object(patch, obj);
+		ret = klp_init_object(patch, obj, false);
 		if (ret)
 			goto free;
 	}
@@ -777,6 +1068,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto err;
 	}
 
+	klp_remove_nops(patch);
 	klp_free_patch(patch);
 
 	mutex_unlock(&klp_mutex);
@@ -930,7 +1222,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..fe45ee2 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -1,6 +1,12 @@
 #ifndef _LIVEPATCH_CORE_H
 #define _LIVEPATCH_CORE_H
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
+extern struct list_head klp_patches;
+extern struct list_head klp_replaced_patches;
+
+void klp_remove_nops(struct klp_patch *patch);
 
 #endif /* _LIVEPATCH_CORE_H */
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 52c4e90..aae6e4d 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -117,6 +117,9 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	/* if its a 'nop', then run the original function */
+	if (func->nop)
+		goto unlock;
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	preempt_enable_notrace();
@@ -235,15 +238,21 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_object(struct klp_object *obj, bool nop)
 {
 	struct klp_func *func;
 
-	klp_for_each_func(obj, func)
+	klp_for_each_func(obj, func) {
+		if (nop && !func->nop)
+			continue;
+
 		if (func->patched)
 			klp_unpatch_func(func);
+	}
 
-	obj->patched = false;
+	if (!nop)
+		obj->patched = false;
 }
 
 int klp_patch_object(struct klp_object *obj)
@@ -257,7 +266,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 +275,12 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_objects(struct klp_patch *patch, bool nop)
 {
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, nop);
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index 0db2271..52cc086 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 nop);
+void klp_unpatch_objects(struct klp_patch *patch, bool nop);
 
 #endif /* _LIVEPATCH_PATCH_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index b004a1f..af6c951 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -81,13 +81,35 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	struct klp_patch *old_patch, *tmp_patch;
+
+	/*
+	 * for replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook.
+	 */
+	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
+		list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches,
+					 list) {
+			if (old_patch == klp_transition_patch)
+				break;
+
+			klp_unpatch_objects(old_patch, false);
+			old_patch->enabled = false;
+			if (old_patch->replace)
+				list_move(&old_patch->list,
+					  &klp_replaced_patches);
+			else
+				list_del_init(&old_patch->list);
+		}
+		klp_unpatch_objects(klp_transition_patch, 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);
 
 		/*
 		 * Make sure klp_ftrace_handler() can no longer see functions
@@ -130,6 +152,19 @@ static void klp_complete_transition(void)
 	}
 
 done:
+	/*
+	 * Free the added nops to free the memory and make ensure they are
+	 * re-calculated by a subsequent enable. There are 3 cases:
+	 * 1) enable succeeded -> we've called ftrace_shutdown(), which
+	 *			  means ftrace hooks are no longer visible.
+	 * 2) disable after enable -> nothing to free, since freed by previous
+	 *			      enable
+	 * 3) disable after failed enable -> klp_synchronize_transition() has
+	 *				     been called above, so we should be
+	 *				     ok to free as per usual rcu.
+	 */
+	klp_remove_nops(klp_transition_patch);
+
 	klp_target_state = KLP_UNDEFINED;
 	klp_transition_patch = NULL;
 }
@@ -202,10 +237,17 @@ 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 nop, then we're running the original
+			  * function.
 			  */
-			func_addr = (unsigned long)func->new_func;
-			func_size = func->new_size;
+			if (func->nop) {
+				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

Re: [PATCH v4 2/3] livepatch: shuffle core.c function order

[Petr Mladek]

On Thu 2017-10-12 17:12:28, Jason Baron wrote:
> In preparation for __klp_enable_patch() to call a number of 'static'
> functions, in a subsequent patch, move them earlier in core.c. This patch
> should be a nop from a functional pov.
> 
> 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>
> ---
>  kernel/livepatch/core.c | 349 ++++++++++++++++++++++++------------------------
>  1 file changed, 173 insertions(+), 176 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b7f77be..f53eed5 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -283,6 +283,179 @@ static int klp_write_object_relocations(struct module *pmod,
> +static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> +{
> +	if (!func->old_name || !func->new_func)
> +		return -EINVAL;
> +
> +	INIT_LIST_HEAD(&func->stack_node);
> +	func->patched = false;
> +	func->transition = false;

You lost the change from the 1st patch:

	list_add(&func->func_entry, &obj->func_list);

> +
> +	/* The format for the sysfs directory is <function,sympos> where sympos
> +	 * is the nth occurrence of this symbol in kallsyms for the patched
> +	 * object. If the user selects 0 for old_sympos, then 1 will be used
> +	 * since a unique symbol will be the first occurrence.
> +	 */
> +	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> +				    &obj->kobj, "%s,%lu", func->old_name,
> +				    func->old_sympos ? func->old_sympos : 1);
> +}

[...]

> +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->patched = false;
> +	obj->mod = NULL;
> +
> +	klp_find_object_module(obj);
> +
> +	name = klp_is_module(obj) ? obj->name : "vmlinux";
> +	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
> +				   &patch->kobj, "%s", name);
> +	if (ret)
> +		return ret;
> +

Same here:

	list_add(&obj->obj_entry, &patch->obj_list);
	INIT_LIST_HEAD(&obj->func_list);
	klp_for_each_func_static(obj, func) {

> +	klp_for_each_func(obj, 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:
> +	klp_free_funcs_limited(obj, func);
> +	kobject_put(&obj->kobj);
> +	return ret;
> +}

Best Regards,
Petr

Re: [PATCH v4 2/3] livepatch: shuffle core.c function order

[Jason Baron]

On 10/16/2017 10:42 AM, Petr Mladek wrote:
> On Thu 2017-10-12 17:12:28, Jason Baron wrote:
>> In preparation for __klp_enable_patch() to call a number of 'static'
>> functions, in a subsequent patch, move them earlier in core.c. This patch
>> should be a nop from a functional pov.
>>
>> 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>
>> ---
>>  kernel/livepatch/core.c | 349 ++++++++++++++++++++++++------------------------
>>  1 file changed, 173 insertions(+), 176 deletions(-)
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index b7f77be..f53eed5 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -283,6 +283,179 @@ static int klp_write_object_relocations(struct module *pmod,
>> +static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>> +{
>> +	if (!func->old_name || !func->new_func)
>> +		return -EINVAL;
>> +
>> +	INIT_LIST_HEAD(&func->stack_node);
>> +	func->patched = false;
>> +	func->transition = false;
> 
> You lost the change from the 1st patch:
> 
> 	list_add(&func->func_entry, &obj->func_list);
> 
>> +
>> +	/* The format for the sysfs directory is <function,sympos> where sympos
>> +	 * is the nth occurrence of this symbol in kallsyms for the patched
>> +	 * object. If the user selects 0 for old_sympos, then 1 will be used
>> +	 * since a unique symbol will be the first occurrence.
>> +	 */
>> +	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
>> +				    &obj->kobj, "%s,%lu", func->old_name,
>> +				    func->old_sympos ? func->old_sympos : 1);
>> +}
> 
> [...]
> 
>> +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->patched = false;
>> +	obj->mod = NULL;
>> +
>> +	klp_find_object_module(obj);
>> +
>> +	name = klp_is_module(obj) ? obj->name : "vmlinux";
>> +	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
>> +				   &patch->kobj, "%s", name);
>> +	if (ret)
>> +		return ret;
>> +
> 
> Same here:
> 
> 	list_add(&obj->obj_entry, &patch->obj_list);
> 	INIT_LIST_HEAD(&obj->func_list);
> 	klp_for_each_func_static(obj, func) {

Yes, thanks. I noticed that too when insertion didn't match deletion :(
The final patch ends up adding it back, instead of this one, so the
states of things after patch 3 is ok.

Thanks,

-Jason

Re: [PATCH v4 1/3] livepatch: use lists to manage patches, objects and functions

[Miroslav Benes]

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e4..b7f77be 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -608,6 +608,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  	INIT_LIST_HEAD(&func->stack_node);
>  	func->patched = false;
>  	func->transition = false;
> +	list_add(&func->func_entry, &obj->func_list);

I doubt it matters much, but would list_add_tail() be better? The order 
would then be the same in both linked list and static array.
  
>  	/* The format for the sysfs directory is <function,sympos> where sympos
>  	 * is the nth occurrence of this symbol in kallsyms for the patched
> @@ -689,7 +690,9 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	if (ret)
>  		return ret;
>  
> -	klp_for_each_func(obj, func) {
> +	list_add(&obj->obj_entry, &patch->obj_list);

Here as well.

Miroslav

Re: [PATCH v4 3/3] livepatch: add atomic replace

[Petr Mladek]

On Thu 2017-10-12 17:12:29, Jason Baron wrote:
> Sometimes we would like to revert a particular fix. This is currently
> This is not easy because we want to keep all other fixes active and we
> could revert only the last applied patch.
> 
> One solution would be to apply new patch that implemented all
> the reverted functions like in the original code. It would work
> as expected but there will be unnecessary redirections. In addition,
> it would also require knowing which functions need to be reverted at
> build time.
> 
> A better solution would be to say that a new patch replaces
> an older one. This might be complicated if we want to replace
> a particular patch. But it is rather easy when a new cummulative
> patch replaces all others.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index f53eed5..d1c7a06 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -283,8 +301,21 @@ static int klp_write_object_relocations(struct module *pmod,
>  	return ret;
>  }
>  
> +atomic_t klp_nop_release_count;
> +static DECLARE_WAIT_QUEUE_HEAD(klp_nop_release_wait);
> +
>  static void klp_kobj_release_object(struct kobject *kobj)
>  {
> +	struct klp_object *obj;
> +
> +	obj = container_of(kobj, struct klp_object, kobj);
> +	/* Free dynamically allocated object */
> +	if (!obj->funcs) {
> +		kfree(obj->name);
> +		kfree(obj);
> +		atomic_dec(&klp_nop_release_count);
> +		wake_up(&klp_nop_release_wait);

I would slightly optimize this by

		if (atomic_dec_and_test((&klp_nop_release_count))
			wake_up(&klp_nop_release_wait);

> +	}
>  }
>  
>  static struct kobj_type klp_ktype_object = {
> @@ -294,6 +325,16 @@ static struct kobj_type klp_ktype_object = {
>  
>  static void klp_kobj_release_func(struct kobject *kobj)
>  {
> +	struct klp_func *func;
> +
> +	func = container_of(kobj, struct klp_func, kobj);
> +	/* Free dynamically allocated functions */
> +	if (!func->new_func) {
> +		kfree(func->old_name);
> +		kfree(func);
> +		atomic_dec(&klp_nop_release_count);
> +		wake_up(&klp_nop_release_wait);

Same here

		if (atomic_dec_and_test((&klp_nop_release_count))
			wake_up(&klp_nop_release_wait);


> +	}
>  }
>  
>  static struct kobj_type klp_ktype_func = {
> @@ -436,8 +480,14 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	if (ret)
>  		return ret;
>  
> -	klp_for_each_func(obj, func) {
> -		ret = klp_init_func(obj, func);
> +	list_add(&obj->obj_entry, &patch->obj_list);
> +	INIT_LIST_HEAD(&obj->func_list);
> +
> +	if (nop)
> +		return 0;

Ah, this is something that I wanted to avoid. It makes the code
very hard to read and maintain. It forces us to duplicate
some code in klp_alloc_nop_func(). I think that I complained
about this in v2 already.

I understand that you actually kept it because of me.
It is related to the possibility to re-enable released
patches :-(

The klp_init_*() stuff is called from __klp_enable_patch()
for the "nop" functions now. And it has already been called
for the statically defined structures in klp_register_patch().
Therefore we need to avoid calling it twice for the static
structures.

One solution would be to do these operations for the statically
defined structures in __klp_enable_patch() as well. But this
would mean a big redesign of the code.

Another solution would be to give up on the idea that the replaced
patches might be re-enabled without re-loading. I am afraid
that this the only reasonable approach. It will help to avoid
also the extra klp_replaced_patches list. All this will help to
make the code much easier.

I am really sorry that I asked you to do this exercise and
support the patch re-enablement. It looked like a good idea.
I did not expect that it would be that complicated.

I stop reviewing this patch because it will look quite
different again. I will only keep some random comments
around that I added before finding this main design flaw.

Thanks a lot for the hard work. v4 looks much better than
v2 in many ways. I think that we are going on the right way.

> +
> +	klp_for_each_func_static(obj, func) {
> +		ret = klp_init_func(obj, func, false);
>  		if (ret)
>  			goto free;
>  	}
> @@ -456,6 +506,226 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	return ret;
>  }

[...]

> +/* Add 'nop' functions which simply return to the caller to run
> + * the original function. The 'nop' functions are added to a
> + * patch to facilitate a 'replace' mode
> + */
> +static int klp_add_nops(struct klp_patch *patch)
> +{
> +	struct klp_patch *old_patch;
> +	struct klp_object *old_obj;
> +	int err = 0;
> +
> +	if (!patch->replace)
> +		return 0;

It would be more sane if this returns -EINVAL and
we call the following in __klp_enable_patch().

	if (patch->replace) {
		ret = klp_add_nops(patch);
		...
	}

Otherwise, people might wonder why klp_add_nops() is always called.
They would need to look how it is implemented just to realize that
it is a NOP when patch->replace is not set.

> +
> +	list_for_each_entry(old_patch, &klp_patches, list) {
> +		if (old_patch == patch)
> +			break;
> +
> +		klp_for_each_object(old_patch, old_obj) {
> +			err = klp_add_nop_object(patch, old_obj);
> +			if (err) {
> +				klp_remove_nops(patch);
> +				return err;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int __klp_disable_patch(struct klp_patch *patch)
>  {
>  	if (klp_transition_patch)
> @@ -527,10 +798,22 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	if (WARN_ON(patch->enabled))
>  		return -EINVAL;
>  
> +	if (klp_is_patch_replaced(patch)) {
> +		list_move_tail(&patch->list, &klp_patches);
> +		replaced = true;
> +	}
> +
>  	/* enforce stacking: only the first disabled patch can be enabled */
>  	if (patch->list.prev != &klp_patches &&
> -	    !list_prev_entry(patch, list)->enabled)
> -		return -EBUSY;
> +	    !list_prev_entry(patch, list)->enabled) {
> +		ret = -EBUSY;
> +		goto cleanup_list;
> +	}
> +
> +	/* setup nops */

Please, remove the comment. It does not add much information.

> +	ret = klp_add_nops(patch);
> +	if (ret)
> +		goto cleanup_list;

It will be more obvious when we do:

	if (patch->replace) {
		ret = klp_add_nops(patch);
		if (ret)
			goto cleanup_list;
	}

>  
>  	/*
>  	 * A reference is taken on the patch module to prevent it from being

Best Regards,
Petr

Re: [PATCH v4 3/3] livepatch: add atomic replace

[Jason Baron]

On 10/17/2017 10:18 AM, Petr Mladek wrote:
> On Thu 2017-10-12 17:12:29, Jason Baron wrote:
>> Sometimes we would like to revert a particular fix. This is currently
>> This is not easy because we want to keep all other fixes active and we
>> could revert only the last applied patch.
>>
>> One solution would be to apply new patch that implemented all
>> the reverted functions like in the original code. It would work
>> as expected but there will be unnecessary redirections. In addition,
>> it would also require knowing which functions need to be reverted at
>> build time.
>>
>> A better solution would be to say that a new patch replaces
>> an older one. This might be complicated if we want to replace
>> a particular patch. But it is rather easy when a new cummulative
>> patch replaces all others.
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index f53eed5..d1c7a06 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -283,8 +301,21 @@ static int klp_write_object_relocations(struct module *pmod,
>>  	return ret;
>>  }
>>  
>> +atomic_t klp_nop_release_count;
>> +static DECLARE_WAIT_QUEUE_HEAD(klp_nop_release_wait);
>> +
>>  static void klp_kobj_release_object(struct kobject *kobj)
>>  {
>> +	struct klp_object *obj;
>> +
>> +	obj = container_of(kobj, struct klp_object, kobj);
>> +	/* Free dynamically allocated object */
>> +	if (!obj->funcs) {
>> +		kfree(obj->name);
>> +		kfree(obj);
>> +		atomic_dec(&klp_nop_release_count);
>> +		wake_up(&klp_nop_release_wait);
> 
> I would slightly optimize this by
> 
> 		if (atomic_dec_and_test((&klp_nop_release_count))
> 			wake_up(&klp_nop_release_wait);
> 
>> +	}
>>  }
>>  
>>  static struct kobj_type klp_ktype_object = {
>> @@ -294,6 +325,16 @@ static struct kobj_type klp_ktype_object = {
>>  
>>  static void klp_kobj_release_func(struct kobject *kobj)
>>  {
>> +	struct klp_func *func;
>> +
>> +	func = container_of(kobj, struct klp_func, kobj);
>> +	/* Free dynamically allocated functions */
>> +	if (!func->new_func) {
>> +		kfree(func->old_name);
>> +		kfree(func);
>> +		atomic_dec(&klp_nop_release_count);
>> +		wake_up(&klp_nop_release_wait);
> 
> Same here
> 
> 		if (atomic_dec_and_test((&klp_nop_release_count))
> 			wake_up(&klp_nop_release_wait);
> 
> 
>> +	}
>>  }
>>  
>>  static struct kobj_type klp_ktype_func = {
>> @@ -436,8 +480,14 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>>  	if (ret)
>>  		return ret;
>>  
>> -	klp_for_each_func(obj, func) {
>> -		ret = klp_init_func(obj, func);
>> +	list_add(&obj->obj_entry, &patch->obj_list);
>> +	INIT_LIST_HEAD(&obj->func_list);
>> +
>> +	if (nop)
>> +		return 0;
> 
> Ah, this is something that I wanted to avoid. It makes the code
> very hard to read and maintain. It forces us to duplicate
> some code in klp_alloc_nop_func(). I think that I complained
> about this in v2 already.
> 
> I understand that you actually kept it because of me.
> It is related to the possibility to re-enable released
> patches :-(

hmmm, I actually think that it is not necessary - although I didn't try
removing it. I was concerned that we would call klp_init_func() for
functions that were already initialized. However, this only called right
after creating a brand new object, so there are no associated functions,
and thus I think the above early return is not necessary. I can try
removing it.

> 
> The klp_init_*() stuff is called from __klp_enable_patch()
> for the "nop" functions now. And it has already been called
> for the statically defined structures in klp_register_patch().
> Therefore we need to avoid calling it twice for the static
> structures.
> 
> One solution would be to do these operations for the statically
> defined structures in __klp_enable_patch() as well. But this
> would mean a big redesign of the code.
> 
> Another solution would be to give up on the idea that the replaced
> patches might be re-enabled without re-loading. I am afraid
> that this the only reasonable approach. It will help to avoid
> also the extra klp_replaced_patches list. All this will help to
> make the code much easier.
> 

It seems like the consensus is to abandon re-enable and go back to
rmmod/insmod for revert. I do like the rmmod/insmod revert path in that
it keeps module list smaller. However, there are cases where that is not
possible, but it sounds like in the interest of keeping the code simpler
for the common case, revert would not be possible in those cases, and we
would simply have to add a new livepatch module in those cases to fix
things.

Thanks,

-Jason