livepatch: introduce atomic replace

In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynmically allocated (needed for atomic replace). Note that this patch is careful, not to grow the size of klp_func as that's the most common data structure. This patch is intended to effectively be a no-op until atomic replace is introduced.

include/linux/livepatch.h +1 -0

index 194991e..5038337 100644
@@ -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)
 

@@ -88,10 +89,23 @@ struct klp_func {
 };
 

include/linux/livepatch.h +16 -1

  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
  * @patched:	the object's funcs have been added to the klp_ops list

@@ -103,6 +117,8 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head func_list;
+	struct list_head obj_entry;
 	struct module *mod;
 	bool patched;
 };

@@ -114,6 +130,7 @@ struct klp_object {
  * @immediate:  patch all funcs immediately, bypassing safety mechanisms
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
+ * @obj_list:	head of list for dynamically allocated struct klp_object
  * @enabled:	the patch is enabled (but operation may be incomplete)
  * @finish:	for waiting till it is safe to remove the patch module
  */

@@ -126,17 +143,96 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
-#define klp_for_each_object(patch, obj) \
+struct obj_iter {
+	struct klp_object *obj;
+	struct list_head *obj_list_head;
+	struct list_head *obj_list_pos;
+};
+
+static inline struct klp_object *obj_iter_next(struct obj_iter *iter)
+{
+	struct klp_object *obj;
+
+	if (iter->obj->funcs || iter->obj->name) {
+		obj = iter->obj;

Quoted 6 lines · Jason Baron

In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynmically allocated (needed for atomic replace). Note that this patch is careful, not to grow the size of klp_func as that's the most common data structure. This patch is intended to effectively be a no-op until atomic replace is introduced.

We need to be careful here. The 3-level hiearachy is already complex enough. This patch adds several asymetric things:

+ the dynamically alocated structures are put into liked lists while the static ones are in arrays

+ the dynamically allocated func lists are linked using an external struct klp_func_no_op while
the dynamically allocated obj lists are linked using additional list_head in struct klp_object

Well, I agree that the combination of the iterators and extra no-op funcs allow to implement the atomic replace relatively easy and transparent way.

Another possibility would be to add some special flags to the affected patches on the stack. But this would require hacks in the ftrace handler, going through all patches in klp_try_complete_transition() and other functions. It looks more complicated.

I just wonder if we could clean this patch a bit.
Please, find some thoughts below.

Quoted 16 lines

include/linux/livepatch.h +1 -0

index 194991e..5038337 100644
@@ -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)
 

@@ -88,10 +89,23 @@ struct klp_func {
 };
 

include/linux/livepatch.h struct klp_func { · line 100

Quoted 10 lines

include/linux/livepatch.h +9 -0

 /**
+ * struct klp_func_no_op - internal object used to link no_op functions, which
+			   avoids the need to bloat struct klp_func
+ * @orig_func:	embeds struct klp_func
+ * @func_entry:	used link struct klp_func_no_op to struct klp_object
+ */
+struct klp_func_no_op {
+	struct klp_func orig_func;
+	struct list_head func_entry;
+};

I am not sure that this is worth it. It saves two pointers in struct klp-func that are mostly unused. But it adds one more asymmetry that would complicate maintaining the code.

struct klp_func is already quite big. Note that struct kobj is hidden there. I think that we could afford one more struct list_head there.

If people are concerned about the size, we could make this feature configurable at build time. People using this feature will benefit from the the ability to remove the replaced patches.

I also thought about using arrays also for the dynamic structures. But we would need to compute the size first. It might require another crazy code, especially when we allow to replace all patches and need to look for duplicates.

include/linux/livepatch.h struct klp_func { · line 108

Quoted 7 lines

include/linux/livepatch.h +3 -0

+/**
  * 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
+ * @func_list:	head of list for struct klp_func_no_op
+ * @obj_entry:	used to link struct klp_object to struct klp_patch

I would prefer to make the difference between the static and dynamic stuff more obvious. The generic names for both variants are confusing.

I would personally use "no_op" in the names of list_heads related to the no_op stuff. But then it would make more sense to add this in the next patch.

Well, I do not have strong opinion about it.

Quoted 43 lines

include/linux/livepatch.h +16 -1

  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
  * @patched:	the object's funcs have been added to the klp_ops list

@@ -103,6 +117,8 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head func_list;
+	struct list_head obj_entry;
 	struct module *mod;
 	bool patched;
 };

@@ -114,6 +130,7 @@ struct klp_object {
  * @immediate:  patch all funcs immediately, bypassing safety mechanisms
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
+ * @obj_list:	head of list for dynamically allocated struct klp_object
  * @enabled:	the patch is enabled (but operation may be incomplete)
  * @finish:	for waiting till it is safe to remove the patch module
  */

@@ -126,17 +143,96 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
-#define klp_for_each_object(patch, obj) \
+struct obj_iter {
+	struct klp_object *obj;
+	struct list_head *obj_list_head;
+	struct list_head *obj_list_pos;
+};
+
+static inline struct klp_object *obj_iter_next(struct obj_iter *iter)
+{
+	struct klp_object *obj;
+
+	if (iter->obj->funcs || iter->obj->name) {
+		obj = iter->obj;

include/linux/livepatch.h struct klp_patch { · line 172

Quoted 10 lines

include/linux/livepatch.h +10 -0

+		iter->obj++;
+	} else {
+		if (iter->obj_list_pos == iter->obj_list_head) {
+			obj = NULL;
+		} else {
+			obj = list_entry(iter->obj_list_pos, struct klp_object,
+					 obj_entry);
+			iter->obj_list_pos = iter->obj_list_pos->next;
+		}
+	}

This code might deserve some comments. Well, I wonder if we could make it working without the struct obj_iter.

If we could somehow distinguish the statically and dynamically allocated struct klp_object then we would know how to find the next one. I mean something like:

static inline struct klp_object *klp_next_object(struct klp_patch *patch,
struct klp_object *obj)

{
	struct klp_object *next_obj = NULL;

	/* Only statically defined objects has funcs array. */
	if (obj->funcs) {
		next_obj = obj + 1;
		if (next_obj)
			goto out;

		/* Is there any dynamically allocated object? */ 
		if (!list_empty(patch->obj_list)) {
			next_obj = container_of(patch->obj_list.next,
					struct klp_object,
					obj_entry);
			goto out;
	}

	/* This must be a dynamically allocated object. Is it the last one? */
	if (obj->obj_entry.next != patch->obj_list)
		next_obj = container_of(obj->obj_entry.next,
					struct klp_object,
					obj_entry);

out:

	return next_obj;
}

Note that this is not even compile tested.

Best Regards,
Petr

Quoted 17 lines · Petr Mladek

Quoted 7 lines · Jason Baron

+/**
* 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
+ * @func_list: head of list for struct klp_func_no_op
+ * @obj_entry: used to link struct klp_object to struct klp_patch

I would prefer to make the difference between the static and dynamic stuff more obvious. The generic names for both variants are confusing.

I would personally use "no_op" in the names of list_heads related to the no_op stuff. But then it would make more sense to add this in the next patch.

Quoted 2 lines

Well, I do not have strong opinion about it.

I think I avoided 'no_op' in the naming because I thought it maybe could be a more generic list, but in practice I'm only using it for the dynamic noops, so I like explicitly calling it the no_ops list to make its usage clear. I also think it can still live in 1/2 with the 'no_op' name, but if that's not ok, I could just rename it in 2/2.

include/linux/livepatch.h +0 -0

 		func = iter->func;
 		iter->func++;
 	} else {

kernel/livepatch/core.c +8 -0

index e63f478..bf353da 100644
@@ -602,6 +605,118 @@ static void klp_free_patch(struct klp_patch *patch)
+
+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;

kernel/livepatch/core.c +15 -0

+
+		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;

kernel/livepatch/core.c +18 -0

+			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;
+				}

kernel/livepatch/core.c +12 -0

+				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)

kernel/livepatch/patch.c +1 -1

 	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;

kernel/livepatch/patch.c +0 -0

 		list_del(&ops->node);
 		kfree(ops);

kernel/livepatch/transition.c +1 -0

index e112826..43e1609 100644
@@ -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;

kernel/livepatch/transition.c +4 -0

+		klp_for_each_object(klp_transition_patch, obj, &o_iter) {
+			klp_for_each_func(obj, func, &f_iter) {
+				if (!func->no_op)
+					continue;

kernel/livepatch/transition.c +3 -0

+			}
+		}
+	}
 
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*

kernel/livepatch/transition.c +6 -1

+		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,

Quoted 38 lines · Jason Baron

kernel/livepatch/core.c +33 -0

index e63f478..bf353da 100644
@@ -602,6 +605,118 @@ static void klp_free_patch(struct klp_patch *patch)
+
+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;

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 647

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+			}
+			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;

This is OK if the operation replaces all older patches. Otherwise, you would need to store here the address from the patch down the stack.

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 677

Quoted 3 lines

kernel/livepatch/core.c +3 -0

+			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;

Hmm, this should be

			new->orig_func.old_addr = prev_func->new_func;

klp_check_stack_func() should check the address of the old function that is currently running. It is the variant of the function that is on top of stack.

I think that we actually have bug in the current livepatch code because old_addr always points to the original function!!! I am going to look at it.

Quoted 2 lines · Jason Baron

Introduce atomic replace, by first creating a 'no_op' klp_func for all the functions that are reverted by patch B.

include/linux/livepatch.h struct klp_object { · line 137

Quoted 9 lines

include/linux/livepatch.h +1 -0

index 5038337..6fd7222 100644
@@ -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

Is there any particular reason why the removed patch could not longer be enabled? The user could get around this by removing the replaced patch (rmmod) and applying it again (insmod) but...

Quoted 8 lines

include/linux/livepatch.h +0 -0

  * @finish:	for waiting till it is safe to remove the patch module
  */
 struct klp_patch {

@@ -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;
 

include/linux/livepatch.h static inline struct klp_func *func_iter_next(struct func_iter *iter) · line 209

Quoted 4 lines

include/linux/livepatch.h +2 -2

-	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)) {

I guess that this check should already be in the previous patch.

Quoted 17 lines

include/linux/livepatch.h +0 -0

 		func = iter->func;
 		iter->func++;
 	} else {

kernel/livepatch/core.c +8 -0

index e63f478..bf353da 100644
@@ -602,6 +605,118 @@ static void klp_free_patch(struct klp_patch *patch)
+
+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;

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 622

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	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;

We must create the no_op object/function structures also for no-yet-loaded modules. The transaction might take a long time. The module might be loaded in the meantime. It must be able to enable the stack of patches completely, including the no_op ones, see klp_module_coming().

Note that the module must be patched even temporarily because some (non-migrated) processes might still use the semantic required by the to-be-removed patches.

This is actually one big advantage of the dynamically allocated no_op functions. The infrastructure for the coming/going modules should just work. The same is true for klp_reverse_transition().

Quoted 22 lines

kernel/livepatch/core.c +20 -0

+
+		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);

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 674

Quoted 5 lines

kernel/livepatch/core.c +5 -0

+			if (!new)
+				return -ENOMEM;
+			new->orig_func = *prev_func;
+			new->orig_func.old_name = prev_func->old_name;
+			new->orig_func.new_func = NULL;

This is OK if the operation replaces all older patches. Otherwise, you would need to store here the address from the patch down the stack.

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 677

Quoted 4 lines

kernel/livepatch/core.c +3 -0

+			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;

Hmm, this should be

			new->orig_func.old_addr = prev_func->new_func;

klp_check_stack_func() should check the address of the old function that is currently running. It is the variant of the function that is on top of stack.

I think that we actually have bug in the current livepatch code because old_addr always points to the original function!!! I am going to look at it.

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 679

Quoted 2 lines

kernel/livepatch/core.c +2 -0

+			INIT_LIST_HEAD(&new->orig_func.stack_node);
+			new->orig_func.old_size = prev_func->old_size;

Same here. It should be the size of the currently used function implementation (from the last active patch).

Quoted 19 lines

kernel/livepatch/core.c +18 -0

+			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;
+				}

kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 707

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+			}
+			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;

The new temporary object should be connected with the new patch. I mean with patch->mod.

Another question is what to do with the kobjects in the dynamically allocated klp_object and klp_func. I do not have strong opinion here.

One thing is that the kobject hierarchy allows to find *patch from *obj and *obj from *func pointers. I guess that we do not rely on this in the current livepatch code. But it is a nice trick that we use in Kgraft.

Also userspace might use the related /sys/ hierarchy to monitor the livepatching situation. If we do not create the kobjects, we will hide some information.

Finally, please try to split this function into more pieces. I wonder if we could separate the code for each level (patch, object, func).

Quoted 17 lines

kernel/livepatch/core.c +12 -0

+				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)

[...]

kernel/livepatch/patch.c static void notrace klp_ftrace_handler(unsigned long ip, · line 121

Quoted 10 lines

kernel/livepatch/patch.c +2 -0

index 1cfdabc..cbb8b9d 100644
@@ -117,6 +117,8 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	if (func->no_op)
+		goto unlock;

JFYI, this means that we will use the original (unpatched) code. It is perfectly fine if we new patch replaces all older ones. It is wrong in the current concept where you want to replace only the last patch on the stack. See above my comments about orig_func.new_func.

To make it clear, I am all for replacing all patches on the stack. IMHO, it is much more practical. The result is well defined.

If you remove only the last patch, the result depends on the other patches on the stack which is harder to predict. IMHO, replacing all patches is a better semantic.

Quoted 18 lines

kernel/livepatch/patch.c +1 -1

 	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;
 

kernel/livepatch/patch.c static void klp_unpatch_func(struct klp_func *func) · line 165

Quoted 9 lines

kernel/livepatch/patch.c +5 -3

-		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);

IMHO, we should rather unpatch the replaced patches before we remove the temporary no_op stuff. Then this should not be needed, see below.

Quoted 18 lines

kernel/livepatch/patch.c +0 -0

 		list_del(&ops->node);
 		kfree(ops);

kernel/livepatch/transition.c +1 -0

index e112826..43e1609 100644
@@ -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;

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 95

Quoted 10 lines

kernel/livepatch/transition.c +7 -0

 	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) {

IMHO, this is the right place to call:

		klp_unpatch_objects(prev_patch);

It will just remove the function from the func_stack(s). All stacks have at least two functions: the replaced one and one from the new patch (real or no_op). It should keep the one from the new patch on top of the stack.

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+		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));

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 109

Quoted 4 lines

kernel/livepatch/transition.c +4 -0

+				WARN_ON(ftrace_set_filter_ip(&ops->fops,
+							     ftrace_loc,
+							     1, 0));
+				no_op = true;

I wonder if we could reuse klp_unpatch_object() and klp_unpatch_function() here. We could add a no_op parameter to unpatch only the no_op stuff.

Quoted 10 lines

kernel/livepatch/transition.c +3 -0

+			}
+		}
+	}
 
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*

