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

[PATCH v5 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

v4-v5
-re-base onto remove-immediate branch (removing immediate dependencies)
-replaced modules can be re-enabled by doing rmmod and then insmod

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     |  25 +-
 kernel/livepatch/core.c       | 626 ++++++++++++++++++++++++++++++------------
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 +-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  49 +++-
 6 files changed, 537 insertions(+), 195 deletions(-)

-- 
2.6.1

[PATCH v5 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 4754f01..e5db2ba 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)
 
@@ -43,6 +44,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
@@ -80,6 +82,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;
@@ -117,6 +120,8 @@ struct klp_callbacks {
  * @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 {
@@ -127,6 +132,8 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head func_list;
+	struct list_head obj_entry;
 	struct module *mod;
 	bool patched;
 };
@@ -137,6 +144,7 @@ struct klp_object {
  * @objs:	object entries for kernel objects to be patched
  * @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
  */
@@ -148,18 +156,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 41be606..49be67c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -677,6 +677,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
@@ -758,7 +759,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;
@@ -798,7 +801,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 v5 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 | 352 ++++++++++++++++++++++++------------------------
 1 file changed, 176 insertions(+), 176 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 49be67c..5f1de35 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -278,6 +278,182 @@ 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;
+	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_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -605,182 +781,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 v5 3/3] livepatch: add atomic replace

[Jason Baron]

Sometimes we would like to revert a particular fix. 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
by removing them (rmmod) and then re-inserting them (insmod).

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e5db2ba..2a4a0db 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -49,6 +49,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:
@@ -86,6 +87,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool nop;
 };
 
 struct klp_object;
@@ -142,6 +144,7 @@ struct klp_object {
  * struct klp_patch - patch structure for live patching
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
+ * @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
@@ -152,6 +155,7 @@ struct klp_patch {
 	/* external */
 	struct module *mod;
 	struct klp_object *objs;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
@@ -169,7 +173,7 @@ struct klp_patch {
 
 #define klp_for_each_func_static(obj, func) \
 	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
+	     func && (func->old_name || func->new_func || func->old_sympos); \
 	     func++)
 
 #define klp_for_each_func(obj, func)	\
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5f1de35..776b825 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 and
+ * can not be re-enabled, but they can be unregistered().
+ */
+LIST_HEAD(klp_replaced_patches);
 
 static struct kobject *klp_root_kobj;
 
@@ -82,17 +89,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;
@@ -278,8 +296,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 = {
@@ -289,6 +320,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 = {
@@ -344,7 +385,7 @@ static void klp_free_patch(struct klp_patch *patch)
 
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
-	if (!func->old_name || !func->new_func)
+	if (!func->old_name || (!func->nop && !func->new_func))
 		return -EINVAL;
 
 	INIT_LIST_HEAD(&func->stack_node);
@@ -400,6 +441,10 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 			return -ENOENT;
 		}
 
+		/* nop funcs don't have a new_func */
+		if (func->nop)
+			continue;
+
 		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
 						  &func->new_size, NULL);
 		if (!ret) {
@@ -412,13 +457,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;
@@ -454,6 +500,216 @@ 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;
+	func->nop = true;
+	err = klp_init_func(obj, func);
+	if (err) {
+		if (!list_empty(&func->func_entry))
+			list_del(&func->func_entry);
+		kfree(func->old_name);
+		kfree(func);
+		return ERR_PTR(err);
+	}
+	func->old_addr = old_func->old_addr;
+	func->old_size = old_func->old_size;
+
+	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) {
+		/* need to free here b/c may not have been added
+		 * to patch list
+		 */
+		if (!list_empty(&obj->obj_entry))
+			list_del(&obj->obj_entry);
+		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)
+		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)
 {
 	struct klp_object *obj;
@@ -539,6 +795,11 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	    !list_prev_entry(patch, list)->enabled)
 		return -EBUSY;
 
+	/* setup nops */
+	ret = klp_add_nops(patch);
+	if (ret)
+		return ret;
+
 	/*
 	 * A reference is taken on the patch module to prevent it from being
 	 * unloaded.
@@ -803,7 +1064,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;
 	}
@@ -839,7 +1100,7 @@ int klp_unregister_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;
 	}
@@ -849,6 +1110,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto err;
 	}
 
+	klp_remove_nops(patch);
 	klp_free_patch(patch);
 
 	mutex_unlock(&klp_mutex);
@@ -928,7 +1190,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 
 				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_post_unpatch_callback(obj);
 			}
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 48a83d4..49ffceb 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -4,7 +4,13 @@
 
 #include <linux/livepatch.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);
 
 static inline bool klp_is_object_loaded(struct klp_object *obj)
 {
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 82d5842..9330c3f 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -118,6 +118,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();
@@ -236,15 +239,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)
@@ -258,7 +267,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;
 		}
 	}
@@ -267,11 +276,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 e72d825..5608757 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -28,7 +28,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 7c6631e..b3991e4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -81,18 +81,39 @@ static void klp_complete_transition(void)
 	struct klp_object *obj;
 	struct klp_func *func;
 	struct task_struct *g, *task;
+	struct klp_patch *old_patch, *tmp_patch;
 	unsigned int cpu;
 
 	pr_debug("'%s': completing %s transition\n",
 		 klp_transition_patch->mod->name,
 		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
 
+	/*
+	 * 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;
+			list_move(&old_patch->list, &klp_replaced_patches);
+			if (!klp_forced)
+				module_put(old_patch->mod);
+		}
+		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
@@ -143,6 +164,19 @@ static void klp_complete_transition(void)
 	if (!klp_forced && klp_target_state == KLP_UNPATCHED)
 		module_put(klp_transition_patch->mod);
 
+	/*
+	 * 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;
 }
@@ -215,10 +249,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 v5 0/3] livepatch: introduce atomic replace

[Petr Mladek]

On Fri 2018-01-12 14:55:13, Jason Baron wrote:
> 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

JFYI, I have spent a lot of time reviewing the patchset this week.
IMHO, it is basically correct. I am just not happy with the design,
namely code symmetry, code duplication, and the way how "dynamic" and "nop"
terms are used.

I have started to play with the code. Otherwise, it was pretty hard
to be sure if another design would work. I think that I have something
useful. But it still need some testing and clean up. I believe that
I would be able to send it next week.

Best Regards,
Petr

PS: Jason, you did a great job. Do not worry. I will keep you as the
author of the patches even when I send an updated version.

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Evgenii Shatokhin]

On 12.01.2018 22:55, Jason Baron wrote:
> 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

Thanks a lot! Atomic replace is really crucial when using cumulative 
patches.

There is one more thing that might need attention here. In my 
experiments with this patch series, I saw that unpatch callbacks are not 
called for the older binary patch (the one being replaced).

That is, I have prepared 2 binary patches, each has all 4 patch/unpatch 
callbacks.

When I load the first patch, its pre-patch and post-patch callbacks are 
called as expected.

Then I replace it with the second patch. Replacement is successful, the 
pre-patch and post-patch callbacks are called for the second patch, 
However, pre-unpatch and post-unpatch callbacks do not run for the first 
one. This makes it more difficult to clean up what its pre/post-patch 
callbacks have done.

It would be nice if pre-/post- unpatch callbacks were called for the 
first patch, perhaps, before/after the patch is actually disabled during 
replacement. I cannot see right now though, which way is the best to 
implement that.
> 
> v4-v5
> -re-base onto remove-immediate branch (removing immediate dependencies)
> -replaced modules can be re-enabled by doing rmmod and then insmod
> 
> 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     |  25 +-
>   kernel/livepatch/core.c       | 626 ++++++++++++++++++++++++++++++------------
>   kernel/livepatch/core.h       |   6 +
>   kernel/livepatch/patch.c      |  22 +-
>   kernel/livepatch/patch.h      |   4 +-
>   kernel/livepatch/transition.c |  49 +++-
>   6 files changed, 537 insertions(+), 195 deletions(-)
> 

Regards,
Evgenii

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Jason Baron]

On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
> On 12.01.2018 22:55, Jason Baron wrote:
>> 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
> 
> Thanks a lot! Atomic replace is really crucial when using cumulative
> patches.
> 
> There is one more thing that might need attention here. In my
> experiments with this patch series, I saw that unpatch callbacks are not
> called for the older binary patch (the one being replaced).

Thanks for testing and pointing this out.

> 
> That is, I have prepared 2 binary patches, each has all 4 patch/unpatch
> callbacks.
> 
> When I load the first patch, its pre-patch and post-patch callbacks are
> called as expected.
> 
> Then I replace it with the second patch. Replacement is successful, the
> pre-patch and post-patch callbacks are called for the second patch,
> However, pre-unpatch and post-unpatch callbacks do not run for the first
> one. This makes it more difficult to clean up what its pre/post-patch
> callbacks have done.
> 
> It would be nice if pre-/post- unpatch callbacks were called for the
> first patch, perhaps, before/after the patch is actually disabled during
> replacement. I cannot see right now though, which way is the best to
> implement that.
So I think the pre_unpatch() can be called for any prior livepatch
modules from __klp_enable_patch(). Perhaps in reverse order of loading
(if there is more than one), and *before* the pre_patch() for the
livepatch module being loaded. Then, if it sucessfully patches in
klp_complete_transition() the post_unpatch() can be called for any prior
livepatch modules as well. I think again it makes sense to call the
post_unpatch() for prior modules *before* the post_patch() for the
current livepatch modules.

Thanks,

-Jason

>>>> v4-v5
>> -re-base onto remove-immediate branch (removing immediate dependencies)
>> -replaced modules can be re-enabled by doing rmmod and then insmod
>>
>> 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     |  25 +-
>>   kernel/livepatch/core.c       | 626
>> ++++++++++++++++++++++++++++++------------
>>   kernel/livepatch/core.h       |   6 +
>>   kernel/livepatch/patch.c      |  22 +-
>>   kernel/livepatch/patch.h      |   4 +-
>>   kernel/livepatch/transition.c |  49 +++-
>>   6 files changed, 537 insertions(+), 195 deletions(-)
>>
> 
> Regards,
> Evgenii

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Petr Mladek]

On Fri 2018-01-19 16:10:42, Jason Baron wrote:
> 
> 
> On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
> > On 12.01.2018 22:55, Jason Baron wrote:
> > There is one more thing that might need attention here. In my
> > experiments with this patch series, I saw that unpatch callbacks are not
> > called for the older binary patch (the one being replaced).
> 
> So I think the pre_unpatch() can be called for any prior livepatch
> modules from __klp_enable_patch(). Perhaps in reverse order of loading
> (if there is more than one), and *before* the pre_patch() for the
> livepatch module being loaded. Then, if it sucessfully patches in
> klp_complete_transition() the post_unpatch() can be called for any prior
> livepatch modules as well. I think again it makes sense to call the
> post_unpatch() for prior modules *before* the post_patch() for the
> current livepatch modules.

I played with this when working on v6. And I am not sure if it is
worth it.

The main reason is that we are talking about cumulative patches.
They are supposed to preserve most of the existing changes and
just remove and/or add few changes. The older patches might or
might not expect to be replaced this way.

If we would decide to run callbacks from the replaced patches
then it would make sense to run the one from the new patch
first. It is because we might need to do some hacks to preserve
the already existing changes.

We might need something like this for __klp_enable_patch():

static int klp_run_pre_patch_callbacks(struct klp_patch *patch)
{
	struct klp_patch *old_patch;
	struct klp_object *old_obj;
	int ret;

	list_for_each_entry_reverse(old_patch, &klp_patches, list) {
		if (!old_patch->enabled && old_patch != patch)
			continue;

		klp_for_each_object(old_patch, old_obj) {
			if (!klp_is_object_loaded())
				continue;

			if (old_patch == patch) {
				/* pre_patch from new patch */
				ret = klp_pre_patch_callback(obj);
				if (ret)
					return ret;
				if (!patch->replace)
					return;
			} else {
				/* preunpatch from replaced patches */
				klp_pre_unpatch_callback(obj);
			}
		}
	}

	return 0;
}

