[PATCH v3 0/2] livepatch: introduce atomic replace

[PATCH v3 0/2] livepatch: introduce atomic replace

[Jason Baron]

Hi,

In testing livepatch, I found that when doing cumulative patches, if a patched
function is completed reverted by a subsequent patch (back to its original state)
livepatch does not revert the funtion to its original state. Specifically, if
patch A introduces a change to function 1, and patch B reverts the change to
function 1 and introduces changes to say function 2 and 3 as well, the change
that patch A 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

Patches:

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

2) livepatch: add atomic replace
Core feature. Note that __klp_enable_patch() calls klp_add_nops(), which
necessitated moving a bunch of existing functions before __klp_enable_patch().
So there is a bit of churn in moving functions that are not modified.

Thanks,

-Jason

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 (2):
  livepatch: Add dynamic klp_object and klp_func iterators
  livepatch: add atomic replace

 include/linux/livepatch.h     |  85 +++--
 kernel/livepatch/core.c       | 738 ++++++++++++++++++++++++++++++++----------
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 +-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 ++-
 6 files changed, 685 insertions(+), 220 deletions(-)

-- 
2.6.1

[PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Jason Baron]

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

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991e..e03ce11 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)
 
@@ -36,18 +37,20 @@
 
 /**
  * struct klp_func - function structure for live patching
- * @old_name:	name of the function to be patched
- * @new_func:	pointer to the patched function code
- * @old_sympos: a hint indicating which symbol position the old function
- *		can be found (optional)
- * @immediate:  patch the func immediately, bypassing safety mechanisms
- * @old_addr:	the address of the function being patched
- * @kobj:	kobject for sysfs resources
- * @stack_node:	list node for klp_ops func_stack list
- * @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
- * @transition:	the func is currently being applied or reverted
+ * @old_name:	    name of the function to be patched
+ * @new_func:	    pointer to the patched function code
+ * @old_sympos:	    a hint indicating which symbol position the old function
+ *		    can be found (optional)
+ * @immediate:	    patch the func immediately, bypassing safety mechanisms
+ * @old_addr:	    the address of the function being patched
+ * @kobj:	    kobject for sysfs resources
+ * @stack_node:	    list node for klp_ops func_stack list
+ * @nop_func_entry: links dynamically allocated 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
+ * @transition:	    the func is currently being applied or reverted
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:
@@ -82,6 +85,7 @@ struct klp_func {
 	unsigned long old_addr;
 	struct kobject kobj;
 	struct list_head stack_node;
+	struct list_head nop_func_entry;
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
@@ -89,12 +93,15 @@ struct klp_func {
 
 /**
  * struct klp_object - kernel object structure for live patching
- * @name:	module name (or NULL for vmlinux)
- * @funcs:	function entries for functions to be patched in the object
- * @kobj:	kobject for sysfs resources
- * @mod:	kernel module associated with the patched object
- *		(NULL for vmlinux)
- * @patched:	the object's funcs have been added to the klp_ops list
+ * @name:	   module name (or NULL for vmlinux)
+ * @funcs:	   function entries for functions to be patched in the object
+ * @kobj:	   kobject for sysfs resources
+ * @nop_func_list: head of list for dynamically allocated struct klp_func
+ * @nop_obj_entry: links dynamically allocated struct klp_object to struct
+ *		   klp_patch
+ * @mod:	   kernel module associated with the patched object
+ *		   (NULL for vmlinux)
+ * @patched:	   the object's funcs have been added to the klp_ops list
  */
 struct klp_object {
 	/* external */
@@ -103,19 +110,22 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head nop_func_list;
+	struct list_head nop_obj_entry;
 	struct module *mod;
 	bool patched;
 };
 
 /**
  * struct klp_patch - patch structure for live patching
- * @mod:	reference to the live patch module
- * @objs:	object entries for kernel objects to be patched
- * @immediate:  patch all funcs immediately, bypassing safety mechanisms
- * @list:	list node for global list of registered patches
- * @kobj:	kobject for sysfs resources
- * @enabled:	the patch is enabled (but operation may be incomplete)
- * @finish:	for waiting till it is safe to remove the patch module
+ * @mod:	  reference to the live patch module
+ * @objs:	  object entries for kernel objects to be patched
+ * @immediate:	  patch all funcs immediately, bypassing safety mechanisms
+ * @list:	  list node for global list of registered patches
+ * @kobj:	  kobject for sysfs resources
+ * @nop_obj_list: head of list for dynamically allocated struct klp_object
+ * @enabled:	  the patch is enabled (but operation may be incomplete)
+ * @finish:	  for waiting till it is safe to remove the patch module
  */
 struct klp_patch {
 	/* external */
@@ -126,17 +136,26 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head nop_obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
-#define klp_for_each_object(patch, obj) \
-	for (obj = patch->objs; obj->funcs || obj->name; obj++)
+struct klp_object *klp_obj_iter_init(struct klp_patch *patch);
+struct klp_object *klp_obj_iter_next(struct klp_patch *patch,
+				     struct klp_object *obj);
 
-#define klp_for_each_func(obj, func) \
-	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
-	     func++)
+#define klp_for_each_object(patch, obj)		  \
+	for (obj = klp_obj_iter_init(patch); obj; \
+	     obj = klp_obj_iter_next(patch, obj))
+
+struct klp_func *klp_func_iter_init(struct klp_object *obj);
+struct klp_func *klp_func_iter_next(struct klp_object *obj,
+				    struct klp_func *func);
+
+#define klp_for_each_func(obj, func)		   \
+	for (func = klp_func_iter_init(obj); func; \
+	     func = klp_func_iter_next(obj, func))
 
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e4..0d92fe6 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,6 +49,100 @@ static LIST_HEAD(klp_patches);
 
 static struct kobject *klp_root_kobj;
 
+static bool klp_valid_obj_entry(struct klp_object *obj)
+{
+	return obj->funcs || obj->name;
+}
+
+struct klp_object *klp_obj_iter_init(struct klp_patch *patch)
+{
+	if (klp_valid_obj_entry(patch->objs))
+		return patch->objs;
+
+	return NULL;
+}
+
+struct klp_object *klp_obj_iter_next(struct klp_patch *patch,
+				 struct klp_object *obj)
+{
+	struct klp_object *next_obj = NULL;
+
+	/*
+	 * Statically defined objects are in NULL-ended array.
+	 * Only dynamic ones are in the nop_obj_list.
+	 */
+	if (list_empty(&obj->nop_obj_entry)) {
+		next_obj = obj + 1;
+		if (klp_valid_obj_entry(next_obj))
+			goto out;
+		next_obj = NULL;
+		if (!list_empty(&patch->nop_obj_list))
+			next_obj = list_entry(patch->nop_obj_list.next,
+					      struct klp_object,
+					      nop_obj_entry);
+		goto out;
+	}
+
+	if (obj->nop_obj_entry.next != &patch->nop_obj_list)
+		next_obj = list_entry(obj->nop_obj_entry.next,
+				      struct klp_object,
+				      nop_obj_entry);
+
+out:
+	return next_obj;
+}
+
+static bool klp_valid_func_entry(struct klp_func *func)
+{
+	return func->old_name || func->new_func || func->old_sympos;
+}
+
+struct klp_func *klp_func_iter_init(struct klp_object *obj)
+{
+	/* statically allocated */
+	if (list_empty(&obj->nop_obj_entry)) {
+		if (klp_valid_func_entry(obj->funcs))
+			return obj->funcs;
+	} else {
+		if (!list_empty(obj->nop_func_list.next))
+			return list_entry(obj->nop_func_list.next,
+					  struct klp_func,
+					  nop_func_entry);
+	}
+
+	return NULL;
+}
+
+struct klp_func *klp_func_iter_next(struct klp_object *obj,
+				    struct klp_func *func)
+{
+	struct klp_func *next_func = NULL;
+
+	/*
+	 * Statically defined functions are in NULL-ended array.
+	 * Only dynamic ones are in the nop_func_list.
+	 */
+	if (list_empty(&func->nop_func_entry)) {
+		next_func = func + 1;
+		if (klp_valid_func_entry(next_func))
+			goto out;
+		next_func = NULL;
+		if (!list_empty(&obj->nop_func_list))
+			next_func = list_entry(obj->nop_func_list.next,
+					       struct klp_func,
+					       nop_func_entry);
+		goto out;
+	}
+
+	if (func->nop_func_entry.next != &obj->nop_func_list)
+		next_func = list_entry(func->nop_func_entry.next,
+				       struct klp_func,
+				       nop_func_entry);
+
+out:
+	return next_func;
+}
+
 static bool klp_is_module(struct klp_object *obj)
 {
 	return obj->name;
@@ -709,6 +803,20 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	return ret;
 }
 
+static void klp_init_patch_dyn(struct klp_patch *patch)
+{
+	struct klp_object *obj;
+	struct klp_func *func;
+
+	INIT_LIST_HEAD(&patch->nop_obj_list);
+	klp_for_each_object(patch, obj) {
+		INIT_LIST_HEAD(&obj->nop_obj_entry);
+		INIT_LIST_HEAD(&obj->nop_func_list);
+		klp_for_each_func(obj, func)
+			INIT_LIST_HEAD(&func->nop_func_entry);
+	}
+}
+
 static int klp_init_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -729,6 +837,8 @@ static int klp_init_patch(struct klp_patch *patch)
 		return ret;
 	}
 