@@ -132,6 +161,24 @@ static void klp_complete_transition(void)
 	}
 

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 169

Quoted 7 lines

kernel/livepatch/transition.c +6 -0

 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;

Hmm, the patch is normally marked disabled before the transition starts, see __klp_disable_patch().

We should do this in __klp_enable_patch(). It needs to be handled also in klp_reverse_transition().

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 178

Quoted 10 lines

kernel/livepatch/transition.c +9 -0

+			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);
+			}
+		}

This should be done earlier. See above the hint about adding no_op parameter to klp_unpatch_object() and klp_uptach_func() and reusing their code.

My intention is that we should ideally do the same actions in the same locations as we already do for normal patches. I mean manipulating the various flags, ftrace handlers, and other per-patch/object/func values. It should help to simply review and even reuse some more code.

Quoted 20 lines

kernel/livepatch/transition.c +8 -3

+		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) {

kernel/livepatch/transition.c static int klp_check_stack_func(struct klp_func *func, · line 264

Quoted 5 lines

kernel/livepatch/transition.c +5 -0

+				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;

This looks wrong. It should not be needed if we set new_func and new_addr correctly in klp_init_patch_no_ops().

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.

Quoted 13 lines · 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 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 :).

Quoted 8 lines

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

Quoted 7 lines

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

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.

Quoted 8 lines

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.

Yes, this should not be a problem to implement with the current code.

But I must say I am not entirely happy with the code as it is. The big advantage is that there are almost no changes to the consistency model. Almost everything is created and allocated during the initialization and the patch then looks ordinary. The current code can handle it smoothly. "Dynamic" superstructure is what I am not happy with. It is too complicated in my opinion. On the other hand I don't see a simple way out now.

1. We can try to make your code "nicer".

2. The main problem is that all user structures (klp_func, klp_object, klp_patch) are statically allocated in a kernel module. You need to add dynamically allocated structures, which is not easy. We can either change everything to dynamic allocation, or make everything static. As far as the former is concerned, we've been down that road IIRC. It didn't end up well. Is the latter even possible?

3. We could bypass everything and register special no-op fops to each reverted function. Those could be held and handled separately. It could be simpler, but it might be problematic to deal with the consistency model interaction.

Quoted 17 lines · Miroslav Benes

Quoted 14 lines

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.

Yes, this should not be a problem to implement with the current code.

Quoted 7 lines

But I must say I am not entirely happy with the code as it is. The big advantage is that there are almost no changes to the consistency model. Almost everything is created and allocated during the initialization and the patch then looks ordinary. The current code can handle it smoothly. "Dynamic" superstructure is what I am not happy with. It is too complicated in my opinion. On the other hand I don't see a simple way out now.

Indeed. It is possible create the 'no-op' functions statically and relative to a previous livepatch module at build time, but I didn't like this approach because it means that the created 'no-op' functions are tied to a specific prior livepatch module, which it is potentially also inconvenient to keep around. So for me, the dynamic creation of the 'no-op' functions is important.

Given that premise, the posted patch adds an additional linked list head to klp_patch (to add the dynamic klp_objects), and an additional linked list head to the klp_object (to add the dynamic klp_funcs). It hides this mostly behind the klp_for_each_object() and klp_for_each_func(), such that a lot of the code does not need to be adjusted. Note as well that I think in general the lists of 'no-ops' should be fairly small, and that as soon as the live patch module is enabled the linked lists are removed and thus the livepatch module looks the same as before (only contains the static allocations).

Quoted 3 lines

1. We can try to make your code "nicer".

Sure - I'm happy to incorporate any suggestions, and I could re-review to try and simplify further.

Quoted 7 lines

2. The main problem is that all user structures (klp_func, klp_object, klp_patch) are statically allocated in a kernel module. You need to add dynamically allocated structures, which is not easy. We can either change everything to dynamic allocation, or make everything static. As far as the former is concerned, we've been down that road IIRC. It didn't end up well. Is the latter even possible?

I think making everything static is possible - but I think it ties things to the previously loaded module, which I don't think is desirable.

That said we
could also potentially reallocate() the memory for the static regions and increase them to the required size (although it may be a little tricky to free the original static region b/c other data structures may be stored nearby?).

I
think its also nice that the current patch frees these 'no-op' functions when no longer needed, so by keeping them separate, this is somewhat simpler.

Quoted 5 lines

3. We could bypass everything and register special no-op fops to each reverted function. Those could be held and handled separately. It could be simpler, but it might be problematic to deal with the consistency model interaction.

Perhaps...I sort of feel like this moves us closer to handling these differently, whereas the proposal hides a lot of this behind klp_for_each_object() and klp_for_each_func().