This was quite hairy. Alternative would be:

static void klp_run_pre_unpatch_callbacks_when_replacing(struct klp_patch *patch)
{
	struct klp_patch *old_patch;
	struct klp_object *old_obj;

	if (WARN_ON(!patch->replace))
		return;

	list_for_each_entry_reverse(old_patch, &klp_patches, list) {
		if (!old_patch->enabled || old_patch == patch)
			continue;

		klp_for_each_object(old_patch, old_obj) {
			if (!klp_is_object_loaded())
				continue;

			klp_pre_unpatch_callback(obj);
		}
	}
}

static int klp_run_pre_patch_callbacks(struct klp_patch *patch)
{
	struct klp_object *old_obj;
	int ret;

	klp_for_each_object(patch, old_obj) {
		if (!klp_is_object_loaded())
			continue;

		ret = klp_pre_patch_callback(obj);
		if (ret)
			return ret;
	}

	if (patch->replace)
		klp_run_pre_unpatch_callbacks_when_replacing(patch);

	return 0;
}

2nd variant is easier to read but a lot of code. And this is only
what we would need for __klp_enable_patch(). But we would need
solution also for:

    klp_cancel_transition();
    klp_try_transition();   (2 variants for patching and unpatching)
    klp_module_coming();
    klp_module_going();