+	klp_init_patch_dyn(patch);
+
 	klp_for_each_object(patch, obj) {
 		ret = klp_init_object(patch, obj);
 		if (ret)
-- 
2.6.1

[PATCH v3 2/2] livepatch: add atomic replace

[Jason Baron]

Sometimes we would like to revert a particular fix. This is currently
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.

Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving
a bunch of existing functions before __klp_enable_patch(). So there is a
bit of churn in moving functions that are not modified.

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e03ce11..93e8d44 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -51,6 +51,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:
@@ -89,6 +90,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool nop;
 };
 
 /**
@@ -121,6 +123,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
  * @nop_obj_list: head of list for dynamically allocated struct klp_object
@@ -132,6 +135,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 0d92fe6..f29da1b 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;
 
@@ -181,17 +188,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;
@@ -377,6 +395,413 @@ 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 = {
+	.release = klp_kobj_release_object,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+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 = {
+	.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);
+}
+
+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->nop_func_list,
+					 nop_func_entry) {
+			list_del_init(&func->nop_func_entry);
+			atomic_inc(&klp_nop_release_count);
+			kobject_put(&func->kobj);
+		}
+		INIT_LIST_HEAD(&obj->nop_func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->nop_obj_list,
+				 nop_obj_entry) {
+		list_del_init(&obj->nop_obj_entry);
+		atomic_inc(&klp_nop_release_count);
+		kobject_put(&obj->kobj);
+	}
+	INIT_LIST_HEAD(&patch->nop_obj_list);
+
+	wait_event(klp_nop_release_wait,
+		   atomic_read(&klp_nop_release_count) == 0);
+}
+
+static int klp_init_func(struct klp_object *obj, struct klp_func *func,
+			 bool nop)
+{
+	if (!func->old_name || (!nop && !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,
+			   bool nop)
+{
+	struct klp_func *func;
+	int ret;
+	const char *name;
+
+	if (!obj->funcs && !nop)
+		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;
+
+	if (nop)
+		return 0;
+
+	klp_for_each_func(obj, func) {
+		ret = klp_init_func(obj, func, false);
+		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 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->nop_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) {
+		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);
+
+	INIT_LIST_HEAD(&obj->nop_func_list);
+	INIT_LIST_HEAD(&obj->nop_obj_entry);
+	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);
+	list_add(&func->nop_func_entry, &obj->nop_func_list);
+
+	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);
+		list_add(&obj->nop_obj_entry, &patch->nop_obj_list);
+	}
+	klp_for_each_func(old_obj, old_func) {
+		err = klp_add_nop_func(obj, old_func);
+		if (err)
+			return err;
+	}
+
+	return 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)
@@ -441,6 +866,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;
@@ -448,10 +874,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
@@ -462,8 +900,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);
 
@@ -497,6 +937,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;
 }
 
 /**
@@ -514,7 +960,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;
 	}
@@ -553,7 +999,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.
@@ -630,179 +1076,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;
-
-	/* 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 void klp_init_patch_dyn(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -840,7 +1113,7 @@ static int klp_init_patch(struct klp_patch *patch)
 	klp_init_patch_dyn(patch);
 
 	klp_for_each_object(patch, obj) {
-		ret = klp_init_object(patch, obj);
+		ret = klp_init_object(patch, obj, false);
 		if (ret)
 			goto free;
 	}
@@ -886,6 +1159,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto err;
 	}
 
+	klp_remove_nops(patch);
 	klp_free_patch(patch);
 
 	mutex_unlock(&klp_mutex);
@@ -1039,7 +1313,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 v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Josh Poimboeuf]

On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote:
> In preparation to introducing atomic replace, introduce iterators for
> klp_func and klp_object, such that objects and functions can be dynamically
> allocated (needed for atomic replace). This patch is intended to
> effectively be a no-op until atomic replace is introduced.

Hi Jason,

Sorry for coming in so late on this.  This will be a nice feature.  I
haven't gotten to the second patch yet, but I have a few comments
already.

1) Having both static *and* dynamic lists seems to add a lot of
   complexity and brittleness.

   The livepatch API is easier to use with static data structures, so I
   don't think we should change the API.  But I also don't think we
   should allow the API to overly constrain our internal representation
   of the data.

   What I'd *really* like to do is completely separate the user-supplied
   data structures from our internal data .  At registration time, we
   can convert all the arrays to dynamic lists, and then never touch the
   arrays again.  Then managing the lists is completely straightforward.

   That would also allow us to have an external public struct and an
   internal private struct, so the interface is more clear and we don't
   have to worry about any patch modules accidentally messing with our
   private data.

2) There seems to be a general consensus that the cumulative "replace"
   model is the best one.  In light of that, I wonder if we can simplify
   our code a bit.  Specifically, can we get rid of the func stack?

   If the user always uses replace, then the func stack will never have
   more than one func.  And we can reject a patch if the user doesn't
   use replace when they're trying to patch a function which has already
   been patched.

   Then the func stack doesn't have to be a stack, it can just be a
   single func.

   That would allow us to simplify the code in several places for the
   common case, yet still allow those who want to apply non-cumulative
   patches to do so most of the time.

   It's not a *huge* simplification, but we've already made livepatch so
   flexible that it's too complex to fit everything in your head at the
   same time, and any simplification we can get away with is well worth
   it IMO.  And once we have replace, I wonder if anybody would really
   miss the func stack.

Thoughts?

-- 
Josh

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Josh Poimboeuf]

On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
> 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

I don't really like allowing a previously replaced patch to replace the
current patch.  It's just more unnecessary complexity.  If the user
wants to atomically revert back to kpatch-a, they should be able to:

  rmmod kpatch-a
  insmod kpatch-a.ko

> 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.

If you don't allow a previously replaced patch to be enabled again, I
think it would be trivial to let it be unloaded.

> Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving
> a bunch of existing functions before __klp_enable_patch(). So there is a
> bit of churn in moving functions that are not modified.

To make review easier, can you put the moving of functions into a
separate patch?

-- 
Josh

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Jason Baron]

On 10/06/2017 05:22 PM, Josh Poimboeuf wrote:
> On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote:
>> In preparation to introducing atomic replace, introduce iterators for
>> klp_func and klp_object, such that objects and functions can be dynamically
>> allocated (needed for atomic replace). This patch is intended to
>> effectively be a no-op until atomic replace is introduced.
> 
> Hi Jason,
> 
> Sorry for coming in so late on this.  This will be a nice feature.  I
> haven't gotten to the second patch yet, but I have a few comments
> already.
> 


Hi Josh,

> 1) Having both static *and* dynamic lists seems to add a lot of
>    complexity and brittleness.
> 
>    The livepatch API is easier to use with static data structures, so I
>    don't think we should change the API.  But I also don't think we
>    should allow the API to overly constrain our internal representation
>    of the data.
> 
>    What I'd *really* like to do is completely separate the user-supplied
>    data structures from our internal data .  At registration time, we
>    can convert all the arrays to dynamic lists, and then never touch the
>    arrays again.  Then managing the lists is completely straightforward.
> 
>    That would also allow us to have an external public struct and an
>    internal private struct, so the interface is more clear and we don't
>    have to worry about any patch modules accidentally messing with our
>    private data.
> 

Separating the external/internal data structures makes sense. Another
idea here would be to simply walk the static data structures at register
time and then add them onto the dynamic lists. Then, all subsequent
access can be done using the simple list walks. That would remove the
complex iterators here. The external/internal division could still be
done later but maybe doesn't couple as much to this patchset...

> 2) There seems to be a general consensus that the cumulative "replace"
>    model is the best one.  In light of that, I wonder if we can simplify
>    our code a bit.  Specifically, can we get rid of the func stack?
> 
>    If the user always uses replace, then the func stack will never have
>    more than one func.  And we can reject a patch if the user doesn't
>    use replace when they're trying to patch a function which has already
>    been patched.
> 
>    Then the func stack doesn't have to be a stack, it can just be a
>    single func.
> 
>    That would allow us to simplify the code in several places for the
>    common case, yet still allow those who want to apply non-cumulative
>    patches to do so most of the time.
> 
>    It's not a *huge* simplification, but we've already made livepatch so
>    flexible that it's too complex to fit everything in your head at the
>    same time, and any simplification we can get away with is well worth
>    it IMO.  And once we have replace, I wonder if anybody would really
>    miss the func stack.
> 

I think that means instead of having a list of functions or the func
stack, you have 2 pointers - one to the active patch and one to the
previous patch. Maybe it would make sense to do in steps? IE have this
patchset reject patches that patch a patch to existing function, leaving
the func stack. And if that doesn't break any use-cases, switch to a
model where there is just an active and previous patch?

Thanks,

-Jason

[PATCH v3.1 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 | 346 ++++++++++++++++++++++++------------------------
 1 file changed, 173 insertions(+), 173 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 0d92fe6..871ea88 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -377,6 +377,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)
@@ -630,179 +803,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;
-
-	/* 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 void klp_init_patch_dyn(struct klp_patch *patch)
 {
 	struct klp_object *obj;
-- 
2.6.1

[PATCH v3.1 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       | 306 +++++++++++++++++++++++++++++++++++++++---
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 ++-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 ++++++-
 6 files changed, 364 insertions(+), 28 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e03ce11..93e8d44 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -51,6 +51,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:
@@ -89,6 +90,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool nop;
 };
 
 /**
@@ -121,6 +123,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
  * @nop_obj_list: head of list for dynamically allocated struct klp_object
@@ -132,6 +135,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 871ea88..f29da1b 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;
 
@@ -181,17 +188,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;
@@ -377,8 +395,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 = {
@@ -388,6 +419,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 = {
@@ -441,9 +482,36 @@ static void klp_free_patch(struct klp_patch *patch)
 		list_del(&patch->list);
 }
 
-static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+void klp_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->nop_func_list,
+					 nop_func_entry) {
+			list_del_init(&func->nop_func_entry);
+			atomic_inc(&klp_nop_release_count);
+			kobject_put(&func->kobj);
+		}
+		INIT_LIST_HEAD(&obj->nop_func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->nop_obj_list,
+				 nop_obj_entry) {
+		list_del_init(&obj->nop_obj_entry);
+		atomic_inc(&klp_nop_release_count);
+		kobject_put(&obj->kobj);
+	}
+	INIT_LIST_HEAD(&patch->nop_obj_list);
+
+	wait_event(klp_nop_release_wait,
+		   atomic_read(&klp_nop_release_count) == 0);
+}
+
+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);
@@ -510,13 +578,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;
@@ -530,8 +599,11 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (ret)
 		return ret;
 
+	if (nop)
+		return 0;
+
 	klp_for_each_func(obj, func) {
-		ret = klp_init_func(obj, func);
+		ret = klp_init_func(obj, func, false);
 		if (ret)
 			goto free;
 	}
@@ -550,6 +622,186 @@ 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->nop_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) {
+		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);
+
+	INIT_LIST_HEAD(&obj->nop_func_list);
+	INIT_LIST_HEAD(&obj->nop_obj_entry);
+	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);
+	list_add(&func->nop_func_entry, &obj->nop_func_list);
+
+	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);
+		list_add(&obj->nop_obj_entry, &patch->nop_obj_list);
+	}
+	klp_for_each_func(old_obj, old_func) {
+		err = klp_add_nop_func(obj, old_func);
+		if (err)
+			return err;
+	}
+
+	return 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)
@@ -614,6 +866,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;
@@ -621,10 +874,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
@@ -635,8 +900,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);
 
@@ -670,6 +937,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;
 }
 
 /**
@@ -687,7 +960,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;
 	}
@@ -726,7 +999,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.
@@ -840,7 +1113,7 @@ static int klp_init_patch(struct klp_patch *patch)
 	klp_init_patch_dyn(patch);
 
 	klp_for_each_object(patch, obj) {
-		ret = klp_init_object(patch, obj);
+		ret = klp_init_object(patch, obj, false);
 		if (ret)
 			goto free;
 	}
@@ -886,6 +1159,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto err;
 	}
 
+	klp_remove_nops(patch);
 	klp_free_patch(patch);
 
 	mutex_unlock(&klp_mutex);
@@ -1039,7 +1313,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 v3 2/2] livepatch: add atomic replace

[Jason Baron]

On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
>> 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
> 
> I don't really like allowing a previously replaced patch to replace the
> current patch.  It's just more unnecessary complexity.  If the user
> wants to atomically revert back to kpatch-a, they should be able to:
> 
>   rmmod kpatch-a
>   insmod kpatch-a.ko
>

Right - that's how I sent v1 (using rmmod/insmod to revert), but it
didn't account for the fact the patch or some functions may be marked
'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
some cases 'rmmod' was not feasible, I thought it would be simpler from
an operational pov to just say we always revert by re-enabling a
previously replaced patch as opposed to rmmod/insmod.


>> 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.
> 
> If you don't allow a previously replaced patch to be enabled again, I
> think it would be trivial to let it be unloaded.
>

The concern is around being replaced by 'immediate' functions and thus
not knowing if the code is still in use.

>> Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving
>> a bunch of existing functions before __klp_enable_patch(). So there is a
>> bit of churn in moving functions that are not modified.
> 
> To make review easier, can you put the moving of functions into a
> separate patch?
> 

Agreed - I've re-posted to this thread as v3.1.

Thanks,

-Jason

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Josh Poimboeuf]

On Tue, Oct 10, 2017 at 11:15:33AM -0400, Jason Baron wrote:
> 
> 
> On 10/06/2017 05:22 PM, Josh Poimboeuf wrote:
> > On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote:
> >> In preparation to introducing atomic replace, introduce iterators for
> >> klp_func and klp_object, such that objects and functions can be dynamically
> >> allocated (needed for atomic replace). This patch is intended to
> >> effectively be a no-op until atomic replace is introduced.
> > 
> > Hi Jason,
> > 
> > Sorry for coming in so late on this.  This will be a nice feature.  I
> > haven't gotten to the second patch yet, but I have a few comments
> > already.
> > 
> 
> 
> Hi Josh,
> 
> > 1) Having both static *and* dynamic lists seems to add a lot of
> >    complexity and brittleness.
> > 
> >    The livepatch API is easier to use with static data structures, so I
> >    don't think we should change the API.  But I also don't think we
> >    should allow the API to overly constrain our internal representation
> >    of the data.
> > 
> >    What I'd *really* like to do is completely separate the user-supplied
> >    data structures from our internal data .  At registration time, we
> >    can convert all the arrays to dynamic lists, and then never touch the
> >    arrays again.  Then managing the lists is completely straightforward.
> > 
> >    That would also allow us to have an external public struct and an
> >    internal private struct, so the interface is more clear and we don't
> >    have to worry about any patch modules accidentally messing with our
> >    private data.
> > 
> 
> Separating the external/internal data structures makes sense. Another
> idea here would be to simply walk the static data structures at register
> time and then add them onto the dynamic lists. Then, all subsequent
> access can be done using the simple list walks. That would remove the
> complex iterators here. The external/internal division could still be
> done later but maybe doesn't couple as much to this patchset...

Sounds good.

> > 2) There seems to be a general consensus that the cumulative "replace"
> >    model is the best one.  In light of that, I wonder if we can simplify
> >    our code a bit.  Specifically, can we get rid of the func stack?
> > 
> >    If the user always uses replace, then the func stack will never have
> >    more than one func.  And we can reject a patch if the user doesn't
> >    use replace when they're trying to patch a function which has already
> >    been patched.
> > 
> >    Then the func stack doesn't have to be a stack, it can just be a
> >    single func.
> > 
> >    That would allow us to simplify the code in several places for the
> >    common case, yet still allow those who want to apply non-cumulative
> >    patches to do so most of the time.
> > 
> >    It's not a *huge* simplification, but we've already made livepatch so
> >    flexible that it's too complex to fit everything in your head at the
> >    same time, and any simplification we can get away with is well worth
> >    it IMO.  And once we have replace, I wonder if anybody would really
> >    miss the func stack.
> > 
> 
> I think that means instead of having a list of functions or the func
> stack, you have 2 pointers - one to the active patch and one to the
> previous patch. Maybe it would make sense to do in steps? IE have this
> patchset reject patches that patch a patch to existing function, leaving
> the func stack. And if that doesn't break any use-cases, switch to a
> model where there is just an active and previous patch?

Hm.  Maybe I didn't think it through.  I guess the func stack could have
at most *two* funcs -- not one.  Let's just forget this idea for now...

-- 
Josh

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Miroslav Benes]

On Tue, 10 Oct 2017, Josh Poimboeuf wrote:

> On Tue, Oct 10, 2017 at 11:15:33AM -0400, Jason Baron wrote:
> > 
> > 
> > On 10/06/2017 05:22 PM, Josh Poimboeuf wrote:
> > > On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote:
> > >> In preparation to introducing atomic replace, introduce iterators for
> > >> klp_func and klp_object, such that objects and functions can be dynamically
> > >> allocated (needed for atomic replace). This patch is intended to
> > >> effectively be a no-op until atomic replace is introduced.
> > > 
> > > Hi Jason,
> > > 
> > > Sorry for coming in so late on this.  This will be a nice feature.  I
> > > haven't gotten to the second patch yet, but I have a few comments
> > > already.
> > > 
> > 
> > 
> > Hi Josh,
> > 
> > > 1) Having both static *and* dynamic lists seems to add a lot of
> > >    complexity and brittleness.
> > > 
> > >    The livepatch API is easier to use with static data structures, so I
> > >    don't think we should change the API.  But I also don't think we
> > >    should allow the API to overly constrain our internal representation
> > >    of the data.
> > > 
> > >    What I'd *really* like to do is completely separate the user-supplied
> > >    data structures from our internal data .  At registration time, we
> > >    can convert all the arrays to dynamic lists, and then never touch the
> > >    arrays again.  Then managing the lists is completely straightforward.
> > > 
> > >    That would also allow us to have an external public struct and an
> > >    internal private struct, so the interface is more clear and we don't
> > >    have to worry about any patch modules accidentally messing with our
> > >    private data.
> > > 
> > 
> > Separating the external/internal data structures makes sense. Another
> > idea here would be to simply walk the static data structures at register
> > time and then add them onto the dynamic lists. Then, all subsequent
> > access can be done using the simple list walks. That would remove the
> > complex iterators here. The external/internal division could still be
> > done later but maybe doesn't couple as much to this patchset...
> 
> Sounds good.

I haven't seen v4 yet, but this seems to be the best approach so far.

I was the one who opposed external/internal division in the past, but 
given atomic replace feature we may end up there in the end. But I'd keep 
it separate.
 
> > > 2) There seems to be a general consensus that the cumulative "replace"
> > >    model is the best one.  In light of that, I wonder if we can simplify
> > >    our code a bit.  Specifically, can we get rid of the func stack?
> > > 
> > >    If the user always uses replace, then the func stack will never have
> > >    more than one func.  And we can reject a patch if the user doesn't
> > >    use replace when they're trying to patch a function which has already
> > >    been patched.
> > > 
> > >    Then the func stack doesn't have to be a stack, it can just be a
> > >    single func.
> > > 
> > >    That would allow us to simplify the code in several places for the
> > >    common case, yet still allow those who want to apply non-cumulative
> > >    patches to do so most of the time.
> > > 
> > >    It's not a *huge* simplification, but we've already made livepatch so
> > >    flexible that it's too complex to fit everything in your head at the
> > >    same time, and any simplification we can get away with is well worth
> > >    it IMO.  And once we have replace, I wonder if anybody would really
> > >    miss the func stack.
> > > 
> > 
> > I think that means instead of having a list of functions or the func
> > stack, you have 2 pointers - one to the active patch and one to the
> > previous patch. Maybe it would make sense to do in steps? IE have this
> > patchset reject patches that patch a patch to existing function, leaving
> > the func stack. And if that doesn't break any use-cases, switch to a
> > model where there is just an active and previous patch?
> 
> Hm.  Maybe I didn't think it through.  I guess the func stack could have
> at most *two* funcs -- not one.  Let's just forget this idea for now...

Yes, there would still be some kind of stacking needed just to make atomic 
replace work (if I understand you correctly).

Moreover, I'd be hesitant to remove stacking. Yes, we use cumulative 
patches at SUSE, because they correspond well to our particular use case. 
Some other company or even an ordinary user of linux might do things 
differently. If we removed it now, it would be almost impossible to add it 
later, because it is so complex. So if there's way to preserve it, I'd 
keep. As long as possible.

Regards,
Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

On Tue, 10 Oct 2017, Jason Baron wrote:

> 
> 
> On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> > On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
> >> 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
> > 
> > I don't really like allowing a previously replaced patch to replace the
> > current patch.  It's just more unnecessary complexity.  If the user
> > wants to atomically revert back to kpatch-a, they should be able to:
> > 
> >   rmmod kpatch-a
> >   insmod kpatch-a.ko
> >

I agree.
 
> Right - that's how I sent v1 (using rmmod/insmod to revert), but it
> didn't account for the fact the patch or some functions may be marked
> 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
> some cases 'rmmod' was not feasible, I thought it would be simpler from
> an operational pov to just say we always revert by re-enabling a
> previously replaced patch as opposed to rmmod/insmod.
> 
> 
> >> 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.
> > 
> > If you don't allow a previously replaced patch to be enabled again, I
> > think it would be trivial to let it be unloaded.
> >
> 
> The concern is around being replaced by 'immediate' functions and thus
> not knowing if the code is still in use.

Hm. Would it make sense to remove immediate and rely only on the 
consistency model? At least for the architectures where the model is 
implemented (x86_64)?

If not, then I'd keep such modules there without a possibility to remove 
them ever. If its functionality was required again, it would of course 
mean to insmod a new module with it.

Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

On Tue, 17 Oct 2017, Miroslav Benes wrote:

> On Tue, 10 Oct 2017, Jason Baron wrote:
> 
> > 
> > 
> > On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> > > On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
> > >> 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
> > > 
> > > I don't really like allowing a previously replaced patch to replace the
> > > current patch.  It's just more unnecessary complexity.  If the user
> > > wants to atomically revert back to kpatch-a, they should be able to:
> > > 
> > >   rmmod kpatch-a
> > >   insmod kpatch-a.ko
> > >
> 
> I agree.
>  
> > Right - that's how I sent v1 (using rmmod/insmod to revert), but it
> > didn't account for the fact the patch or some functions may be marked
> > 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
> > some cases 'rmmod' was not feasible, I thought it would be simpler from
> > an operational pov to just say we always revert by re-enabling a
> > previously replaced patch as opposed to rmmod/insmod.
> > 
> > 
> > >> 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.
> > > 
> > > If you don't allow a previously replaced patch to be enabled again, I
> > > think it would be trivial to let it be unloaded.
> > >
> > 
> > The concern is around being replaced by 'immediate' functions and thus
> > not knowing if the code is still in use.
> 
> Hm. Would it make sense to remove immediate and rely only on the 
> consistency model? At least for the architectures where the model is 
> implemented (x86_64)?
> 
> If not, then I'd keep such modules there without a possibility to remove 
> them ever. If its functionality was required again, it would of course 
> mean to insmod a new module with it.

Petr pointed out (offline) that immediate could still be useful. So let me 
describe how I envision the whole atomic replace semantics.

Let's have three functions - a, b, c. Patch 1 is immediate and patches 
a().

func		a	b	c
patches		1i

Patch 2 is not immediate and patches b()

func		a	b	c
patches		1i
			2

Patch 3 is atomic replace, which patches a() and c().

func		a	b	c
patches		1i
			2
		3r		3r

With 3 applied, 3versions of a() and c() are called, original b() is
called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2
cannot be reenabled.

3 can be disabled. Original a() and c() will be called in such case. If
one wants to have a() from patch 1 there, he would need to apply patch
4.

func		a	b	c
patches		1i
			2
		3r		3r
		4i

Does it make sense? This is what I would expect and I think it is easier
to implement. Whole initialization of nop functions could be done in
klp_init_ functions, if I am not mistaken.

Thoughts?

Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Petr Mladek]

On Tue 2017-10-17 11:02:29, Miroslav Benes wrote:
> On Tue, 10 Oct 2017, Jason Baron wrote:
> > On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> > > I don't really like allowing a previously replaced patch to replace the
> > > current patch.  It's just more unnecessary complexity.

I am sorry to say but it really makes the code too complex.

> > > If the user
> > > wants to atomically revert back to kpatch-a, they should be able to:
> > > 
> > >   rmmod kpatch-a
> > >   insmod kpatch-a.ko
> > >
> > Right - that's how I sent v1 (using rmmod/insmod to revert), but it
> > didn't account for the fact the patch or some functions may be marked
> > 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
> > some cases 'rmmod' was not feasible, I thought it would be simpler from
> > an operational pov to just say we always revert by re-enabling a
> > previously replaced patch as opposed to rmmod/insmod.
> > 
> Hm. Would it make sense to remove immediate and rely only on the 
> consistency model? At least for the architectures where the model is 
> implemented (x86_64)?
> 
> If not, then I'd keep such modules there without a possibility to remove 
> them ever. If its functionality was required again, it would of course 
> mean to insmod a new module with it.

I am fine with this compromise. It seems to be the only way to keep the
livepatch code somehow sane.

Best Regards,
Petr

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Jason Baron]

On 10/17/2017 09:50 AM, Miroslav Benes wrote:
> On Tue, 17 Oct 2017, Miroslav Benes wrote:
> 
>> On Tue, 10 Oct 2017, Jason Baron wrote:
>>
>>>
>>>
>>> On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
>>>> On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
>>>>> 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
>>>>
>>>> I don't really like allowing a previously replaced patch to replace the
>>>> current patch.  It's just more unnecessary complexity.  If the user
>>>> wants to atomically revert back to kpatch-a, they should be able to:
>>>>
>>>>   rmmod kpatch-a
>>>>   insmod kpatch-a.ko
>>>>
>>
>> I agree.
>>  
>>> Right - that's how I sent v1 (using rmmod/insmod to revert), but it
>>> didn't account for the fact the patch or some functions may be marked
>>> 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
>>> some cases 'rmmod' was not feasible, I thought it would be simpler from
>>> an operational pov to just say we always revert by re-enabling a
>>> previously replaced patch as opposed to rmmod/insmod.
>>>
>>>
>>>>> 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.
>>>>
>>>> If you don't allow a previously replaced patch to be enabled again, I
>>>> think it would be trivial to let it be unloaded.
>>>>
>>>
>>> The concern is around being replaced by 'immediate' functions and thus
>>> not knowing if the code is still in use.
>>
>> Hm. Would it make sense to remove immediate and rely only on the 
>> consistency model? At least for the architectures where the model is 
>> implemented (x86_64)?
>>
>> If not, then I'd keep such modules there without a possibility to remove 
>> them ever. If its functionality was required again, it would of course 
>> mean to insmod a new module with it.
> 
> Petr pointed out (offline) that immediate could still be useful. So let me 
> describe how I envision the whole atomic replace semantics.
> 
> Let's have three functions - a, b, c. Patch 1 is immediate and patches 
> a().
> 
> func		a	b	c
> patches		1i
> 
> Patch 2 is not immediate and patches b()
> 
> func		a	b	c
> patches		1i
> 			2
> 
> Patch 3 is atomic replace, which patches a() and c().
> 
> func		a	b	c
> patches		1i
> 			2
> 		3r		3r
> 
> With 3 applied, 3versions of a() and c() are called, original b() is
> called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2
> cannot be reenabled.

Yes, if we change back to allowing the 'atomic replace' patch to drop
the module reference on previous modules when possible, then yes 2 can
be rmmoded. I originally started off with that model of being able to do
'revert' via rmmod and insmod of previous modules, but then we moved to
a re-enable model in v4 based on the fact that in some cases we can not
do the rmmod and insmod thing. For example if patch 3 was an immediate
patch, then we would replace function b 'immediately' and thus not know
when it was safe to rmmod patch 2. And in fact, in your example i think
its safe to rmmod patch 1 as well, since in the transition to patch 3 we
would have checked the func stack for the immediately preceding function
for function a which would have been 1i, and thus since we know its not
running we can rmmod patch 1 as well.

I'm happy to go back to rmmod/insmod for revert which seems to be the
consensus here - it certainly helps clean up and avoid large stacks of
modules (in most cases), which is nice.

> 
> 3 can be disabled. Original a() and c() will be called in such case. If
> one wants to have a() from patch 1 there, he would need to apply patch
> 4.
> 
> func		a	b	c
> patches		1i
> 			2
> 		3r		3r
> 		4i
> 
> Does it make sense? This is what I would expect and I think it is easier
> to implement. Whole initialization of nop functions could be done in
> klp_init_ functions, if I am not mistaken.
>> Thoughts?
>

yes, that makes sense to me. It might be worth clarifying the logic
around when we can drop references on previous modules when an atomic
revert patch is applied -

If the atomic replace patch has any immediate functions or is marked as
immediate patch, then we can not drop the reference on the previous
module, since it may still be in use. If the atomic replace patch does
not contain any immediates, then we can drop the reference on the
immediately preceding patch only. That is because there may have been
previous transitions to immediate functions in the func stack, and the
transition to the atomic replace patch only checks immediately preceding
transition. It would be possible to check all of the previous immediate
function transitions, but this adds complexity and seems like not a
common pattern. So I would suggest that we just drop the reference on
the previous patch if the atomic replace patch does not contain any
immediate functions.

Thanks,

-Jason


> Miroslav
> 

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

On Tue, 17 Oct 2017, Jason Baron wrote:

> 
> 
> On 10/17/2017 09:50 AM, Miroslav Benes wrote:
> > On Tue, 17 Oct 2017, Miroslav Benes wrote:
> > 
> >> On Tue, 10 Oct 2017, Jason Baron wrote:
> >>
> >>>
> >>>
> >>> On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> >>>> On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
> >>>>> 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
> >>>>
> >>>> I don't really like allowing a previously replaced patch to replace the
> >>>> current patch.  It's just more unnecessary complexity.  If the user
> >>>> wants to atomically revert back to kpatch-a, they should be able to:
> >>>>
> >>>>   rmmod kpatch-a
> >>>>   insmod kpatch-a.ko
> >>>>
> >>
> >> I agree.
> >>  
> >>> Right - that's how I sent v1 (using rmmod/insmod to revert), but it
> >>> didn't account for the fact the patch or some functions may be marked
> >>> 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
> >>> some cases 'rmmod' was not feasible, I thought it would be simpler from
> >>> an operational pov to just say we always revert by re-enabling a
> >>> previously replaced patch as opposed to rmmod/insmod.
> >>>
> >>>
> >>>>> 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.
> >>>>
> >>>> If you don't allow a previously replaced patch to be enabled again, I
> >>>> think it would be trivial to let it be unloaded.
> >>>>
> >>>
> >>> The concern is around being replaced by 'immediate' functions and thus
> >>> not knowing if the code is still in use.
> >>
> >> Hm. Would it make sense to remove immediate and rely only on the 
> >> consistency model? At least for the architectures where the model is 
> >> implemented (x86_64)?
> >>
> >> If not, then I'd keep such modules there without a possibility to remove 
> >> them ever. If its functionality was required again, it would of course 
> >> mean to insmod a new module with it.
> > 
> > Petr pointed out (offline) that immediate could still be useful. So let me 
> > describe how I envision the whole atomic replace semantics.
> > 
> > Let's have three functions - a, b, c. Patch 1 is immediate and patches 
> > a().
> > 
> > func		a	b	c
> > patches		1i
> > 
> > Patch 2 is not immediate and patches b()
> > 
> > func		a	b	c
> > patches		1i
> > 			2
> > 
> > Patch 3 is atomic replace, which patches a() and c().
> > 
> > func		a	b	c
> > patches		1i
> > 			2
> > 		3r		3r
> > 
> > With 3 applied, 3versions of a() and c() are called, original b() is
> > called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2
> > cannot be reenabled.
> 
> Yes, if we change back to allowing the 'atomic replace' patch to drop
> the module reference on previous modules when possible, then yes 2 can
> be rmmoded. I originally started off with that model of being able to do
> 'revert' via rmmod and insmod of previous modules, but then we moved to
> a re-enable model in v4 based on the fact that in some cases we can not
> do the rmmod and insmod thing. For example if patch 3 was an immediate
> patch, then we would replace function b 'immediately' and thus not know
> when it was safe to rmmod patch 2.

Correct.

> And in fact, in your example i think
> its safe to rmmod patch 1 as well, since in the transition to patch 3 we
> would have checked the func stack for the immediately preceding function
> for function a which would have been 1i, and thus since we know its not
> running we can rmmod patch 1 as well.

Correct as well.

> I'm happy to go back to rmmod/insmod for revert which seems to be the
> consensus here - it certainly helps clean up and avoid large stacks of
> modules (in most cases), which is nice.
> 
> > 
> > 3 can be disabled. Original a() and c() will be called in such case. If
> > one wants to have a() from patch 1 there, he would need to apply patch
> > 4.
> > 
> > func		a	b	c
> > patches		1i
> > 			2
> > 		3r		3r
> > 		4i
> > 
> > Does it make sense? This is what I would expect and I think it is easier
> > to implement. Whole initialization of nop functions could be done in
> > klp_init_ functions, if I am not mistaken.
> >> Thoughts?
> >
> 
> yes, that makes sense to me. It might be worth clarifying the logic
> around when we can drop references on previous modules when an atomic
> revert patch is applied -
> 
> If the atomic replace patch has any immediate functions or is marked as
> immediate patch, then we can not drop the reference on the previous
> module, since it may still be in use. 

Yes, then we know nothing about tasks in the system.

> If the atomic replace patch does
> not contain any immediates, then we can drop the reference on the
> immediately preceding patch only. That is because there may have been
> previous transitions to immediate functions in the func stack, and the
> transition to the atomic replace patch only checks immediately preceding
> transition. It would be possible to check all of the previous immediate
> function transitions, but this adds complexity and seems like not a
> common pattern. So I would suggest that we just drop the reference on
> the previous patch if the atomic replace patch does not contain any
> immediate functions.

It is even more complicated and it is not connected only to atomic replace 
patch (I realized this while reading the first part of your email and 
then you confirmed it with this paragraph). The consistency model is 
broken with respect to immediate patches.

func		a
patches		1i
		2i
		3

Now, when you're applying 3, only 2i function is checked. But there might 
be a task sleeping in 1i. Such task would be migrated to 3, because we do 
not check 1 in klp_check_stack_func() at all.

I see three solutions.

1. Say it is an user's fault. Since it is not obvious and it is 
easy-to-make mistake, I would not go this way.

2. We can fix klp_check_stack_func() in an exact way you're proposing. 
We'd go back in func stack as long as there are immediate patches there. 
This adds complexity and I'm not sure if all the problems would be solved 
because scenarios how patches are stacked and applied to different 
functions may be quite complex.

3. Drop immediate. It causes problems only and its advantages on x86_64 
are theoretical. You would still need to solve the interaction with atomic 
replace on other architecture with immediate preserved, but that may be 
easier. Or we can be aggressive and drop immediate completely. The force 
transition I proposed earlier could achieve the same.

Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Josh Poimboeuf]

On Wed, Oct 18, 2017 at 11:10:09AM +0200, Miroslav Benes wrote:
> 3. Drop immediate. It causes problems only and its advantages on x86_64 
> are theoretical. You would still need to solve the interaction with atomic 
> replace on other architecture with immediate preserved, but that may be 
> easier. Or we can be aggressive and drop immediate completely. The force 
> transition I proposed earlier could achieve the same.

I like this idea.  When can we expect v3 of the force patches? :-)

-- 
Josh

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Petr Mladek]

On Wed 2017-10-18 11:10:09, Miroslav Benes wrote:
> On Tue, 17 Oct 2017, Jason Baron wrote:
> > If the atomic replace patch does
> > not contain any immediates, then we can drop the reference on the
> > immediately preceding patch only. That is because there may have been
> > previous transitions to immediate functions in the func stack, and the
> > transition to the atomic replace patch only checks immediately preceding
> > transition. It would be possible to check all of the previous immediate
> > function transitions, but this adds complexity and seems like not a
> > common pattern. So I would suggest that we just drop the reference on
> > the previous patch if the atomic replace patch does not contain any
> > immediate functions.
> 
> It is even more complicated and it is not connected only to atomic replace 
> patch (I realized this while reading the first part of your email and 
> then you confirmed it with this paragraph). The consistency model is 
> broken with respect to immediate patches.
> 
> func		a
> patches		1i
> 		2i
> 		3
> 
> Now, when you're applying 3, only 2i function is checked. But there might 
> be a task sleeping in 1i. Such task would be migrated to 3, because we do 
> not check 1 in klp_check_stack_func() at all.
> 
> I see three solutions.
> 
> 1. Say it is an user's fault. Since it is not obvious and it is 
> easy-to-make mistake, I would not go this way.
> 
> 2. We can fix klp_check_stack_func() in an exact way you're proposing. 
> We'd go back in func stack as long as there are immediate patches there. 
> This adds complexity and I'm not sure if all the problems would be solved 
> because scenarios how patches are stacked and applied to different 
> functions may be quite complex.
> 
> 3. Drop immediate. It causes problems only and its advantages on x86_64 
> are theoretical. You would still need to solve the interaction with atomic 
> replace on other architecture with immediate preserved, but that may be 
> easier. Or we can be aggressive and drop immediate completely. The force 
> transition I proposed earlier could achieve the same.

To make it clear. We currently rely on the immediate handling on
architectures without a reliable stack checking. The question
is if anyone uses it for another purpose in practice.

A solution would be to remove the per-func immediate flag
and invert the logic of the per-patch one. We could rename
it to something like "consistency_required" or "semantic_changes".
A patch with this flag set then might be refused on systems
without reliable stacks. Otherwise, the consistency model
would be used for all patches.

As a result, all patches would be handled either using
the consistency model or immediately. We would need to
care about any mix of these.

Best Regards,
Petr

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

On Wed, 18 Oct 2017, Josh Poimboeuf wrote:

> On Wed, Oct 18, 2017 at 11:10:09AM +0200, Miroslav Benes wrote:
> > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > are theoretical. You would still need to solve the interaction with atomic 
> > replace on other architecture with immediate preserved, but that may be 
> > easier. Or we can be aggressive and drop immediate completely. The force 
> > transition I proposed earlier could achieve the same.
> 
> I like this idea.  When can we expect v3 of the force patches? :-)

Hopefully soon. First I wanted to eliminate the amount of 
unreaded/unreviewed patches. Almost there. However there is OSS 
next week :/. But soon... :)

Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Jiri Kosina]

On Wed, 18 Oct 2017, Miroslav Benes wrote:

> 3. Drop immediate. It causes problems only and its advantages on x86_64 
> are theoretical. You would still need to solve the interaction with atomic 
> replace on other architecture with immediate preserved, but that may be 
> easier. Or we can be aggressive and drop immediate completely. The force 
> transition I proposed earlier could achieve the same.

After brief off-thread discussion, I've been thinking about this a bit 
more and I also think that we should claim immediate "an experiment that 
failed", especially as the force functionality (which provides equal 
functionality from the userspace POV) will likely be there sonnish.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Josh Poimboeuf]

On Wed, Oct 18, 2017 at 03:36:42PM +0200, Jiri Kosina wrote:
> On Wed, 18 Oct 2017, Miroslav Benes wrote:
> 
> > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > are theoretical. You would still need to solve the interaction with atomic 
> > replace on other architecture with immediate preserved, but that may be 
> > easier. Or we can be aggressive and drop immediate completely. The force 
> > transition I proposed earlier could achieve the same.
> 
> After brief off-thread discussion, I've been thinking about this a bit 
> more and I also think that we should claim immediate "an experiment that 
> failed", especially as the force functionality (which provides equal 
> functionality from the userspace POV) will likely be there sonnish.

Agreed.

To clarify, we'll need the force patch before removing
klp_patch.immediate, so we don't break non-x86 arches in the meantime.

On the other hand I think we could remove klp_func.immediate
immediately.


While we're on the subject of removing code... :-)


I've mentioned this several times before, but the more features we add,
the more obvious this point becomes: if we could figure out how to get
rid of the "patching unloaded modules" feature, the code would be so
much better, and I'd actually be able to fit the code in my brain.  Then
we could get rid of all these sneaky bugs that Miroslav and Petr keep
finding, and I wouldn't get an uneasy feeling everytime somebody posts a
new feature.

Here's one vague idea for how to achieve that.  More ideas welcome.

1) Make the consistency model synchronous with respect to modules: don't
   allow any modules to load or unload until the patch transition is
   complete.

2) Instead of one big uber patch module which patches vmlinux and
   modules at the same time, make each patch module specific to a single
   object.  Then bundle the patch modules together somehow into a "patch
   bundle" so they're treated as a single atomic unit.

3) The patch bundle, when loaded, would load some of its patch modules
   immediately (for those objects which are already loaded).  For
   unloaded objects, the corresponding patch modules will be copied into
   memory but not loaded.

4) Then, when a to-be-patched module is loaded, the module loader loads
   it into memory and relocates it, but doesn't make it live.  Then it
   loads the patch module from the memory blob, makes the patch module
   live, and then makes the to-be-patched module live.

(A variation would be to create a way for user space to load a module in
the paused state.  Then user space can handle the dependencies and do
the patch juggling.  I think that would mean depmod/modprobe would need
to be involved.)

It could be a terrible idea, though it might be worth looking into.

-- 
Josh

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

On Wed, 18 Oct 2017, Josh Poimboeuf wrote:

> On Wed, Oct 18, 2017 at 03:36:42PM +0200, Jiri Kosina wrote:
> > On Wed, 18 Oct 2017, Miroslav Benes wrote:
> > 
> > > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > > are theoretical. You would still need to solve the interaction with atomic 
> > > replace on other architecture with immediate preserved, but that may be 
> > > easier. Or we can be aggressive and drop immediate completely. The force 
> > > transition I proposed earlier could achieve the same.
> > 
> > After brief off-thread discussion, I've been thinking about this a bit 
> > more and I also think that we should claim immediate "an experiment that 
> > failed", especially as the force functionality (which provides equal 
> > functionality from the userspace POV) will likely be there sonnish.
> 
> Agreed.
> 
> To clarify, we'll need the force patch before removing
> klp_patch.immediate, so we don't break non-x86 arches in the meantime.

Yes, that is correct. I've just started to work on the force patch.
 
> On the other hand I think we could remove klp_func.immediate
> immediately.
> 
> 
> While we're on the subject of removing code... :-)
> 
> 
> I've mentioned this several times before, but the more features we add,
> the more obvious this point becomes: if we could figure out how to get
> rid of the "patching unloaded modules" feature, the code would be so
> much better, and I'd actually be able to fit the code in my brain.  Then
> we could get rid of all these sneaky bugs that Miroslav and Petr keep
> finding, and I wouldn't get an uneasy feeling everytime somebody posts a
> new feature.
> 
> Here's one vague idea for how to achieve that.  More ideas welcome.
> 
> 1) Make the consistency model synchronous with respect to modules: don't
>    allow any modules to load or unload until the patch transition is
>    complete.
> 
> 2) Instead of one big uber patch module which patches vmlinux and
>    modules at the same time, make each patch module specific to a single
>    object.  Then bundle the patch modules together somehow into a "patch
>    bundle" so they're treated as a single atomic unit.
> 
> 3) The patch bundle, when loaded, would load some of its patch modules
>    immediately (for those objects which are already loaded).  For
>    unloaded objects, the corresponding patch modules will be copied into
>    memory but not loaded.
> 
> 4) Then, when a to-be-patched module is loaded, the module loader loads
>    it into memory and relocates it, but doesn't make it live.  Then it
>    loads the patch module from the memory blob, makes the patch module
>    live, and then makes the to-be-patched module live.
> 
> (A variation would be to create a way for user space to load a module in
> the paused state.  Then user space can handle the dependencies and do
> the patch juggling.  I think that would mean depmod/modprobe would need
> to be involved.)

It might be easier to do at least part of it in userspace.
 
> It could be a terrible idea, though it might be worth looking into.

I'm worried that we would only move the complexity elsewhere, but maybe 
not. We'd remove the code from kernel/livepatch/, but some non-trivial 
changes would go to the module loader.

Jessica, would that be possible?

We need to discuss it with packaging if that'd be possible to deliver it.

Anyway, interesting idea.

Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Josh Poimboeuf]

On Thu, Oct 19, 2017 at 10:30:24AM +0200, Miroslav Benes wrote:
> On Wed, 18 Oct 2017, Josh Poimboeuf wrote:
> 
> > On Wed, Oct 18, 2017 at 03:36:42PM +0200, Jiri Kosina wrote:
> > > On Wed, 18 Oct 2017, Miroslav Benes wrote:
> > > 
> > > > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > > > are theoretical. You would still need to solve the interaction with atomic 
> > > > replace on other architecture with immediate preserved, but that may be 
> > > > easier. Or we can be aggressive and drop immediate completely. The force 
> > > > transition I proposed earlier could achieve the same.
> > > 
> > > After brief off-thread discussion, I've been thinking about this a bit 
> > > more and I also think that we should claim immediate "an experiment that 
> > > failed", especially as the force functionality (which provides equal 
> > > functionality from the userspace POV) will likely be there sonnish.
> > 
> > Agreed.
> > 
> > To clarify, we'll need the force patch before removing
> > klp_patch.immediate, so we don't break non-x86 arches in the meantime.
> 
> Yes, that is correct. I've just started to work on the force patch.

Good!

> > On the other hand I think we could remove klp_func.immediate
> > immediately.
> > 
> > 
> > While we're on the subject of removing code... :-)
> > 
> > 
> > I've mentioned this several times before, but the more features we add,
> > the more obvious this point becomes: if we could figure out how to get
> > rid of the "patching unloaded modules" feature, the code would be so
> > much better, and I'd actually be able to fit the code in my brain.  Then
> > we could get rid of all these sneaky bugs that Miroslav and Petr keep
> > finding, and I wouldn't get an uneasy feeling everytime somebody posts a
> > new feature.
> > 
> > Here's one vague idea for how to achieve that.  More ideas welcome.
> > 
> > 1) Make the consistency model synchronous with respect to modules: don't
> >    allow any modules to load or unload until the patch transition is
> >    complete.
> > 
> > 2) Instead of one big uber patch module which patches vmlinux and
> >    modules at the same time, make each patch module specific to a single
> >    object.  Then bundle the patch modules together somehow into a "patch
> >    bundle" so they're treated as a single atomic unit.
> > 
> > 3) The patch bundle, when loaded, would load some of its patch modules
> >    immediately (for those objects which are already loaded).  For
> >    unloaded objects, the corresponding patch modules will be copied into
> >    memory but not loaded.
> > 
> > 4) Then, when a to-be-patched module is loaded, the module loader loads
> >    it into memory and relocates it, but doesn't make it live.  Then it
> >    loads the patch module from the memory blob, makes the patch module
> >    live, and then makes the to-be-patched module live.
> > 
> > (A variation would be to create a way for user space to load a module in
> > the paused state.  Then user space can handle the dependencies and do
> > the patch juggling.  I think that would mean depmod/modprobe would need
> > to be involved.)
> 
> It might be easier to do at least part of it in userspace.
>  
> > It could be a terrible idea, though it might be worth looking into.
> 
> I'm worried that we would only move the complexity elsewhere, but maybe 
> not. We'd remove the code from kernel/livepatch/, but some non-trivial 
> changes would go to the module loader.

Yes, that's a very real possibility.  In order for it to be an
improvement, it would have to be self-contained.  We don't want to make
the module code worse either.

> Jessica, would that be possible?
> 
> We need to discuss it with packaging if that'd be possible to deliver it.
> 
> Anyway, interesting idea.

Yeah, just an idea to consider...  I'd like to play around with it
someday.

-- 
Josh

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Jason Baron]

On 10/18/2017 07:25 AM, Petr Mladek wrote:
> On Wed 2017-10-18 11:10:09, Miroslav Benes wrote:
>> On Tue, 17 Oct 2017, Jason Baron wrote:
>>> If the atomic replace patch does
>>> not contain any immediates, then we can drop the reference on the
>>> immediately preceding patch only. That is because there may have been
>>> previous transitions to immediate functions in the func stack, and the
>>> transition to the atomic replace patch only checks immediately preceding
>>> transition. It would be possible to check all of the previous immediate
>>> function transitions, but this adds complexity and seems like not a
>>> common pattern. So I would suggest that we just drop the reference on
>>> the previous patch if the atomic replace patch does not contain any
>>> immediate functions.
>>
>> It is even more complicated and it is not connected only to atomic replace 
>> patch (I realized this while reading the first part of your email and 
>> then you confirmed it with this paragraph). The consistency model is 
>> broken with respect to immediate patches.
>>
>> func		a
>> patches		1i
>> 		2i
>> 		3
>>
>> Now, when you're applying 3, only 2i function is checked. But there might 
>> be a task sleeping in 1i. Such task would be migrated to 3, because we do 
>> not check 1 in klp_check_stack_func() at all.
>>
>> I see three solutions.
>>
>> 1. Say it is an user's fault. Since it is not obvious and it is 
>> easy-to-make mistake, I would not go this way.
>>
>> 2. We can fix klp_check_stack_func() in an exact way you're proposing. 
>> We'd go back in func stack as long as there are immediate patches there. 
>> This adds complexity and I'm not sure if all the problems would be solved 
>> because scenarios how patches are stacked and applied to different 
>> functions may be quite complex.
>>
>> 3. Drop immediate. It causes problems only and its advantages on x86_64 
>> are theoretical. You would still need to solve the interaction with atomic 
>> replace on other architecture with immediate preserved, but that may be 
>> easier. Or we can be aggressive and drop immediate completely. The force 
>> transition I proposed earlier could achieve the same.
> 
> To make it clear. We currently rely on the immediate handling on
> architectures without a reliable stack checking. The question
> is if anyone uses it for another purpose in practice.
> 
> A solution would be to remove the per-func immediate flag
> and invert the logic of the per-patch one. We could rename
> it to something like "consistency_required" or "semantic_changes".
> A patch with this flag set then might be refused on systems
> without reliable stacks. Otherwise, the consistency model
> would be used for all patches.
> 
> As a result, all patches would be handled either using
> the consistency model or immediately. We would need to
> care about any mix of these.
> 
> Best Regards,
> Petr
> 

Hi,

So for atomic replace, it seems as if we don't want to allow replaced
patches to be re-enabled but instead, allow them to rmmod/insmod'ed to
allow revert.

In your above proposed model around immediate, if all patches are
immediate then the atomic replace doesn't allow any of the previous
patches to be removed from the kernel via rmmod, while if all patches
are handled using the consistency model then all patches previous to the
atomic replace patch could be removed via rmmod. So this would simplify
the logic around when replaced patches could removed.

I was thinking in the current model to only allow the one preceding
patch to the atomic replace patch to be rmmod'able, if the atomic
replace patch was using the consistency model (to avoid complications
with immediate functions/patches). And this could be extended to all
*all* patches previous to the atomic replace patch to be rmmod'able when
we move to the proposed model around immediates. So I don't think this
should hold up the atomic replace patches?

Thanks,

-Jason

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Jason Baron]

On 10/18/2017 05:10 AM, Miroslav Benes wrote:
> On Tue, 17 Oct 2017, Jason Baron wrote:
> 
>>
>>
>> On 10/17/2017 09:50 AM, Miroslav Benes wrote:
>>> On Tue, 17 Oct 2017, Miroslav Benes wrote:
>>>
>>>> On Tue, 10 Oct 2017, Jason Baron wrote:
>>>>
>>>>>
>>>>>
>>>>> On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
>>>>>> On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
>>>>>>> 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
>>>>>>
>>>>>> I don't really like allowing a previously replaced patch to replace the
>>>>>> current patch.  It's just more unnecessary complexity.  If the user
>>>>>> wants to atomically revert back to kpatch-a, they should be able to:
>>>>>>
>>>>>>   rmmod kpatch-a
>>>>>>   insmod kpatch-a.ko
>>>>>>
>>>>
>>>> I agree.
>>>>  
>>>>> Right - that's how I sent v1 (using rmmod/insmod to revert), but it
>>>>> didn't account for the fact the patch or some functions may be marked
>>>>> 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
>>>>> some cases 'rmmod' was not feasible, I thought it would be simpler from
>>>>> an operational pov to just say we always revert by re-enabling a
>>>>> previously replaced patch as opposed to rmmod/insmod.
>>>>>
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> If you don't allow a previously replaced patch to be enabled again, I
>>>>>> think it would be trivial to let it be unloaded.
>>>>>>
>>>>>
>>>>> The concern is around being replaced by 'immediate' functions and thus
>>>>> not knowing if the code is still in use.
>>>>
>>>> Hm. Would it make sense to remove immediate and rely only on the 
>>>> consistency model? At least for the architectures where the model is 
>>>> implemented (x86_64)?
>>>>
>>>> If not, then I'd keep such modules there without a possibility to remove 
>>>> them ever. If its functionality was required again, it would of course 
>>>> mean to insmod a new module with it.
>>>
>>> Petr pointed out (offline) that immediate could still be useful. So let me 
>>> describe how I envision the whole atomic replace semantics.
>>>
>>> Let's have three functions - a, b, c. Patch 1 is immediate and patches 
>>> a().
>>>
>>> func		a	b	c
>>> patches		1i
>>>
>>> Patch 2 is not immediate and patches b()
>>>
>>> func		a	b	c
>>> patches		1i
>>> 			2
>>>
>>> Patch 3 is atomic replace, which patches a() and c().
>>>
>>> func		a	b	c
>>> patches		1i
>>> 			2
>>> 		3r		3r
>>>
>>> With 3 applied, 3versions of a() and c() are called, original b() is
>>> called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2
>>> cannot be reenabled.
>>
>> Yes, if we change back to allowing the 'atomic replace' patch to drop
>> the module reference on previous modules when possible, then yes 2 can
>> be rmmoded. I originally started off with that model of being able to do
>> 'revert' via rmmod and insmod of previous modules, but then we moved to
>> a re-enable model in v4 based on the fact that in some cases we can not
>> do the rmmod and insmod thing. For example if patch 3 was an immediate
>> patch, then we would replace function b 'immediately' and thus not know
>> when it was safe to rmmod patch 2.
> 
> Correct.
> 
>> And in fact, in your example i think
>> its safe to rmmod patch 1 as well, since in the transition to patch 3 we
>> would have checked the func stack for the immediately preceding function
>> for function a which would have been 1i, and thus since we know its not
>> running we can rmmod patch 1 as well.
> 
> Correct as well.
> 
>> I'm happy to go back to rmmod/insmod for revert which seems to be the
>> consensus here - it certainly helps clean up and avoid large stacks of
>> modules (in most cases), which is nice.
>>
>>>
>>> 3 can be disabled. Original a() and c() will be called in such case. If
>>> one wants to have a() from patch 1 there, he would need to apply patch
>>> 4.
>>>
>>> func		a	b	c
>>> patches		1i
>>> 			2
>>> 		3r		3r
>>> 		4i
>>>
>>> Does it make sense? This is what I would expect and I think it is easier
>>> to implement. Whole initialization of nop functions could be done in
>>> klp_init_ functions, if I am not mistaken.
>>>> Thoughts?
>>>
>>
>> yes, that makes sense to me. It might be worth clarifying the logic
>> around when we can drop references on previous modules when an atomic
>> revert patch is applied -
>>
>> If the atomic replace patch has any immediate functions or is marked as
>> immediate patch, then we can not drop the reference on the previous
>> module, since it may still be in use. 
> 
> Yes, then we know nothing about tasks in the system.
> 
>> If the atomic replace patch does
>> not contain any immediates, then we can drop the reference on the
>> immediately preceding patch only. That is because there may have been
>> previous transitions to immediate functions in the func stack, and the
>> transition to the atomic replace patch only checks immediately preceding
>> transition. It would be possible to check all of the previous immediate
>> function transitions, but this adds complexity and seems like not a
>> common pattern. So I would suggest that we just drop the reference on
>> the previous patch if the atomic replace patch does not contain any
>> immediate functions.
> 
> It is even more complicated and it is not connected only to atomic replace 
> patch (I realized this while reading the first part of your email and 
> then you confirmed it with this paragraph). The consistency model is 
> broken with respect to immediate patches.

Indeed. I came to the same conclusion.

> 
> func		a
> patches		1i
> 		2i
> 		3
> 
> Now, when you're applying 3, only 2i function is checked. But there might 
> be a task sleeping in 1i. Such task would be migrated to 3, because we do 
> not check 1 in klp_check_stack_func() at all.
> 
> I see three solutions.
> 
> 1. Say it is an user's fault. Since it is not obvious and it is 
> easy-to-make mistake, I would not go this way.
> 
> 2. We can fix klp_check_stack_func() in an exact way you're proposing. 
> We'd go back in func stack as long as there are immediate patches there. 
> This adds complexity and I'm not sure if all the problems would be solved 
> because scenarios how patches are stacked and applied to different 
> functions may be quite complex.
>

I think if its fixed in the current code (before atomic replace is
introduced), then it would allow the atomic replace to drop all
references to previous modules, since we would know they are no longer
in use.

Thanks,

-Jason


> 3. Drop immediate. It causes problems only and its advantages on x86_64 
> are theoretical. You would still need to solve the interaction with atomic 
> replace on other architecture with immediate preserved, but that may be 
> easier. Or we can be aggressive and drop immediate completely. The force 
> transition I proposed earlier could achieve the same.
> 
> Miroslav
> 

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Petr Mladek]

On Thu 2017-10-19 17:44:32, Jason Baron wrote:
> So for atomic replace, it seems as if we don't want to allow replaced
> patches to be re-enabled but instead, allow them to rmmod/insmod'ed to
> allow revert.
>
> In your above proposed model around immediate, if all patches are
> immediate then the atomic replace doesn't allow any of the previous
> patches to be removed from the kernel via rmmod, while if all patches
> are handled using the consistency model then all patches previous to the
> atomic replace patch could be removed via rmmod. So this would simplify
> the logic around when replaced patches could removed.

yes

> I was thinking in the current model to only allow the one preceding
> patch to the atomic replace patch to be rmmod'able, if the atomic
> replace patch was using the consistency model (to avoid complications
> with immediate functions/patches). And this could be extended to all
> *all* patches previous to the atomic replace patch to be rmmod'able when
> we move to the proposed model around immediates. So I don't think this
> should hold up the atomic replace patches?

Yes, this sounds reasonable from the atomic replace patch point of view.

Well, the consistency model is broken at the moment when immediate
flag is used. We should fix this ASAP. IMHO, the fix might need to go
upstream before the atomic replace feature. It might be even stable
material.

Best Regards,
Petr

PS: One problem is that there are some conferences (Kernel Summit,
Open Source Summit) next week. At least all livepatch-related SUSE
people will be less reachable.

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

On Wed, 18 Oct 2017, Petr Mladek wrote:

> On Wed 2017-10-18 11:10:09, Miroslav Benes wrote:
> > On Tue, 17 Oct 2017, Jason Baron wrote:
> > > If the atomic replace patch does
> > > not contain any immediates, then we can drop the reference on the
> > > immediately preceding patch only. That is because there may have been
> > > previous transitions to immediate functions in the func stack, and the
> > > transition to the atomic replace patch only checks immediately preceding
> > > transition. It would be possible to check all of the previous immediate
> > > function transitions, but this adds complexity and seems like not a
> > > common pattern. So I would suggest that we just drop the reference on
> > > the previous patch if the atomic replace patch does not contain any
> > > immediate functions.
> > 
> > It is even more complicated and it is not connected only to atomic replace 
> > patch (I realized this while reading the first part of your email and 
> > then you confirmed it with this paragraph). The consistency model is 
> > broken with respect to immediate patches.
> > 
> > func		a
> > patches		1i
> > 		2i
> > 		3
> > 
> > Now, when you're applying 3, only 2i function is checked. But there might 
> > be a task sleeping in 1i. Such task would be migrated to 3, because we do 
> > not check 1 in klp_check_stack_func() at all.
> > 
> > I see three solutions.
> > 
> > 1. Say it is an user's fault. Since it is not obvious and it is 
> > easy-to-make mistake, I would not go this way.
> > 
> > 2. We can fix klp_check_stack_func() in an exact way you're proposing. 
> > We'd go back in func stack as long as there are immediate patches there. 
> > This adds complexity and I'm not sure if all the problems would be solved 
> > because scenarios how patches are stacked and applied to different 
> > functions may be quite complex.
> > 
> > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > are theoretical. You would still need to solve the interaction with atomic 
> > replace on other architecture with immediate preserved, but that may be 
> > easier. Or we can be aggressive and drop immediate completely. The force 
> > transition I proposed earlier could achieve the same.
> 
> To make it clear. We currently rely on the immediate handling on
> architectures without a reliable stack checking. The question
> is if anyone uses it for another purpose in practice.
> 
> A solution would be to remove the per-func immediate flag
> and invert the logic of the per-patch one. We could rename
> it to something like "consistency_required" or "semantic_changes".
> A patch with this flag set then might be refused on systems
> without reliable stacks. Otherwise, the consistency model
> would be used for all patches.

I have a problem with this. I'd like to see the consistency model as a 
default and not something to ask for. It should be used always unless 
explicitly forbidden.

Just to be sure, we agreed to remove immediate, didn't we?

Miroslav

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

> > It is even more complicated and it is not connected only to atomic replace 
> > patch (I realized this while reading the first part of your email and 
> > then you confirmed it with this paragraph). The consistency model is 
> > broken with respect to immediate patches.
> 
> Indeed. I came to the same conclusion.
> 
> > 
> > func		a
> > patches		1i
> > 		2i
> > 		3
> > 
> > Now, when you're applying 3, only 2i function is checked. But there might 
> > be a task sleeping in 1i. Such task would be migrated to 3, because we do 
> > not check 1 in klp_check_stack_func() at all.
> > 
> > I see three solutions.
> > 
> > 1. Say it is an user's fault. Since it is not obvious and it is 
> > easy-to-make mistake, I would not go this way.
> > 
> > 2. We can fix klp_check_stack_func() in an exact way you're proposing. 
> > We'd go back in func stack as long as there are immediate patches there. 
> > This adds complexity and I'm not sure if all the problems would be solved 
> > because scenarios how patches are stacked and applied to different 
> > functions may be quite complex.
> >
> 
> I think if its fixed in the current code (before atomic replace is
> introduced), then it would allow the atomic replace to drop all
> references to previous modules, since we would know they are no longer
> in use.

That's correct. Although, I'll submit my force patch soon, we can remove 
immediate afterwards and atomic replace should be simpler with that.

Miroslav