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

Groups > linux.kernel > #1692054 > unrolled thread

[PATCH 0/3] livepatch: introduce atomic replace

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] livepatch: introduce atomic replace Jason Baron <jbaron@akamai.com> - 2017-07-19 19:40 +0200
    [PATCH 3/3] livepatch: Add a sysctl livepatch_mode for atomic replace Jason Baron <jbaron@akamai.com> - 2017-07-19 19:40 +0200
    [PATCH 2/3] livepatch: add atomic replace Jason Baron <jbaron@akamai.com> - 2017-07-19 19:50 +0200
    Re: [PATCH 0/3] livepatch: introduce atomic replace Miroslav Benes <mbenes@suse.cz> - 2017-07-21 15:10 +0200
      Re: [PATCH 0/3] livepatch: introduce atomic replace Jason Baron <jbaron@akamai.com> - 2017-07-21 20:00 +0200

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

Hi,

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

Patches:

1) livepatch: Add klp_object and klp_func iterators
	Just a prep patch for the 'atomic revert' feature

2) livepatch: add atomic replace
	Core feature

3) livepatch: Add a sysctl livepatch_mode for atomic replace
	Introduces a knob for enabling atomic replace. I hate knobs and perhaps
	its possible to default to cumulative replace? Although I suspect there
	are workflows relying on the existing behavior - I'm not sure. It may
	be desirable to associate the knob with the patch itself as in the
	'immediate' flag, such that we don't introduce a global sysctl that
	likely would also need to built-in, if there are patches in the initrd.

Thanks,

-Jason

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

 include/linux/livepatch.h     | 118 ++++++++++++++++++++++++++++++--
 kernel/livepatch/core.c       | 154 ++++++++++++++++++++++++++++++++++++++++--
 kernel/livepatch/core.h       |   4 ++
 kernel/livepatch/patch.c      |  23 ++++---
 kernel/livepatch/patch.h      |   1 +
 kernel/livepatch/transition.c |  79 +++++++++++++++++++---
 kernel/sysctl.c               |  12 ++++
 7 files changed, 362 insertions(+), 29 deletions(-)

-- 
2.6.1

[toc] | [next] | [standalone]

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

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

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 6fd7222..08e760a 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -35,6 +35,14 @@
 #define KLP_UNPATCHED	 0
 #define KLP_PATCHED	 1
 