So, we are talking about a lot of rather non-trivial code.
IMHO, it might be easier to run just the callbacks from
the new patch. In reality, the author should always know
what it might be replacing and what needs to be done.

By other words, it might be much easier to handle all
situations in a single script in the new patch. Alternative
would be doing crazy hacks to prevent the older scripts from
destroying what we would like to keep. We would need to
keep in mind interactions between the scripts and
the order in which they are called.

Or do you plan to use cumulative patches to simply
get rid of any other "random" livepatches with something
completely different? In this case, it might be much more
safe to disable the older patches a normal way.

I would suggest to just document the current behavior.
We should create Documentation/livepatch/cummulative-patches.txt
anyway.

Best Regards,
Petr

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Evgenii Shatokhin]

On 26.01.2018 13:23, Petr Mladek wrote:
> On Fri 2018-01-19 16:10:42, Jason Baron wrote:
>>
>>
>> On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
>>> On 12.01.2018 22:55, Jason Baron wrote:
>>> There is one more thing that might need attention here. In my
>>> experiments with this patch series, I saw that unpatch callbacks are not
>>> called for the older binary patch (the one being replaced).
>>
>> So I think the pre_unpatch() can be called for any prior livepatch
>> modules from __klp_enable_patch(). Perhaps in reverse order of loading
>> (if there is more than one), and *before* the pre_patch() for the
>> livepatch module being loaded. Then, if it sucessfully patches in
>> klp_complete_transition() the post_unpatch() can be called for any prior
>> livepatch modules as well. I think again it makes sense to call the
>> post_unpatch() for prior modules *before* the post_patch() for the
>> current livepatch modules.
> 
> I played with this when working on v6. And I am not sure if it is
> worth it.
> 
> The main reason is that we are talking about cumulative patches.
> They are supposed to preserve most of the existing changes and
> just remove and/or add few changes. The older patches might or
> might not expect to be replaced this way.
> 
> If we would decide to run callbacks from the replaced patches
> then it would make sense to run the one from the new patch
> first. It is because we might need to do some hacks to preserve
> the already existing changes.
> 
> We might need something like this for __klp_enable_patch():
> 
> static int klp_run_pre_patch_callbacks(struct klp_patch *patch)
> {
> 	struct klp_patch *old_patch;
> 	struct klp_object *old_obj;
> 	int ret;
> 
> 	list_for_each_entry_reverse(old_patch, &klp_patches, list) {
> 		if (!old_patch->enabled && old_patch != patch)
> 			continue;
> 
> 		klp_for_each_object(old_patch, old_obj) {
> 			if (!klp_is_object_loaded())
> 				continue;
> 
> 			if (old_patch == patch) {
> 				/* pre_patch from new patch */
> 				ret = klp_pre_patch_callback(obj);
> 				if (ret)
> 					return ret;
> 				if (!patch->replace)
> 					return;
> 			} else {
> 				/* preunpatch from replaced patches */
> 				klp_pre_unpatch_callback(obj);
> 			}
> 		}
> 	}
> 
> 	return 0;
> }
> 
> This was quite hairy. Alternative would be:
> 
> static void klp_run_pre_unpatch_callbacks_when_replacing(struct klp_patch *patch)
> {
> 	struct klp_patch *old_patch;
> 	struct klp_object *old_obj;
> 
> 	if (WARN_ON(!patch->replace))
> 		return;
> 
> 	list_for_each_entry_reverse(old_patch, &klp_patches, list) {
> 		if (!old_patch->enabled || old_patch == patch)
> 			continue;
> 
> 		klp_for_each_object(old_patch, old_obj) {
> 			if (!klp_is_object_loaded())
> 				continue;
> 
> 			klp_pre_unpatch_callback(obj);
> 		}
> 	}
> }
> 
> static int klp_run_pre_patch_callbacks(struct klp_patch *patch)
> {
> 	struct klp_object *old_obj;
> 	int ret;
> 
> 	klp_for_each_object(patch, old_obj) {
> 		if (!klp_is_object_loaded())
> 			continue;
> 
> 		ret = klp_pre_patch_callback(obj);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	if (patch->replace)
> 		klp_run_pre_unpatch_callbacks_when_replacing(patch);
> 
> 	return 0;
> }
> 
> 2nd variant is easier to read but a lot of code. And this is only
> what we would need for __klp_enable_patch(). But we would need
> solution also for:
> 
>      klp_cancel_transition();
>      klp_try_transition();   (2 variants for patching and unpatching)
>      klp_module_coming();
>      klp_module_going();
> 
> 
> 
> So, we are talking about a lot of rather non-trivial code.
> IMHO, it might be easier to run just the callbacks from
> the new patch. In reality, the author should always know
> what it might be replacing and what needs to be done.
> 
> By other words, it might be much easier to handle all
> situations in a single script in the new patch. Alternative
> would be doing crazy hacks to prevent the older scripts from
> destroying what we would like to keep. We would need to
> keep in mind interactions between the scripts and
> the order in which they are called.
> 
> Or do you plan to use cumulative patches to simply
> get rid of any other "random" livepatches with something
> completely different? In this case, it might be much more
> safe to disable the older patches a normal way.

In my experience, it was quite convenient sometimes to just "replace all 
binary patches the user currently has loaded with this single one". No 
matter what these original binary patches did and where they came from.

Another problematic situation is when you need to actually downgrade a 
cumulative patch. Should be rare, but...

Well, I think we will disable the old patches explicitly in these cases, 
before loading of the new one. May be fragile but easier to maintain.

> 
> I would suggest to just document the current behavior.
> We should create Documentation/livepatch/cummulative-patches.txt
> anyway.

Yes, this would be helpful, because the behaviour is not very obvious.

Regards,
Evgenii

> 
> Best Regards,
> Petr
> .
> 

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Petr Mladek]

On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote:
> On 26.01.2018 13:23, Petr Mladek wrote:
> > On Fri 2018-01-19 16:10:42, Jason Baron wrote:
> > > 
> > > 
> > > On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
> > > > On 12.01.2018 22:55, Jason Baron wrote:
> > > > There is one more thing that might need attention here. In my
> > > > experiments with this patch series, I saw that unpatch callbacks are not
> > > > called for the older binary patch (the one being replaced).
> > > 
> > > So I think the pre_unpatch() can be called for any prior livepatch
> > > modules from __klp_enable_patch(). Perhaps in reverse order of loading
> > > (if there is more than one), and *before* the pre_patch() for the
> > > livepatch module being loaded. Then, if it sucessfully patches in
> > > klp_complete_transition() the post_unpatch() can be called for any prior
> > > livepatch modules as well. I think again it makes sense to call the
> > > post_unpatch() for prior modules *before* the post_patch() for the
> > > current livepatch modules.
> > 
> > So, we are talking about a lot of rather non-trivial code.
> > IMHO, it might be easier to run just the callbacks from
> > the new patch. In reality, the author should always know
> > what it might be replacing and what needs to be done.
> > 
> > By other words, it might be much easier to handle all
> > situations in a single script in the new patch. Alternative
> > would be doing crazy hacks to prevent the older scripts from
> > destroying what we would like to keep. We would need to
> > keep in mind interactions between the scripts and
> > the order in which they are called.
> > 
> > Or do you plan to use cumulative patches to simply
> > get rid of any other "random" livepatches with something
> > completely different? In this case, it might be much more
> > safe to disable the older patches a normal way.
> 
> In my experience, it was quite convenient sometimes to just "replace all
> binary patches the user currently has loaded with this single one". No
> matter what these original binary patches did and where they came from.

To be honest, I would feel better if the livepatch framework is
more safe. It should prevent breaking the system by a patch
that atomically replaces other random patches that rely on callbacks.

Well, combining random livepatches from random vendors is a call
for troubles. It might easily fail when two patches add
different changes to the same function.

I wonder if we should introduce some tags, keys, vendors. I mean
to define an identification or dependencies that would say that some
patches are compatible or incompatible.

We could allow to livepatch one function by two livepatches
only if they are from the same vendor. But then the order still
might be important. Also I am not sure if this condition is safe
enough.

One the other hand, we could handle callbacks like the shadow
variables. Every shadow variable has an ID. We have an API to
add/read/update/remove them. We might allow to have more
callbacks with different IDs. Then we could run callbacks
only for IDs that are newly added or removed. Sigh, this would
be very complex implementation as well. But it might make
these features easier to use and maintain.


Alternatively, I thought about having two modes. One is
stack of "random" patches. Second is a cumulative mode.
IMHO, the combination of the two modes makes things very
complex. It might be much easier if we allow to load
patch with the replace flag enabled only on top of
another patch with this flag enabled.


> Another problematic situation is when you need to actually downgrade a
> cumulative patch. Should be rare, but...

Good point. Well, especially the callbacks should be rare.

I would like to hear from people that have some experience
or plans with using callbacks and cumulative patches.

I wonder how they plan to technically reuse a feature
in the cummulative patches. IMHO, it should be very
rare to add/remove callbacks. Then it might be safe
to downgrade a cummulative patch if the callbacks
are exactly the same.

> Well, I think we will disable the old patches explicitly in these cases,
> before loading of the new one. May be fragile but easier to maintain.

I am afraid that we will not be able to support all use cases
and keep the code sane.

Best Regards,
Petr

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Evgenii Shatokhin]