+/* livepatch mode */
+
+extern int sysctl_livepatch_mode;
+enum {
+	LIVEPATCH_MODE_DEFAULT,
+	LIVEPATCH_MODE_REPLACE,
+};
+
 /**
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bf353da..b1df5c4 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,6 +49,8 @@ static LIST_HEAD(klp_patches);
 
 static struct kobject *klp_root_kobj;
 
+int sysctl_livepatch_mode;
+
 static bool klp_is_module(struct klp_object *obj)
 {
 	return obj->name;
@@ -643,6 +645,9 @@ static int klp_init_patch_no_ops(struct klp_patch *patch)
 	if (patch->list.prev == &klp_patches)
 		return 0;
 
+	if (sysctl_livepatch_mode != LIVEPATCH_MODE_REPLACE)
+		return 0;
+
 	prev_patch = list_prev_entry(patch, list);
 	klp_for_each_object(prev_patch, prev_obj, &prev_o_iter) {
 		if (!klp_is_object_loaded(prev_obj))
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4dfba1a..3a0a1f6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -67,6 +67,7 @@
 #include <linux/kexec.h>
 #include <linux/bpf.h>
 #include <linux/mount.h>
+#include <linux/livepatch.h>
 
 #include <linux/uaccess.h>
 #include <asm/processor.h>
@@ -1203,6 +1204,17 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &one,
 	},
 #endif
+#ifdef CONFIG_LIVEPATCH
+	{
+		.procname	= "livepatch_mode",
+		.data		= &sysctl_livepatch_mode,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
+#endif
 	{ }
 };
 
-- 
2.6.1

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

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

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

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

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

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 5038337..6fd7222 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -49,6 +49,7 @@
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
  * @transition:	the func is currently being applied or reverted
+ * @no_op:	this is a no_op function used to compelete revert a function
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:
@@ -86,6 +87,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool no_op;
 };
 
 /**
@@ -132,6 +134,7 @@ struct klp_object {
  * @kobj:	kobject for sysfs resources
  * @obj_list:	head of list for dynamically allocated struct klp_object
  * @enabled:	the patch is enabled (but operation may be incomplete)
+ * @replaced:	the patch has been replaced an can not be re-enabled
  * @finish:	for waiting till it is safe to remove the patch module
  */
 struct klp_patch {
@@ -145,6 +148,7 @@ struct klp_patch {
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool enabled;
+	bool replaced;
 	struct completion finish;
 };
 
@@ -201,8 +205,8 @@ static inline struct klp_func *func_iter_next(struct func_iter *iter)
 	struct klp_func *func;
 	struct klp_func_no_op *func_no_op;
 
-	if (iter->func->old_name || iter->func->new_func ||
-					iter->func->old_sympos) {
+	if (iter->func && (iter->func->old_name || iter->func->new_func ||
+			   iter->func->old_sympos)) {
 		func = iter->func;
 		iter->func++;
 	} else {
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e63f478..bf353da 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -352,6 +352,9 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (klp_transition_patch)
 		return -EBUSY;
 
+	if (patch->replaced)
+		return -EINVAL;
+
 	if (WARN_ON(patch->enabled))
 		return -EINVAL;
 
@@ -602,6 +605,118 @@ static void klp_free_patch(struct klp_patch *patch)
 		list_del(&patch->list);
 }
 
+void klp_patch_free_no_ops(struct klp_patch *patch)
+{
+	struct obj_iter o_iter;
+	struct func_iter f_iter;
+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func;
+	struct klp_func_no_op *func_no_op;
+
+	klp_for_each_object(patch, obj, &o_iter) {
+		klp_for_each_func(obj, func, &f_iter) {
+			if (func->no_op) {
+				func_no_op = container_of(func,
+							  struct klp_func_no_op,
+							  orig_func);
+				list_del_init(&func_no_op->func_entry);
+				kfree(func_no_op);
+			}
+		}
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kfree(obj);
+	}
+}
+
+static int klp_init_patch_no_ops(struct klp_patch *patch)
+{
+	struct klp_object *obj, *prev_obj, *new_obj;
+	struct klp_func *prev_func, *func;
+	struct klp_func_no_op *new;
+	struct klp_patch *prev_patch;
+	struct obj_iter o_iter, prev_o_iter;
+	struct func_iter prev_f_iter, f_iter;
+	bool found, mod;
+
+	if (patch->list.prev == &klp_patches)
+		return 0;
+
+	prev_patch = list_prev_entry(patch, list);
+	klp_for_each_object(prev_patch, prev_obj, &prev_o_iter) {
+		if (!klp_is_object_loaded(prev_obj))
+			continue;
+
+		klp_for_each_func(prev_obj, prev_func, &prev_f_iter) {
+			found = false;
+			klp_for_each_object(patch, obj, &o_iter) {
+				klp_for_each_func(obj, func, &f_iter) {
+					if ((strcmp(prev_func->old_name,
+						    func->old_name) == 0) &&
+						(prev_func->old_sympos ==
+							func->old_sympos)) {
+						found = true;
+						break;
+					}
+				}
+				if (found)
+					break;
+			}
+			if (found)
+				continue;
+
+			new = kmalloc(sizeof(*new), GFP_KERNEL);
+			if (!new)
+				return -ENOMEM;
+			new->orig_func = *prev_func;
+			new->orig_func.old_name = prev_func->old_name;
+			new->orig_func.new_func = NULL;
+			new->orig_func.old_sympos = prev_func->old_sympos;
+			new->orig_func.immediate = prev_func->immediate;
+			new->orig_func.old_addr = prev_func->old_addr;
+			INIT_LIST_HEAD(&new->orig_func.stack_node);
+			new->orig_func.old_size = prev_func->old_size;
+			new->orig_func.new_size = 0;
+			new->orig_func.no_op = true;
+			new->orig_func.patched = false;
+			new->orig_func.transition = false;
+			found = false;
+			mod = klp_is_module(prev_obj);
+			klp_for_each_object(patch, obj, &o_iter) {
+				if (mod) {
+					if (klp_is_module(obj) &&
+					    strcmp(prev_obj->name,
+						   obj->name) == 0) {
+						found = true;
+						break;
+					}
+				} else if (!klp_is_module(obj)) {
+					found = true;
+					break;
+				}
+			}
+			if (found) {
+				list_add(&new->func_entry, &obj->func_list);
+			} else {
+				new_obj = kmalloc(sizeof(*new_obj), GFP_KERNEL);
+				if (!new_obj)
+					return -ENOMEM;
+				new_obj->name = prev_obj->name;
+				new_obj->funcs = NULL;
+				new_obj->mod = prev_obj->mod;
+				new_obj->patched = false;
+				INIT_LIST_HEAD(&new_obj->func_list);
+				INIT_LIST_HEAD(&new_obj->obj_entry);
+				list_add(&new->func_entry, &new_obj->func_list);
+				list_add(&new_obj->obj_entry, &patch->obj_list);
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
 	if (!func->old_name || !func->new_func)
@@ -725,6 +840,7 @@ static int klp_init_patch(struct klp_patch *patch)
 	mutex_lock(&klp_mutex);
 
 	patch->enabled = false;
+	patch->replaced = false;
 	init_completion(&patch->finish);
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
@@ -746,12 +862,19 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	list_add_tail(&patch->list, &klp_patches);
 
+	ret = klp_init_patch_no_ops(patch);
+	if (ret) {
+		list_del(&patch->list);
+		goto free;
+	}
+
 	mutex_unlock(&klp_mutex);
 
 	return 0;
 
 free:
 	klp_free_objects_limited(patch, obj);
+	klp_patch_free_no_ops(patch);
 
 	mutex_unlock(&klp_mutex);
 
@@ -786,6 +909,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 	}
 
 	klp_free_patch(patch);
+	klp_patch_free_no_ops(patch);
 
 	mutex_unlock(&klp_mutex);
 
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index c74f24c..fa20e4d 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -1,6 +1,10 @@
 #ifndef _LIVEPATCH_CORE_H
 #define _LIVEPATCH_CORE_H
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
 
+void klp_patch_free_no_ops(struct klp_patch *patch);
+
 #endif /* _LIVEPATCH_CORE_H */
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 1cfdabc..cbb8b9d 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -117,6 +117,8 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	if (func->no_op)
+		goto unlock;
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	preempt_enable_notrace();
@@ -135,7 +137,7 @@ static unsigned long klp_get_ftrace_location(unsigned long faddr)
 }
 #endif
 
-static void klp_unpatch_func(struct klp_func *func)
+void klp_unpatch_func(struct klp_func *func, bool unregistered)
 {
 	struct klp_ops *ops;
 
@@ -155,9 +157,11 @@ static void klp_unpatch_func(struct klp_func *func)
 		if (WARN_ON(!ftrace_loc))
 			return;
 
-		WARN_ON(unregister_ftrace_function(&ops->fops));
-		WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
-
+		if (!unregistered) {
+			WARN_ON(unregister_ftrace_function(&ops->fops));
+			WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1,
+				0));
+		}
 		list_del_rcu(&func->stack_node);
 		list_del(&ops->node);
 		kfree(ops);
@@ -242,7 +246,7 @@ void klp_unpatch_object(struct klp_object *obj)
 
 	klp_for_each_func(obj, func, &f_iter)
 		if (func->patched)
-			klp_unpatch_func(func);
+			klp_unpatch_func(func, false);
 
 	obj->patched = false;
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index 0db2271..59c1430 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -29,5 +29,6 @@ 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_func(struct klp_func *func, bool unregistered);
 
 #endif /* _LIVEPATCH_PATCH_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index e112826..43e1609 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -21,6 +21,8 @@
 
 #include <linux/cpu.h>
 #include <linux/stacktrace.h>
+#include <linux/ftrace.h>
+#include <linux/delay.h>
 #include "core.h"
 #include "patch.h"
 #include "transition.h"
@@ -70,6 +72,7 @@ static void klp_synchronize_transition(void)
 	schedule_on_each_cpu(klp_sync);
 }
 
+
 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.
@@ -81,8 +84,32 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	bool no_op = false;
 	struct obj_iter o_iter;
 	struct func_iter f_iter;
+	unsigned long ftrace_loc;
+	struct klp_ops *ops;
+	struct klp_patch *prev_patch;
+
+	/* remove ftrace hook for all no_op functions. */
+	if (klp_target_state == KLP_PATCHED) {
+		klp_for_each_object(klp_transition_patch, obj, &o_iter) {
+			klp_for_each_func(obj, func, &f_iter) {
+				if (!func->no_op)
+					continue;
+
+				ops = klp_find_ops(func->old_addr);
+				if (WARN_ON(!ops))
+					continue;
+				ftrace_loc = func->old_addr;
+				WARN_ON(unregister_ftrace_function(&ops->fops));
+				WARN_ON(ftrace_set_filter_ip(&ops->fops,
+							     ftrace_loc,
+							     1, 0));
+				no_op = true;
+			}
+		}
+	}
 
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*
@@ -90,7 +117,9 @@ static void klp_complete_transition(void)
 		 * remove the new functions from the func_stack.
 		 */
 		klp_unpatch_objects(klp_transition_patch);
+	}
 
+	if (klp_target_state == KLP_UNPATCHED || no_op) {
 		/*
 		 * Make sure klp_ftrace_handler() can no longer see functions
 		 * from this patch on the ops->func_stack.  Otherwise, after
@@ -132,6 +161,24 @@ static void klp_complete_transition(void)
 	}
 
 done:
+	/* remove and free any no_op functions */
+	if (no_op && klp_target_state == KLP_PATCHED) {
+		prev_patch = list_prev_entry(klp_transition_patch, list);
+		if (prev_patch->enabled) {
+			klp_unpatch_objects(prev_patch);
+			prev_patch->enabled = false;
+			prev_patch->replaced = true;
+			module_put(prev_patch->mod);
+		}
+		klp_for_each_object(klp_transition_patch, obj, &o_iter) {
+			klp_for_each_func(obj, func, &f_iter) {
+				if (func->no_op)
+					klp_unpatch_func(func, true);
+			}
+		}
+		klp_patch_free_no_ops(klp_transition_patch);
+	}
+
 	klp_target_state = KLP_UNDEFINED;
 	klp_transition_patch = NULL;
 }
@@ -204,10 +251,18 @@ static int klp_check_stack_func(struct klp_func *func,
 		if (klp_target_state == KLP_UNPATCHED) {
 			 /*
 			  * Check for the to-be-unpatched function
-			  * (the func itself).
+			  * (the func itself). If we're unpatching
+			  * a no-op, then we're running the original
+			  * function. We never 'patch' a no-op function,
+			  * since we just remove the ftrace hook.
 			  */
-			func_addr = (unsigned long)func->new_func;
-			func_size = func->new_size;
+			if (func->no_op) {
+				func_addr = (unsigned long)func->old_addr;
+				func_size = func->old_size;
+			} else {
+				func_addr = (unsigned long)func->new_func;
+				func_size = func->new_size;
+			}
 		} else {
 			/*
 			 * Check for the to-be-patched function
-- 
2.6.1

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

#1693679

On Wed, 19 Jul 2017, Jason Baron wrote:

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

Hi Jason,

this has been on my TODO list for a long time now, so thanks for working 
on this. We have the same feature in kGraft and we use it heavily (in fact 
we distribute our patches as cumulative and "replace_all" how we call it).

The forward port of the feature from kGraft is unfortunately not 
straightforward. We do not have a concept of klp_target_state there, so we 
can freely let functions to be patched or reverted in one go. We cannot do 
the same upstream. At first glance, you used nop function exactly for this 
case. Nice hack :).
 
> Patches:
> 
> 1) livepatch: Add klp_object and klp_func iterators
> 	Just a prep patch for the 'atomic revert' feature
> 
> 2) livepatch: add atomic replace
> 	Core feature
> 
> 3) livepatch: Add a sysctl livepatch_mode for atomic replace
> 	Introduces a knob for enabling atomic replace. I hate knobs and perhaps
> 	its possible to default to cumulative replace? Although I suspect there
> 	are workflows relying on the existing behavior - I'm not sure. It may
> 	be desirable to associate the knob with the patch itself as in the
> 	'immediate' flag, such that we don't introduce a global sysctl that
> 	likely would also need to built-in, if there are patches in the initrd.

Yes. I think it should be associated with the patch itself. This would 
allow more flexible behaviour. You could stack more patches on top of 
"atomic replace" patch.

Anyway, I'm on holiday next week, so I'll take a proper look the week 
after.

Thanks,
Miroslav

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

#1693907

On 07/21/2017 09:06 AM, Miroslav Benes wrote:
> On Wed, 19 Jul 2017, Jason Baron wrote:
>
>> Hi,
>>
>> In testing livepatch, I found that when doing cumulative patches, if a patched
>> function is completed reverted by a subsequent patch (back to its original state)
>> livepatch does not revert the funtion to its original state. Specifically, if
>> patch A introduces a change to function 1, and patch B reverts the change to
>> function 1 and introduces changes to say function 2 and 3 as well, the change
>> that patch A introducd to function 1 is still present. This could be addressed
>> by first completely removing patch A (disable and then rmmod) and then inserting
>> patch B (insmod and enable), but this leaves an unpatched window. In discussing
>> this issue with Josh on the kpatch mailing list, he mentioned that we could get
>> 'atomic replace working properly', and that is the direction of this patchset:
>> https://www.redhat.com/archives/kpatch/2017-June/msg00005.html
>
> Hi Jason,
>
> this has been on my TODO list for a long time now, so thanks for working
> on this. We have the same feature in kGraft and we use it heavily (in fact
> we distribute our patches as cumulative and "replace_all" how we call it).
>

Hi Miroslav,

Cool - we feel like this is an important feature as well and would like 
to have an upstream solution as well.

> The forward port of the feature from kGraft is unfortunately not
> straightforward. We do not have a concept of klp_target_state there, so we
> can freely let functions to be patched or reverted in one go. We cannot do
> the same upstream. At first glance, you used nop function exactly for this
> case. Nice hack :).
>
>> Patches:
>>
>> 1) livepatch: Add klp_object and klp_func iterators
>> 	Just a prep patch for the 'atomic revert' feature
>>
>> 2) livepatch: add atomic replace
>> 	Core feature
>>
>> 3) livepatch: Add a sysctl livepatch_mode for atomic replace
>> 	Introduces a knob for enabling atomic replace. I hate knobs and perhaps
>> 	its possible to default to cumulative replace? Although I suspect there
>> 	are workflows relying on the existing behavior - I'm not sure. It may
>> 	be desirable to associate the knob with the patch itself as in the
>> 	'immediate' flag, such that we don't introduce a global sysctl that
>> 	likely would also need to built-in, if there are patches in the initrd.
>
> Yes. I think it should be associated with the patch itself. This would
> allow more flexible behaviour. You could stack more patches on top of
> "atomic replace" patch.
>

Ok - associating the "atomic replace" with the patch itself makes sense 
to me. It would also basically work, I think with the patch I proposed 
except for the case where the the "atomic replace" was on top of several 
non-"atomic replace" patches. The reason is that the "atomic replace" I 
posted looks back 1 patch to see what it needs to replace (assuming all 
patches are in atomic replace mode). So instead of just looking back 1 
patch, it could 'look back' and make sure it was replacing all 
previously loaded patches.


> Anyway, I'm on holiday next week, so I'll take a proper look the week
> after.
>

Ok - have a nice holiday!

Thanks,

-Jason


> Thanks,
> Miroslav
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web