On 30.01.2018 17:03, Petr Mladek wrote:
> On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote:
>> On 26.01.2018 13:23, Petr Mladek wrote:
>>> On Fri 2018-01-19 16:10:42, Jason Baron wrote:
>>>>
>>>>
>>>> On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
>>>>> On 12.01.2018 22:55, Jason Baron wrote:
>>>>> There is one more thing that might need attention here. In my
>>>>> experiments with this patch series, I saw that unpatch callbacks are not
>>>>> called for the older binary patch (the one being replaced).
>>>>
>>>> So I think the pre_unpatch() can be called for any prior livepatch
>>>> modules from __klp_enable_patch(). Perhaps in reverse order of loading
>>>> (if there is more than one), and *before* the pre_patch() for the
>>>> livepatch module being loaded. Then, if it sucessfully patches in
>>>> klp_complete_transition() the post_unpatch() can be called for any prior
>>>> livepatch modules as well. I think again it makes sense to call the
>>>> post_unpatch() for prior modules *before* the post_patch() for the
>>>> current livepatch modules.
>>>
>>> So, we are talking about a lot of rather non-trivial code.
>>> IMHO, it might be easier to run just the callbacks from
>>> the new patch. In reality, the author should always know
>>> what it might be replacing and what needs to be done.
>>>
>>> By other words, it might be much easier to handle all
>>> situations in a single script in the new patch. Alternative
>>> would be doing crazy hacks to prevent the older scripts from
>>> destroying what we would like to keep. We would need to
>>> keep in mind interactions between the scripts and
>>> the order in which they are called.
>>>
>>> Or do you plan to use cumulative patches to simply
>>> get rid of any other "random" livepatches with something
>>> completely different? In this case, it might be much more
>>> safe to disable the older patches a normal way.
>>
>> In my experience, it was quite convenient sometimes to just "replace all
>> binary patches the user currently has loaded with this single one". No
>> matter what these original binary patches did and where they came from.
> 
> To be honest, I would feel better if the livepatch framework is
> more safe. It should prevent breaking the system by a patch
> that atomically replaces other random patches that rely on callbacks.
> 
> Well, combining random livepatches from random vendors is a call
> for troubles. It might easily fail when two patches add
> different changes to the same function.
> 
> I wonder if we should introduce some tags, keys, vendors. I mean
> to define an identification or dependencies that would say that some
> patches are compatible or incompatible.
> 
> We could allow to livepatch one function by two livepatches
> only if they are from the same vendor. But then the order still
> might be important. Also I am not sure if this condition is safe
> enough.
> 
> One the other hand, we could handle callbacks like the shadow
> variables. Every shadow variable has an ID. We have an API to
> add/read/update/remove them. We might allow to have more
> callbacks with different IDs. Then we could run callbacks
> only for IDs that are newly added or removed. Sigh, this would
> be very complex implementation as well. But it might make
> these features easier to use and maintain.
> 
> 
> Alternatively, I thought about having two modes. One is
> stack of "random" patches. Second is a cumulative mode.
> IMHO, the combination of the two modes makes things very
> complex. It might be much easier if we allow to load
> patch with the replace flag enabled only on top of
> another patch with this flag enabled.
> 
> 
>> Another problematic situation is when you need to actually downgrade a
>> cumulative patch. Should be rare, but...
> 
> Good point. Well, especially the callbacks should be rare.

Yes, we will probably use them for the most important fixes only and 
only if there are no other suitable options. The patches with could be 
more difficult to maintain anyway.

> 
> I would like to hear from people that have some experience
> or plans with using callbacks and cumulative patches.
> 
> I wonder how they plan to technically reuse a feature
> in the cummulative patches. IMHO, it should be very
> rare to add/remove callbacks. Then it might be safe
> to downgrade a cummulative patch if the callbacks
> are exactly the same.
> 
>> Well, I think we will disable the old patches explicitly in these cases,
>> before loading of the new one. May be fragile but easier to maintain.
> 
> I am afraid that we will not be able to support all use cases
> and keep the code sane.

Thats is OK.

I agree with you that the current behaviour of the 'replace' operation 
w.r.t. patch callbacks should stay as is. Making kernel code more 
complex than it should be is definitely a bad thing.

I cannot say much about generic policies for cumulative/non-cumulative 
patches here. In our particular case (Virtuozzo ReadyKernel), the 
cumulative patches are easily recognizable by their names, they have 
'cumulative' substring there. Patch version is also included in the name 
and is easily obtainable.

So I think, I'll update our userspace tools (based on kpatch) so that 
their 'replace' command worked as follows:

* Explicitly disable all loaded non-cumulative patches first.
* If the new patch is non-cumulative, just disable all loaded patches 
explicitly and then load the new one.
* If the new patch is cumulative, explicitly disable the loaded 
cumulative patches that have smaller version numbers than the new one 
(if any). Then run replace as usual.

This way, the in-kernel 'replace' functionality will only be used when 
upgrading cumulative patches - and we can design their callbacks (if the 
callbacks are needed at some point) according to the current behaviour.

Patch downgrade operations should be very rare and could only happen if 
something goes wrong at the users' side. I think, it is acceptable to 
disable the loaded patch first in that case. Its callbacks, if present, 
will do proper cleanup. Then the older, "good" patch can be loaded 
normally. If such things happen, we'll have to investigate the affected 
nodes anyway.

Same for the custom (non-cumulative) patches. We sometimes use them to 
"patch things up" quickly before the cumulative patch with the 
appropriate fixes is officially released. It should be reasonably safe 
to disable these patches explicitly when loading a new cumulative patch.

So, I think, the current situation with 'replace' & callbacks is 
acceptable for us.

Just my 2 cents.

> 
> Best Regards,
> Petr
> .
> 

Regards,
Evgenii

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Jason Baron]

On 01/30/2018 10:06 AM, Evgenii Shatokhin wrote:
> On 30.01.2018 17:03, Petr Mladek wrote:
>> On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote:
>>> On 26.01.2018 13:23, Petr Mladek wrote:
>>>> On Fri 2018-01-19 16:10:42, Jason Baron wrote:
>>>>>
>>>>>
>>>>> On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
>>>>>> On 12.01.2018 22:55, Jason Baron wrote:
>>>>>> There is one more thing that might need attention here. In my
>>>>>> experiments with this patch series, I saw that unpatch callbacks
>>>>>> are not
>>>>>> called for the older binary patch (the one being replaced).
>>>>>
>>>>> So I think the pre_unpatch() can be called for any prior livepatch
>>>>> modules from __klp_enable_patch(). Perhaps in reverse order of loading
>>>>> (if there is more than one), and *before* the pre_patch() for the
>>>>> livepatch module being loaded. Then, if it sucessfully patches in
>>>>> klp_complete_transition() the post_unpatch() can be called for any
>>>>> prior
>>>>> livepatch modules as well. I think again it makes sense to call the
>>>>> post_unpatch() for prior modules *before* the post_patch() for the
>>>>> current livepatch modules.
>>>>
>>>> So, we are talking about a lot of rather non-trivial code.
>>>> IMHO, it might be easier to run just the callbacks from
>>>> the new patch. In reality, the author should always know
>>>> what it might be replacing and what needs to be done.
>>>>
>>>> By other words, it might be much easier to handle all
>>>> situations in a single script in the new patch. Alternative
>>>> would be doing crazy hacks to prevent the older scripts from
>>>> destroying what we would like to keep. We would need to
>>>> keep in mind interactions between the scripts and
>>>> the order in which they are called.
>>>>
>>>> Or do you plan to use cumulative patches to simply
>>>> get rid of any other "random" livepatches with something
>>>> completely different? In this case, it might be much more
>>>> safe to disable the older patches a normal way.
>>>
>>> In my experience, it was quite convenient sometimes to just "replace all
>>> binary patches the user currently has loaded with this single one". No
>>> matter what these original binary patches did and where they came from.
>>
>> To be honest, I would feel better if the livepatch framework is
>> more safe. It should prevent breaking the system by a patch
>> that atomically replaces other random patches that rely on callbacks.
>>
>> Well, combining random livepatches from random vendors is a call
>> for troubles. It might easily fail when two patches add
>> different changes to the same function.
>>
>> I wonder if we should introduce some tags, keys, vendors. I mean
>> to define an identification or dependencies that would say that some
>> patches are compatible or incompatible.
>>
>> We could allow to livepatch one function by two livepatches
>> only if they are from the same vendor. But then the order still
>> might be important. Also I am not sure if this condition is safe
>> enough.
>>
>> One the other hand, we could handle callbacks like the shadow
>> variables. Every shadow variable has an ID. We have an API to
>> add/read/update/remove them. We might allow to have more
>> callbacks with different IDs. Then we could run callbacks
>> only for IDs that are newly added or removed. Sigh, this would
>> be very complex implementation as well. But it might make
>> these features easier to use and maintain.
>>
>>
>> Alternatively, I thought about having two modes. One is
>> stack of "random" patches. Second is a cumulative mode.
>> IMHO, the combination of the two modes makes things very
>> complex. It might be much easier if we allow to load
>> patch with the replace flag enabled only on top of
>> another patch with this flag enabled.
>>
>>
>>> Another problematic situation is when you need to actually downgrade a
>>> cumulative patch. Should be rare, but...
>>
>> Good point. Well, especially the callbacks should be rare.
> 
> Yes, we will probably use them for the most important fixes only and
> only if there are no other suitable options. The patches with could be
> more difficult to maintain anyway.
> 
>>
>> I would like to hear from people that have some experience
>> or plans with using callbacks and cumulative patches.
>>
>> I wonder how they plan to technically reuse a feature
>> in the cummulative patches. IMHO, it should be very
>> rare to add/remove callbacks. Then it might be safe
>> to downgrade a cummulative patch if the callbacks
>> are exactly the same.
>>
>>> Well, I think we will disable the old patches explicitly in these cases,
>>> before loading of the new one. May be fragile but easier to maintain.
>>
>> I am afraid that we will not be able to support all use cases
>> and keep the code sane.
> 
> Thats is OK.
> 
> I agree with you that the current behaviour of the 'replace' operation
> w.r.t. patch callbacks should stay as is. Making kernel code more
> complex than it should be is definitely a bad thing.
> 
> I cannot say much about generic policies for cumulative/non-cumulative
> patches here. In our particular case (Virtuozzo ReadyKernel), the
> cumulative patches are easily recognizable by their names, they have
> 'cumulative' substring there. Patch version is also included in the name
> and is easily obtainable.
> 
> So I think, I'll update our userspace tools (based on kpatch) so that
> their 'replace' command worked as follows:
> 
> * Explicitly disable all loaded non-cumulative patches first.
> * If the new patch is non-cumulative, just disable all loaded patches
> explicitly and then load the new one.
> * If the new patch is cumulative, explicitly disable the loaded
> cumulative patches that have smaller version numbers than the new one
> (if any). Then run replace as usual.
> 
> This way, the in-kernel 'replace' functionality will only be used when
> upgrading cumulative patches - and we can design their callbacks (if the
> callbacks are needed at some point) according to the current behaviour.
> 
> Patch downgrade operations should be very rare and could only happen if
> something goes wrong at the users' side. I think, it is acceptable to
> disable the loaded patch first in that case. Its callbacks, if present,
> will do proper cleanup. Then the older, "good" patch can be loaded
> normally. If such things happen, we'll have to investigate the affected
> nodes anyway.
> 
> Same for the custom (non-cumulative) patches. We sometimes use them to
> "patch things up" quickly before the cumulative patch with the
> appropriate fixes is officially released. It should be reasonably safe
> to disable these patches explicitly when loading a new cumulative patch.
> 
> So, I think, the current situation with 'replace' & callbacks is
> acceptable for us.
>

Our main interest in 'atomic replace' is simply to make sure that
cumulative patches work as expected in that they 'replace' any prior
patches. We have an interest primarily in being able to apply patches
from the stable trees via livepatch. I think the current situation with
respect to 'replace' and callbacks is fine for us as well, but could
change based on more experience with livepatch.

As an aside I was just wondering how you are making use of the callbacks
using the tool you mentioned (that is based on kpatch)? I see in the
upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and
'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as
opposed to livepatch), and I do not see these sort of macros for the
recently introduced livepatch callbacks. It seems it would be easy
enough to add similar hooks for the livepatch callbacks. I was thinking
of writing such a patch, but was wondering if there was an existing
solution?

Thanks,

-Jason

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Joe Lawrence]

On 01/30/2018 01:19 PM, Jason Baron wrote:
> [ ... snip ... ]
>
> Our main interest in 'atomic replace' is simply to make sure that
> cumulative patches work as expected in that they 'replace' any prior
> patches. We have an interest primarily in being able to apply patches
> from the stable trees via livepatch. I think the current situation with
> respect to 'replace' and callbacks is fine for us as well, but could
> change based on more experience with livepatch.

Well the callback implementation was based somewhat on theoretical
usage..  it was inspired by the kpatch macros that you talk about below,
in which we had a few specific use-cases.  Converting (un)patch
notifiers to the livepatch model presented additional callback
locations, and as such we ended up with pre-patch, post-patch,
pre-unpatch and post-unpatch callbacks.  Hopefully we'll get a better
idea of their worth as we gain experience using them.  At this point in
time I would suggest keeping it as simple and safe as possible.  :)

> As an aside I was just wondering how you are making use of the callbacks
> using the tool you mentioned (that is based on kpatch)? I see in the
> upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and
> 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as
> opposed to livepatch), and I do not see these sort of macros for the
> recently introduced livepatch callbacks. It seems it would be easy
> enough to add similar hooks for the livepatch callbacks. I was thinking
> of writing such a patch, but was wondering if there was an existing
> solution?

I've got a work in progress PR for this very feature:
https://github.com/dynup/kpatch/pull/780

Evgenii has already provided some helpful feedback. Another set of
eyes/testing would be appreciated!

Regards,

-- Joe

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Jason Baron]

On 01/30/2018 02:24 PM, Joe Lawrence wrote:
> On 01/30/2018 01:19 PM, Jason Baron wrote:
>> [ ... snip ... ]
>>
>> Our main interest in 'atomic replace' is simply to make sure that
>> cumulative patches work as expected in that they 'replace' any prior
>> patches. We have an interest primarily in being able to apply patches
>> from the stable trees via livepatch. I think the current situation with
>> respect to 'replace' and callbacks is fine for us as well, but could
>> change based on more experience with livepatch.
> 
> Well the callback implementation was based somewhat on theoretical
> usage..  it was inspired by the kpatch macros that you talk about below,
> in which we had a few specific use-cases.  Converting (un)patch
> notifiers to the livepatch model presented additional callback
> locations, and as such we ended up with pre-patch, post-patch,
> pre-unpatch and post-unpatch callbacks.  Hopefully we'll get a better
> idea of their worth as we gain experience using them.  At this point in
> time I would suggest keeping it as simple and safe as possible.  :)
> 
>> As an aside I was just wondering how you are making use of the callbacks
>> using the tool you mentioned (that is based on kpatch)? I see in the
>> upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and
>> 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as
>> opposed to livepatch), and I do not see these sort of macros for the
>> recently introduced livepatch callbacks. It seems it would be easy
>> enough to add similar hooks for the livepatch callbacks. I was thinking
>> of writing such a patch, but was wondering if there was an existing
>> solution?
> 
> I've got a work in progress PR for this very feature:
> https://github.com/dynup/kpatch/pull/780
> 
> Evgenii has already provided some helpful feedback. Another set of
> eyes/testing would be appreciated!

Cool - this would be a nice feature to add. I've added some comments at
the above link.

Thanks,

-Jason

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Evgenii Shatokhin]

On 30.01.2018 22:24, Joe Lawrence wrote:
> On 01/30/2018 01:19 PM, Jason Baron wrote:
>> [ ... snip ... ]
>>
>> Our main interest in 'atomic replace' is simply to make sure that
>> cumulative patches work as expected in that they 'replace' any prior
>> patches. We have an interest primarily in being able to apply patches
>> from the stable trees via livepatch. I think the current situation with
>> respect to 'replace' and callbacks is fine for us as well, but could
>> change based on more experience with livepatch.
> 
> Well the callback implementation was based somewhat on theoretical
> usage..  it was inspired by the kpatch macros that you talk about below,
> in which we had a few specific use-cases.  Converting (un)patch
> notifiers to the livepatch model presented additional callback
> locations, and as such we ended up with pre-patch, post-patch,
> pre-unpatch and post-unpatch callbacks.  Hopefully we'll get a better
> idea of their worth as we gain experience using them.  At this point in
> time I would suggest keeping it as simple and safe as possible.  :)
> 
>> As an aside I was just wondering how you are making use of the callbacks
>> using the tool you mentioned (that is based on kpatch)? I see in the
>> upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and
>> 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as
>> opposed to livepatch), and I do not see these sort of macros for the
>> recently introduced livepatch callbacks. It seems it would be easy
>> enough to add similar hooks for the livepatch callbacks. I was thinking
>> of writing such a patch, but was wondering if there was an existing
>> solution?
> 
> I've got a work in progress PR for this very feature:
> https://github.com/dynup/kpatch/pull/780

Yes, exactly, I use that. It is an important piece of the puzzle.

> 
> Evgenii has already provided some helpful feedback. Another set of
> eyes/testing would be appreciated!
> 
> Regards,
> 
> -- Joe
> .
> 

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Miroslav Benes]

> IMHO, it might be easier to run just the callbacks from
> the new patch. In reality, the author should always know
> what it might be replacing and what needs to be done.

I agree. I don't see a good solution to the problem. Imagine a crazy 
situation in which someone would like to patch the entry code. Callbacks 
would be non-empty, because of the need to write to MSR and IDT. Then 
there might be an atomic replace cumulative patch. You don't want to run 
callbacks of the previous patches, because that would make the system 
vulnerable again. On the other hand, callbacks in the replace patch could 
do what is necessary.

It is not only entry patching of course. I think it is valid everytime you 
need to write a value to a global system variable of some sort.
 
> By other words, it might be much easier to handle all
> situations in a single script in the new patch. Alternative
> would be doing crazy hacks to prevent the older scripts from
> destroying what we would like to keep. We would need to
> keep in mind interactions between the scripts and
> the order in which they are called.

Crazy, yes.

> Or do you plan to use cumulative patches to simply
> get rid of any other "random" livepatches with something
> completely different? In this case, it might be much more
> safe to disable the older patches a normal way.
> 
> I would suggest to just document the current behavior.
> We should create Documentation/livepatch/cummulative-patches.txt
> anyway.

Agreed.

Regards,
Miroslav

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Josh Poimboeuf]

On Fri, Jan 26, 2018 at 11:23:26AM +0100, Petr Mladek wrote:
> So, we are talking about a lot of rather non-trivial code.
> IMHO, it might be easier to run just the callbacks from
> the new patch. In reality, the author should always know
> what it might be replacing and what needs to be done.
> 
> By other words, it might be much easier to handle all
> situations in a single script in the new patch. Alternative
> would be doing crazy hacks to prevent the older scripts from
> destroying what we would like to keep. We would need to
> keep in mind interactions between the scripts and
> the order in which they are called.
> 
> Or do you plan to use cumulative patches to simply
> get rid of any other "random" livepatches with something
> completely different? In this case, it might be much more
> safe to disable the older patches a normal way.
> 
> I would suggest to just document the current behavior.
> We should create Documentation/livepatch/cummulative-patches.txt
> anyway.

+1.  Only the new patch (hopefully) knows the details about what is
being reverted.  So it makes sense to ignore the replaced patch's
callbacks, and to only call the new patch's callbacks.

-- 
Josh

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Joe Lawrence]

On 01/30/2018 09:03 AM, Petr Mladek wrote:
> On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote:
>>
>> In my experience, it was quite convenient sometimes to just "replace all
>> binary patches the user currently has loaded with this single one". No
>> matter what these original binary patches did and where they came from.
> 
> To be honest, I would feel better if the livepatch framework is
> more safe. It should prevent breaking the system by a patch
> that atomically replaces other random patches that rely on callbacks.
> 
> Well, combining random livepatches from random vendors is a call
> for troubles. It might easily fail when two patches add
> different changes to the same function.
> 
> I wonder if we should introduce some tags, keys, vendors. I mean
> to define an identification or dependencies that would say that some
> patches are compatible or incompatible.
> 
> We could allow to livepatch one function by two livepatches
> only if they are from the same vendor. But then the order still
> might be important. Also I am not sure if this condition is safe
> enough.
> 
> One the other hand, we could handle callbacks like the shadow
> variables. Every shadow variable has an ID. We have an API to
> add/read/update/remove them. We might allow to have more
> callbacks with different IDs. Then we could run callbacks
> only for IDs that are newly added or removed. Sigh, this would
> be very complex implementation as well. But it might make
> these features easier to use and maintain.

Interesting ideas, but I wonder if this could be accomplished at the
livepatch module level?  ie, leave it to kpatch-build (or a livepatch
equivalent) to produce a module that does this automatically.  I guess
it would then be completely opt-in checking, but transfers the
complexity out of the kernel livepatching core.

I don't see a simple way to provide flexibility of when/if calling
redundant callbacks without making the code even more complicated.  I
don't have any use-cases off hand that would require such features, but
I guess if I did absolutely needed them, I might be inclined to say
prepare ahead of time and write callbacks so that they may be disabled
externally -- either by a new livepatch module's init() or some other
means.  Then whatever crazy policy I need is contained to my modules,
not provided or enforced by the kernel.

-- Joe

Re: [PATCH v5 0/3] livepatch: introduce atomic replace

[Petr Mladek]

On Wed 2018-01-31 17:09:21, Joe Lawrence wrote:
> On 01/30/2018 09:03 AM, Petr Mladek wrote:
> > On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote:
> >>
> >> In my experience, it was quite convenient sometimes to just "replace all
> >> binary patches the user currently has loaded with this single one". No
> >> matter what these original binary patches did and where they came from.
> > 
> > To be honest, I would feel better if the livepatch framework is
> > more safe. It should prevent breaking the system by a patch
> > that atomically replaces other random patches that rely on callbacks.
> > 
> > Well, combining random livepatches from random vendors is a call
> > for troubles. It might easily fail when two patches add
> > different changes to the same function.
> > 
> > I wonder if we should introduce some tags, keys, vendors. I mean
> > to define an identification or dependencies that would say that some
> > patches are compatible or incompatible.
> > 
> > We could allow to livepatch one function by two livepatches
> > only if they are from the same vendor. But then the order still
> > might be important. Also I am not sure if this condition is safe
> > enough.
> > 
> > One the other hand, we could handle callbacks like the shadow
> > variables. Every shadow variable has an ID. We have an API to
> > add/read/update/remove them. We might allow to have more
> > callbacks with different IDs. Then we could run callbacks
> > only for IDs that are newly added or removed. Sigh, this would
> > be very complex implementation as well. But it might make
> > these features easier to use and maintain.
> 
> Interesting ideas, but I wonder if this could be accomplished at the
> livepatch module level?  ie, leave it to kpatch-build (or a livepatch
> equivalent) to produce a module that does this automatically.  I guess
> it would then be completely opt-in checking, but transfers the
> complexity out of the kernel livepatching core.
>
> I don't see a simple way to provide flexibility of when/if calling
> redundant callbacks without making the code even more complicated.  I
> don't have any use-cases off hand that would require such features, but
> I guess if I did absolutely needed them, I might be inclined to say
> prepare ahead of time and write callbacks so that they may be disabled
> externally -- either by a new livepatch module's init() or some other
> means.  Then whatever crazy policy I need is contained to my modules,
> not provided or enforced by the kernel.

This all sounds reasonable. I think that we simply need to get some
more experience with all these features. Then we will see if a better
support from kernel would be needed or helpful.

I'll try to come up with some documentation for the atomic replace
and describe the potential problems and approaches there.

Best Regards,
Petr