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). This patch is intended to effectively be a no-op

until atomic replace is introduced.

include/linux/livepatch.h +1 -0

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

include/linux/livepatch.h +0 -0

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

include/linux/livepatch.h +13 -1

+		return patch->objs;
+	else
+		return NULL;
+}
+
 #define klp_for_each_object(patch, obj) \
-	for (obj = patch->objs; obj->funcs || obj->name; obj++)
+	for (obj = obj_iter_init(patch); obj; obj = obj_iter_next(patch, obj))
+
+static inline struct klp_func *func_iter_next(struct klp_object *obj,
+					      struct klp_func *func)
+{
+	struct klp_func *next_func = NULL;
+
+	if (list_empty(&func->func_entry)) {

include/linux/livepatch.h +9 -0

+		goto out;
+	}
+	if (func->func_entry.next != &obj->func_list)
+		next_func = container_of(func->func_entry.next,
+					 struct klp_func,
+					 func_entry);
+out:
+	return next_func;
+}

On Wed 2017-08-30 17:38:43, Jason Baron wrote:

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

Quoted 6 lines

include/linux/livepatch.h +6 -0

+	if (list_empty(&obj->obj_entry)) {
+		next_obj = obj + 1;
+		if (next_obj->funcs || next_obj->name)
+			goto out;
+		else
+			next_obj = NULL;

Please, add an empty line here to make it better readable.

Quoted 4 lines

include/linux/livepatch.h +4 -0

+static inline struct klp_func *func_iter_next(struct klp_object *obj,
+					      struct klp_func *func)
+{
+	struct klp_func *next_func = NULL;

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

Quoted 8 lines

include/linux/livepatch.h +8 -0

+
+	if (list_empty(&func->func_entry)) {
+		next_func = func + 1;
+		if (next_func->old_name || next_func->new_func ||
+		    next_func->old_sympos)
+			goto out;
+		else
+			next_func = NULL;

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

Quoted 4 lines

include/linux/livepatch.h +4 -0

+		if (!list_empty(&obj->func_list))
+			next_func = container_of(obj->func_list.next,
+					struct klp_func,
+					func_entry);

I have just realized that a practice is to use list_entry() instead of container_of() for list entries. It probably makes the code better readable for a trained eye.

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 replace' leverages the existing consistency model code. Then, after transition to the new code, 'atomic replace' 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 all previous livepatch modules, it explicitly disables all previous livepatch modules, 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 it is removed from the kernel with rmmod, it can be re-inserted (insmod), and act as an atomic replace on top of patch A.

include/linux/livepatch.h +0 -0

index 8d3df55..ee6d18b 100644

include/linux/livepatch.h +2 -0

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

@@ -130,12 +134,14 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool enabled;
+	bool replaced;
 	struct completion finish;
 };
 

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

kernel/livepatch/core.c +4 -0

+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+

kernel/livepatch/core.c +1 -0

+		ret = klp_init_func(obj, func, false);
 		if (ret)
 			goto free;
 	}

kernel/livepatch/core.c +4 -0

+
+	klp_for_each_object(prev_patch, prev_obj) {
+		klp_for_each_func(prev_obj, prev_func) {
+next_func:
 

kernel/livepatch/core.c +5 -0

+			mod = klp_is_module(prev_obj);
+			found = false;
+			klp_for_each_object(patch, obj) {
+				if (mod) {
+					if (klp_is_module(obj) &&

kernel/livepatch/core.c +14 -0

+			if (!found) {
+				obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+				if (!obj)
+					return -ENOMEM;
+				if (prev_obj->name) {
+					obj->name = kstrdup(prev_obj->name,
+							    GFP_KERNEL);
+					if (!obj->name) {
+						kfree(obj);
+						return -ENOMEM;
+					}
+				} else {
+					obj->name = NULL;
+				}

kernel/livepatch/core.c +12 -0

+			func = kzalloc(sizeof(*func), GFP_KERNEL);
+			if (!func)
+				return -ENOMEM;
+			if (prev_func->old_name) {
+				func->old_name = kstrdup(prev_func->old_name,
+							 GFP_KERNEL);
+				if (!func->old_name) {
+					kfree(func);
+					return -ENOMEM;
+				}
+			} else {
+				func->old_name = NULL;

kernel/livepatch/core.c +27 -0

+		}
+	}
+	return 0;
+}
+
+static int klp_init_no_ops(struct klp_patch *patch)
+{
+	struct klp_patch *prev_patch;
+	int ret = 0;
+
+	if (!patch->replace)
+		return 0;
+
+	prev_patch = patch;
+	while (prev_patch->list.prev != &klp_patches) {
+		prev_patch = list_prev_entry(prev_patch, list);
+
+		ret = klp_init_patch_no_ops(prev_patch, patch);
+		if (ret)
+			return ret;
+
+		if (prev_patch->replace)
+			break;
+	}
+	return 0;
+}
+
 static int klp_init_patch(struct klp_patch *patch)
 {

kernel/livepatch/core.c +2 -0

 	struct klp_object *obj;

@@ -721,6 +866,8 @@ 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,

kernel/livepatch/core.c +2 -0

+
 	mutex_unlock(&klp_mutex);
 
 	return 0;
 
 free:
+	klp_patch_free_no_ops(patch);
 	klp_free_objects_limited(patch, obj);
 
 	mutex_unlock(&klp_mutex);

kernel/livepatch/transition.c +0 -0

index b004a1f..d5e620e 100644

kernel/livepatch/transition.c +3 -0

 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.

@@ -81,14 +84,39 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	bool no_op = false;
+	struct klp_patch *prev_patch;
+

Quoted 4 lines · Jason Baron

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.

This is a bit confusing. One might thing that the function 1 still uses the code added by the patch A. I would say something like:

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

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.

For this, a new "replace" flag is added to the struct klp_patch. When it is enabled, 'no_op' 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...

Quoted 22 lines

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 replace' leverages the existing consistency model code. Then, after transition to the new code, 'atomic replace' 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 all previous livepatch modules, it explicitly disables all previous livepatch modules, 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 it is removed from the kernel with rmmod, it can be re-inserted (insmod), and act as an atomic replace on top of patch A.

include/linux/livepatch.h +0 -0

index 8d3df55..ee6d18b 100644

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

Quoted 9 lines

include/linux/livepatch.h +2 -0

@@ -119,10 +121,12 @@ 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:	replace all funcs, reverting functions that are no longer patched
  * @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)
+ * @replaced:	the patch has been replaced an can not be re-enabled

I finally understand why you do this. I forgot that even disabled patches are still in klp_patch list.

This makes some sense for patches that fix different bugs and touch the same function. They should be applied and enabled in the right order because a later patch for the same function must be based on the code from the previous one.

It makes less sense for cummulative patches that we use in kGraft. There basically every patch has the "replace" flag set. If we enable one patch it simply replaces any other one. Then ordering is not important. Each patch is standalone and consistent on its own.

I see two problems with your approach. One is cosmetic. The names of the two flags replace/replaced are too similar. It is quite prone for mistakes ;-)

Second, we could not remove module where any function is patched usign the "immediate" flag. But people might want to revert to this patch if the last one does not work as expected. After all the main purpose of livepatching is to fix problems without a system reboot. Therefore we should allow to enable the replaced patches even without removing the module.

One solution would be to remove the replaced patches from klp_patch list. And enforce stacking the following way in __klp_enable_patch():

	/*
	 * Enforce stacking: only the first disabled patch can be
	 * enabled. The only exception is a patch that atomically
	 * replaces all other patches and was disabled by
	 * another patch of this type.
	 */
	if (list_empty(&patch->list)) {
		if (!patch->replace)
			return -EBUSY;
		list_add_tail(&patch->list, &klp_patches);
	} else if (patch->list.prev != &klp_patches &&
		   !list_prev_entry(patch, list)->enabled) {
		return -EBUSY;
	}

Well, I would add this support later in a separate patch. We would need to (re)generate the no_op stuff in __klp_enable_patch() for this. Also we would need to make sure that this patch could not longer get enabled once klp_unregister_patch() is called.

For now, I would just remove the replaced patches from klp_patches list and refuse enabling patches that are not in the list. I mean to use a check of list_empty(&patch->list) instead of the extra "replaced" variable.

Quoted 31 lines

include/linux/livepatch.h +2 -0

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

@@ -130,12 +134,14 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool enabled;
+	bool replaced;
 	struct completion finish;
 };
 

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

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

Quoted 10 lines

kernel/livepatch/core.c +8 -0

+		}
+		INIT_LIST_HEAD(&obj->func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kobject_put(&obj->kobj);
+		kfree(obj->name);
+		kfree(obj);

Same here.

Quoted 4 lines

kernel/livepatch/core.c +4 -0

+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+

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

Quoted 10 lines

kernel/livepatch/core.c +4 -2

+static int klp_init_func(struct klp_object *obj, struct klp_func *func,
+			 bool no_op)
 {
-	if (!func->old_name || !func->new_func)
+	if (!func->old_name || (!no_op && !func->new_func))
 		return -EINVAL;
 
-	INIT_LIST_HEAD(&func->stack_node);
 	INIT_LIST_HEAD(&func->func_entry);
+	INIT_LIST_HEAD(&func->stack_node);

This just shuffles a line that was added in the previous patch ;-)

kernel/livepatch/core.c static int klp_init_object_loaded(struct klp_patch *patch, · line 702

Quoted 10 lines

kernel/livepatch/core.c +2 -1

 	func->patched = false;
 	func->transition = false;
 

@@ -670,19 +698,23 @@ 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 no_op)

It we initialized the no_op stuff earlier, we might pass this information via obj->no_op. See below for more details.

kernel/livepatch/core.c static int klp_init_object_loaded(struct klp_patch *patch, · line 714

Quoted 13 lines

kernel/livepatch/core.c +3 -1

 {
 	struct klp_func *func;
 	int ret;
 	const char *name;
 
-	if (!obj->funcs)
+	if (!obj->funcs && !no_op)
 		return -EINVAL;
 
 	obj->patched = false;
 	obj->mod = NULL;
+	INIT_LIST_HEAD(&obj->obj_entry);
+	INIT_LIST_HEAD(&obj->func_list);

This should be done already in the previous patch. Well, we might want to change this to

	if (!obj->no_op)
		INIT_LIST_HEAD(&obj->func_list);

Quoted 1 line

kernel/livepatch/core.c static int klp_init_object_loaded(struct klp_patch *patch, · line 717

Quoted 3 lines

kernel/livepatch/core.c +2 -1

-	klp_find_object_module(obj);
+	if (!no_op)
+		klp_find_object_module(obj);

This looks just like an optimization. You copy the information from the related to-be-removed patch. I would avoid it to reduce the extra changes and twists in the code. It is a rare slow path.

Instead, I suggest to initialize the no_op related stuff earlier and then call the classic init() methods that would fill the rest. See below.

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 726

Quoted 8 lines

kernel/livepatch/core.c +2 -0

 	name = klp_is_module(obj) ? obj->name : "vmlinux";
 	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,

@@ -690,8 +722,12 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (ret)
 		return ret;
 
+	if (no_op)
+		return 0;

This might be hard to maintain. It seems to be needed because you call klp_init_no_ops() after you already klp_init_object() for the statically defined ones.

Alternative solution would be to add the dynamically allocated structures earlier and call basically the unchanged klp_init_object() for all of them. See below for more details.'

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 729

Quoted 5 lines

kernel/livepatch/core.c +2 -1

+
 	klp_for_each_func(obj, func) {
-		ret = klp_init_func(obj, func);
+		func->no_op = false;

This is weird and not needed. We should not touch it here. Note that statically defined structures are zeroed except for the explicitely defined members. Therefore it is false out of box. It should be set earlier for the dynamic ones.

Quoted 5 lines

kernel/livepatch/core.c +1 -0

+		ret = klp_init_func(obj, func, false);
 		if (ret)
 			goto free;
 	}

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 755

Quoted 10 lines

kernel/livepatch/core.c +7 -0

@@ -710,6 +746,115 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	return ret;
 }
 
+static int klp_init_patch_no_ops(struct klp_patch *prev_patch,
+				 struct klp_patch *patch)
+{
+	struct klp_object *obj, *prev_obj;
+	struct klp_func *prev_func, *func;
+	int ret;
+	bool found, mod;

This is a lot of spagetti code: more than one sceen and 6 indentation levels. Also all the use of prev_patch is rather confusing.

If we call this function before we initialize the statically defined objects. Then we could keep the original klp_init_* functions almost without changes.

First, I suggest to create a function that would initialize just the stuff specific for the dynamically allocated stuff.

static void klp_init_patch_dyn(struct klp_patch *patch)

{
	struct klp_object *obj;
	struct klp_object *func;

	INIT_LIST_HEAD(&patch->obj_list);
	klp_for_each_object(patch, obj) {
		INIT_LIST_HEAD(&obj->obj_entry);
		INIT_LIST_HEAD(&obj->func_list);
		klp_for_each_func(obj, func) {
			INIT_LIST_HEAD(&func->func_entry);
		}
	}
}

I still suggest to make clear that these list items are not generic. I would prefer to make it clear that they are used by the no_op stuff =>

  patch->obj_list  -> patch->nop_obj_list
  obj->obj_entry   -> obj->nop_obj_entry
  obj->func_list   -> obj->nop_func_list

func->func_entry -> func->nop_func_entry

Note that I used "nop" instead of "no_op". I think that it is a well known shortcut.

let's go back to the init stuff. As a second step, we could allocate and add the structures for the no_op operations. We should fill the no_op-specific stuff and the items that are normally pre-set in statically defined structures. Then we could call the classic klp_init_object().

I would split this into several levels. I do not say that the following is the best split but...

static int klp_check_and_add_no_op_func(struct klp_patch *obj,
struct klp_func *old_func)

{
	struct klp_func *func;

	func = klp_find_func(obj, old_func->old_name, old_func->old_sympos);
	if (func)
		return 0;

	func = klp_create_no_op_func(old_obj->name);
	if (IS_ERR(func))
		return PTR_ERR(func);

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

	return 0;
}

static int klp_check_and_add_no_op_object(struct klp_patch *patch,
struct klp_object *old_obj)

{
	struct klp_object *obj;
	struct klp_func *old_func, *func;
	int err = 0;

	obj = klp_find_obj(patch, old_obj->name);
	if (!obj) {
		obj = klp_create_no_op_object(old_obj->name);
		if (IS_ERR(obj))
			   return PTR_ERR(obj);

		list_add(&obj->obj_entry, &patch->obj_list);
	}

	klp_for_each_func(old_obj, old_func) {
		err = klp_check_and_add_no_op_func(obj, old_func);
		if (err)
			// destroy previously created no_op stuff?
	}

	return 0;
}

static int klp_check_and_add_no_ops(struct klp_patch *patch)

{
	struct klp_patch *old_patch;
	struct klp_object *old_obj, *obj;
	int err = 0;

	list_for_each_entry(old_patch, &klp_patches, list) {
		klp_for_each_object(old_patch, old_obj) {
			err = klp_check_and_add_no_op_object(patch,
					old_obj);
			if (err)
				return err;
		}
	}

	return 0;
}

It is more code but it should be better readable and manageable. Especially, the new functions handle only the no_op specific stuff. They do not duplicate functionality that could be done by the original init() functions for the statically defined structures.

Quoted 6 lines

kernel/livepatch/core.c +4 -0

+
+	klp_for_each_object(prev_patch, prev_obj) {
+		klp_for_each_func(prev_obj, prev_func) {
+next_func:
 

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 769

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+			klp_for_each_object(patch, obj) {
+				klp_for_each_func(obj, func) {
+					if ((strcmp(prev_func->old_name,
+						    func->old_name) == 0) &&
+						(prev_func->old_sympos ==
+							func->old_sympos)) {
+						goto next_func;
+					}
+				}
+			}

This could be used to implement that klp_find_func().

Quoted 5 lines

kernel/livepatch/core.c +5 -0

+			mod = klp_is_module(prev_obj);
+			found = false;
+			klp_for_each_object(patch, obj) {
+				if (mod) {
+					if (klp_is_module(obj) &&

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 784

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+					    strcmp(prev_obj->name,
+						   obj->name) == 0) {
+						found = true;
+						break;
+					}
+				} else if (!klp_is_module(obj)) {
+					found = true;
+					break;
+				}
+			}

This could be used to implement klp_find_object().

Quoted 14 lines

kernel/livepatch/core.c +14 -0

+			if (!found) {
+				obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+				if (!obj)
+					return -ENOMEM;
+				if (prev_obj->name) {
+					obj->name = kstrdup(prev_obj->name,
+							    GFP_KERNEL);
+					if (!obj->name) {
+						kfree(obj);
+						return -ENOMEM;
+					}
+				} else {
+					obj->name = NULL;
+				}

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 808

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+				obj->funcs = NULL;
+				ret = klp_init_object(patch, obj, true);
+				if (ret) {
+					kfree(obj->name);
+					kfree(obj);
+					return ret;
+				}
+				obj->mod = prev_obj->mod;
+				list_add(&obj->obj_entry, &patch->obj_list);
+			}

This is a base for the klp_create_no_op_object(). It should set everything that will be needed for the classic klp_init_object() plus that no_op specific stuff. It might might looks like:

static struct klp_object *
klp_create_no_op_object(const char *name)

{
	struct klp_object *obj;

	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
	if (!obj)
		return ERR_PTR(-ENOMEM);

	obj->no_op = true;
	INIT_LIST_HEAD(&obj->func_list);

	if (name) {
		obj->name = kstrdup(name, GFP_KERNEL);
		if (!obj->name) {
			kfree(obj);
			return ERR_PRT(-ENOMEM);
		}
	}

	return obj;
}

Quoted 12 lines

kernel/livepatch/core.c +12 -0

+			func = kzalloc(sizeof(*func), GFP_KERNEL);
+			if (!func)
+				return -ENOMEM;
+			if (prev_func->old_name) {
+				func->old_name = kstrdup(prev_func->old_name,
+							 GFP_KERNEL);
+				if (!func->old_name) {
+					kfree(func);
+					return -ENOMEM;
+				}
+			} else {
+				func->old_name = NULL;

kernel/livepatch/core.c static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) · line 830

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+			}
+			func->new_func = NULL;
+			func->old_sympos = prev_func->old_sympos;
+			ret = klp_init_func(obj, func, true);
+			func->immediate = prev_func->immediate;
+			func->old_addr = prev_func->old_addr;
+			func->old_size = prev_func->old_size;
+			func->new_size = 0;
+			func->no_op = true;
+			list_add(&func->func_entry, &obj->func_list);

And this is a base for klp_create_no_op_func(). Something like:

static struct klp_func *
klp_create_no_op_func(const char *old_name,
unsigned long old_sympos,
bool immediate)

{
	if (!old_name)
		return ERR_PTR(-EINVAL);

	func = kzalloc(sizeof(*func), GFP_KERNEL);
	if (!func)
		return ERR_PTR(-ENOMEM);

	func->old_name = kstrdup(old_name, GFP_KERNEL);
	if (!func->old_name) {
		kfree(func);
		return ERR_PTR(-ENOMEM);
	}

	func->old_sympos = old_sympos;
	func->immediate = immediate;
	func->no_op = true;

	return func;
}

Quoted 40 lines

kernel/livepatch/core.c +27 -0

+		}
+	}
+	return 0;
+}
+
+static int klp_init_no_ops(struct klp_patch *patch)
+{
+	struct klp_patch *prev_patch;
+	int ret = 0;
+
+	if (!patch->replace)
+		return 0;
+
+	prev_patch = patch;
+	while (prev_patch->list.prev != &klp_patches) {
+		prev_patch = list_prev_entry(prev_patch, list);
+
+		ret = klp_init_patch_no_ops(prev_patch, patch);
+		if (ret)
+			return ret;
+
+		if (prev_patch->replace)
+			break;
+	}
+	return 0;
+}
+
 static int klp_init_patch(struct klp_patch *patch)
 {

kernel/livepatch/core.c +2 -0

 	struct klp_object *obj;

@@ -721,6 +866,8 @@ 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,

kernel/livepatch/core.c static int klp_init_patch(struct klp_patch *patch) · line 882

Quoted 7 lines

kernel/livepatch/core.c +1 -3

@@ -732,20 +879,25 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	INIT_LIST_HEAD(&patch->obj_list);
 	klp_for_each_object(patch, obj) {
-		INIT_LIST_HEAD(&obj->obj_entry);
-		INIT_LIST_HEAD(&obj->func_list);
-		ret = klp_init_object(patch, obj);
+		ret = klp_init_object(patch, obj, false);

The extra parameter still might be needed because only no_ops need to get re-added and reinitialized when the disabled patch is enabled again. Well, this feature might be added later in a separate patch.

kernel/livepatch/core.c static int klp_init_patch(struct klp_patch *patch) · line 893

Quoted 11 lines

kernel/livepatch/core.c +5 -0

 		if (ret)
 			goto free;
 	}
 
 	list_add_tail(&patch->list, &klp_patches);
 
+	ret = klp_init_no_ops(patch);
+	if (ret) {
+		list_del(&patch->list);
+		goto free;
+	}

This should be done earlier using that klp_check_and_add_no_ops().

Quoted 15 lines

kernel/livepatch/core.c +2 -0

+
 	mutex_unlock(&klp_mutex);
 
 	return 0;
 
 free:
+	klp_patch_free_no_ops(patch);
 	klp_free_objects_limited(patch, obj);
 
 	mutex_unlock(&klp_mutex);

kernel/livepatch/transition.c +0 -0

index b004a1f..d5e620e 100644

kernel/livepatch/transition.c static void klp_synchronize_transition(void) · line 75

Quoted 4 lines

kernel/livepatch/transition.c +1 -0

@@ -70,6 +72,7 @@ static void klp_synchronize_transition(void)
 	schedule_on_each_cpu(klp_sync);
 }
 
+

extra empty line?

Quoted 10 lines

kernel/livepatch/transition.c +3 -0

 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.

@@ -81,14 +84,39 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	bool no_op = false;
+	struct klp_patch *prev_patch;
+

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

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+	/*
+	 * for replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook. After
+	 * klp_synchronize_transition() is called its safe to free the
+	 * the dynamic no-op functions, done by klp_patch_free_no_ops()
+	 */
+	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
+		prev_patch = klp_transition_patch;
+		while (prev_patch->list.prev != &klp_patches) {
+			prev_patch = list_prev_entry(prev_patch, list);

I wonder if the following code might be more obvious:

	list_for_each_entry(old_patch, &klp_patches, list) {
		if (old_patch = klp_transition_patch)
			continue;

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

Quoted 3 lines

kernel/livepatch/transition.c +3 -0

+			if (prev_patch->enabled) {
+				klp_unpatch_objects(prev_patch, false);
+				prev_patch->enabled = false;

I suggested to do this in __klp_disable_patch() in the last review. But we need something else.

The fact is that patch->enable is normally manipulated at the beginning of the transition, see __klp_enable_patch() and __klp_disable_patch(). Therefore we are kind of inconsistent when we manipulate it here for the replaced patchses.>

But we must keep the patches enabled during the entire transition. Othewise, klp_module_coming() and klp_module_going() would not patch/unpatch the objects correctly.

In fact, I think that we should set klp_transition_patch->enabled = false in klp_complete_transition() instead of in __klp_disable_patch(). I mean that flag should reflect whether the ftrace handlers see the patch or not. Therefore it should be always true during the transition. Then we would not need the extra check for klp_transition_patch in coming/going handlers. Well, this should be done in separate patch.
I could prepare it if you want.

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

Quoted 4 lines

kernel/livepatch/transition.c +2 -0

+				prev_patch->replaced = true;
+				module_put(prev_patch->mod);

In each case, we could do this only when the related no_op functions were not applied using "immediate" flag.

I am not 100% sure that it is really necessary. But I would feel more comfortable when we do this after klp_synchronize_transition(); so that any ftrace handler could not see the related functions on the stack.

Quoted 4 lines

kernel/livepatch/transition.c +3 -0

+			}
+		}
+		klp_unpatch_objects(klp_transition_patch, true);

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

Quoted 10 lines

kernel/livepatch/transition.c +3 -1

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

This code patch made me think about a situation when the "enable" operation was reverted during the transition. Then the target state is KLP_UNPATCHED and no_op stuff is there.

Well, in this case, we need to keep the no_op stuff until either the the patch is enabled egain and the transition finishes or we need to remove/free it klp_unregister_patch().

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

Quoted 8 lines

kernel/livepatch/transition.c +2 -0

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

Best Regards,
Petr

Quoted 35 lines · Petr Mladek

Quoted 5 lines · Jason Baron

include/linux/livepatch.h +0 -0

index 8d3df55..ee6d18b 100644

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

Quoted 9 lines

include/linux/livepatch.h +2 -0

@@ -119,10 +121,12 @@ 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:	replace all funcs, reverting functions that are no longer patched
  * @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)
+ * @replaced:	the patch has been replaced an can not be re-enabled

I finally understand why you do this. I forgot that even disabled patches are still in klp_patch list.

This makes some sense for patches that fix different bugs and touch the same function. They should be applied and enabled in the right order because a later patch for the same function must be based on the code from the previous one.

It makes less sense for cummulative patches that we use in kGraft. There basically every patch has the "replace" flag set. If we enable one patch it simply replaces any other one. Then ordering is not important. Each patch is standalone and consistent on its own.

I see two problems with your approach. One is cosmetic. The names of the two flags replace/replaced are too similar. It is quite prone for mistakes ;-)

Quoted 8 lines

Second, we could not remove module where any function is patched usign the "immediate" flag. But people might want to revert to this patch if the last one does not work as expected. After all the main purpose of livepatching is to fix problems without a system reboot. Therefore we should allow to enable the replaced patches even without removing the module.

If the livepatch has the 'replace' bit set and not the 'immediate' bit, then I believe that we could remove *all* types of previous livepatches even if they have the 'immediate' flag set. That is, because of the consistency model, we are sure that we are running only functions from the cumulative replace patch.

So I think it may be worth making a distinction between patches that have 'replace' bit set and the immediate bit *unset*, and those that have the 'replace' bit set and the immediate bit *set*. So it seems to me that this patchset could just reject 'replace' patches that have the 'immediate' flag set, and treat the 'immediate' flag being set as separate todo item if there is interest?

Then, the 'replace' patch with 'immediate' not set, conceptually does a 'disable' and an 'unregister' of all previous live patches, such that they could be removed (rmmod) and re-inserted.

Quoted 25 lines

One solution would be to remove the replaced patches from klp_patch list. And enforce stacking the following way in __klp_enable_patch():

	/*
	 * Enforce stacking: only the first disabled patch can be
	 * enabled. The only exception is a patch that atomically
	 * replaces all other patches and was disabled by
	 * another patch of this type.
	 */
	if (list_empty(&patch->list)) {
		if (!patch->replace)
			return -EBUSY;
		list_add_tail(&patch->list, &klp_patches);
	} else if (patch->list.prev != &klp_patches &&
		   !list_prev_entry(patch, list)->enabled) {
		return -EBUSY;
	}

Well, I would add this support later in a separate patch. We would need to (re)generate the no_op stuff in __klp_enable_patch() for this. Also we would need to make sure that this patch could not longer get enabled once klp_unregister_patch() is called.

Quoted 5 lines

For now, I would just remove the replaced patches from klp_patches list and refuse enabling patches that are not in the list. I mean to use a check of list_empty(&patch->list) instead of the extra "replaced" variable.

Ok, that makes sense to me as a way to remove 'replaced'.

Quoted 55 lines

Quoted 31 lines

include/linux/livepatch.h +2 -0

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

@@ -130,12 +134,14 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool enabled;
+	bool replaced;
 	struct completion finish;
 };
 

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

Quoted 4 lines

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

Thanks for pointing this out - it looks like the livepatch code uses wait_for_completion() with special kobj callbacks. Perhaps, there could be a nop callback, but I'd have to look at this more closely...

Quoted 63 lines

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

Quoted 8 lines

kernel/livepatch/core.c +8 -0

+		}
+		INIT_LIST_HEAD(&obj->func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kobject_put(&obj->kobj);
+		kfree(obj->name);
+		kfree(obj);

Same here.

Quoted 11 lines

kernel/livepatch/core.c +4 -0

+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+
 
 

kernel/livepatch/transition.c +0 -0

index b004a1f..d5e620e 100644

kernel/livepatch/transition.c static void klp_synchronize_transition(void) · line 75

Quoted 4 lines

kernel/livepatch/transition.c +1 -0

@@ -70,6 +72,7 @@ static void klp_synchronize_transition(void)
 	schedule_on_each_cpu(klp_sync);
 }
 
+

extra empty line?

Quoted 10 lines

kernel/livepatch/transition.c +3 -0

 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.

@@ -81,14 +84,39 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	bool no_op = false;
+	struct klp_patch *prev_patch;
+

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

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+	/*
+	 * for replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook. After
+	 * klp_synchronize_transition() is called its safe to free the
+	 * the dynamic no-op functions, done by klp_patch_free_no_ops()
+	 */
+	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
+		prev_patch = klp_transition_patch;
+		while (prev_patch->list.prev != &klp_patches) {
+			prev_patch = list_prev_entry(prev_patch, list);

I wonder if the following code might be more obvious:

	list_for_each_entry(old_patch, &klp_patches, list) {
		if (old_patch = klp_transition_patch)
			continue;

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

Quoted 3 lines

kernel/livepatch/transition.c +3 -0

+			if (prev_patch->enabled) {
+				klp_unpatch_objects(prev_patch, false);
+				prev_patch->enabled = false;

I suggested to do this in __klp_disable_patch() in the last review. But we need something else.

Quoted 5 lines

The fact is that patch->enable is normally manipulated at the beginning of the transition, see __klp_enable_patch() and __klp_disable_patch(). Therefore we are kind of inconsistent when we manipulate it here for the replaced patchses.>

ok, the suggestion is that for the non-immediate replace we do a 'special' disable here, once we know that the transition patch has been successfully enabled.

Quoted 3 lines

But we must keep the patches enabled during the entire transition. Othewise, klp_module_coming() and klp_module_going() would not patch/unpatch the objects correctly.

I think the patch does, its only when its been successfully enabled that it goes and disables the previous patches.

Quoted 9 lines

In fact, I think that we should set klp_transition_patch->enabled = false in klp_complete_transition() instead of in __klp_disable_patch(). I mean that flag should reflect whether the ftrace handlers see the patch or not. Therefore it should be always true during the transition. Then we would not need the extra check for klp_transition_patch in coming/going handlers. Well, this should be done in separate patch.
I could prepare it if you want.

I don't quite see why this is needed for this patch series

I think the added nops should be handled the way all the other functions are...

Quoted 8 lines

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

Quoted 5 lines

kernel/livepatch/transition.c +2 -0

+				prev_patch->replaced = true;
+				module_put(prev_patch->mod);

In each case, we could do this only when the related no_op functions were not applied using "immediate" flag.

yes.

Quoted 1 line

Quoted 4 lines

I am not 100% sure that it is really necessary. But I would feel more comfortable when we do this after klp_synchronize_transition(); so that any ftrace handler could not see the related functions on the stack.

Which bit are you referring to here?

Quoted 20 lines

Quoted 4 lines

kernel/livepatch/transition.c +3 -0

+			}
+		}
+		klp_unpatch_objects(klp_transition_patch, true);

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

Quoted 10 lines

kernel/livepatch/transition.c +3 -1

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

This code patch made me think about a situation when the "enable" operation was reverted during the transition. Then the target state is KLP_UNPATCHED and no_op stuff is there.

Quoted 4 lines

Well, in this case, we need to keep the no_op stuff until either the the patch is enabled egain and the transition finishes or we need to remove/free it klp_unregister_patch().

In the case that klp_cancel_transition() calls klp_complete_transition() and we remove the nop functions, we also remove all the regular 'functions', which should be re-added on the function stacks if the patch is re-enabled. So I think this is fine.

Quoted 45 lines · Jason Baron

Quoted 35 lines · Petr Mladek

Quoted 5 lines · Jason Baron

include/linux/livepatch.h +0 -0

index 8d3df55..ee6d18b 100644

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

Quoted 9 lines

include/linux/livepatch.h +2 -0

@@ -119,10 +121,12 @@ 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:	replace all funcs, reverting functions that are no longer patched
  * @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)
+ * @replaced:	the patch has been replaced an can not be re-enabled

I finally understand why you do this. I forgot that even disabled patches are still in klp_patch list.

This makes some sense for patches that fix different bugs and touch the same function. They should be applied and enabled in the right order because a later patch for the same function must be based on the code from the previous one.

It makes less sense for cummulative patches that we use in kGraft. There basically every patch has the "replace" flag set. If we enable one patch it simply replaces any other one. Then ordering is not important. Each patch is standalone and consistent on its own.

I see two problems with your approach. One is cosmetic. The names of the two flags replace/replaced are too similar. It is quite prone for mistakes ;-)

Quoted 8 lines

Second, we could not remove module where any function is patched usign the "immediate" flag. But people might want to revert to this patch if the last one does not work as expected. After all the main purpose of livepatching is to fix problems without a system reboot. Therefore we should allow to enable the replaced patches even without removing the module.

Quoted 5 lines

If the livepatch has the 'replace' bit set and not the 'immediate' bit, then I believe that we could remove *all* types of previous livepatches even if they have the 'immediate' flag set. That is, because of the consistency model, we are sure that we are running only functions from the cumulative replace patch.

You are partly right. The catch is if a function was previously patched using the immediate flag and the function is not longer patched by the new cumulative patch with the 'replace' flag. Then we need to create "no_op" for the function and the 'no_op' must inherit the immediate flag set. Then the consistency model does not guarantee that the code from the older patch is not running and we could not allow to remove the older patch.

Quoted 7 lines

So I think it may be worth making a distinction between patches that have 'replace' bit set and the immediate bit *unset*, and those that have the 'replace' bit set and the immediate bit *set*. So it seems to me that this patchset could just reject 'replace' patches that have the 'immediate' flag set, and treat the 'immediate' flag being set as separate todo item if there is interest?

I would do it the other way. If we apply a livepatch with both "replace" and "immediate" flag set then I would avoid calling module_put() for all the replaced patches. IMHO, it is a useful feature and this is safe and easy.

We could later add an optimization that would allow to remove patches that were not immediate and where all the functions were replaced without the immediate flag as well.

I guess that you know this. But just for sure note that there are two possibilities. The "immediate" flag might be set either for the entire patch (struct klp_patch) or for particular functions (struct klp_func).

Quoted 3 lines

Then, the 'replace' patch with 'immediate' not set, conceptually does a 'disable' and an 'unregister' of all previous live patches, such that they could be removed (rmmod) and re-inserted.

Yes, if you consider also the flags used by "no_op" functions.

Quoted 88 lines

Quoted 25 lines

One solution would be to remove the replaced patches from klp_patch list. And enforce stacking the following way in __klp_enable_patch():

	/*
	 * Enforce stacking: only the first disabled patch can be
	 * enabled. The only exception is a patch that atomically
	 * replaces all other patches and was disabled by
	 * another patch of this type.
	 */
	if (list_empty(&patch->list)) {
		if (!patch->replace)
			return -EBUSY;
		list_add_tail(&patch->list, &klp_patches);
	} else if (patch->list.prev != &klp_patches &&
		   !list_prev_entry(patch, list)->enabled) {
		return -EBUSY;
	}

Well, I would add this support later in a separate patch. We would need to (re)generate the no_op stuff in __klp_enable_patch() for this. Also we would need to make sure that this patch could not longer get enabled once klp_unregister_patch() is called.

Quoted 5 lines

For now, I would just remove the replaced patches from klp_patches list and refuse enabling patches that are not in the list. I mean to use a check of list_empty(&patch->list) instead of the extra "replaced" variable.

Ok, that makes sense to me as a way to remove 'replaced'.

Quoted 55 lines

Quoted 31 lines

include/linux/livepatch.h +2 -0

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

@@ -130,12 +134,14 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool enabled;
+	bool replaced;
 	struct completion finish;
 };
 

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

Quoted 8 lines

Quoted 4 lines

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "the enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

Thanks for pointing this out - it looks like the livepatch code uses wait_for_completion() with special kobj callbacks. Perhaps, there could be a nop callback, but I'd have to look at this more closely...

The completion is usable for the root kobject but you do not free it here. It might be pretty complicated if you need separate completion for all the freed kobjects.

A solution might be a global atomic counter and a waitqueue. Feel free to ask for more details.

Quoted 91 lines

Quoted 64 lines

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

Quoted 9 lines

kernel/livepatch/core.c +8 -0

+		}
+		INIT_LIST_HEAD(&obj->func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kobject_put(&obj->kobj);
+		kfree(obj->name);
+		kfree(obj);

Same here.

Quoted 11 lines

kernel/livepatch/core.c +4 -0

+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+
 
 

kernel/livepatch/transition.c +0 -0

index b004a1f..d5e620e 100644

kernel/livepatch/transition.c static void klp_synchronize_transition(void) · line 75

Quoted 4 lines

kernel/livepatch/transition.c +1 -0

@@ -70,6 +72,7 @@ static void klp_synchronize_transition(void)
 	schedule_on_each_cpu(klp_sync);
 }
 
+

extra empty line?

Quoted 10 lines

kernel/livepatch/transition.c +3 -0

 /*
  * The transition to the target patch state is complete.  Clean up the data
  * structures.

@@ -81,14 +84,39 @@ static void klp_complete_transition(void)
 	struct task_struct *g, *task;
 	unsigned int cpu;
 	bool immediate_func = false;
+	bool no_op = false;
+	struct klp_patch *prev_patch;
+

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

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+	/*
+	 * for replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook. After
+	 * klp_synchronize_transition() is called its safe to free the
+	 * the dynamic no-op functions, done by klp_patch_free_no_ops()
+	 */
+	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
+		prev_patch = klp_transition_patch;
+		while (prev_patch->list.prev != &klp_patches) {
+			prev_patch = list_prev_entry(prev_patch, list);

I wonder if the following code might be more obvious:

	list_for_each_entry(old_patch, &klp_patches, list) {
		if (old_patch = klp_transition_patch)
			continue;

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

Quoted 3 lines

kernel/livepatch/transition.c +3 -0

+			if (prev_patch->enabled) {
+				klp_unpatch_objects(prev_patch, false);
+				prev_patch->enabled = false;

I suggested to do this in __klp_disable_patch() in the last review. But we need something else.

Quoted 5 lines

The fact is that patch->enable is normally manipulated at the beginning of the transition, see __klp_enable_patch() and __klp_disable_patch(). Therefore we are kind of inconsistent when we manipulate it here for the replaced patchses.>

ok, the suggestion is that for the non-immediate replace we do a 'special' disable here, once we know that the transition patch has been successfully enabled.

Quoted 3 lines

But we must keep the patches enabled during the entire transition. Othewise, klp_module_coming() and klp_module_going() would not patch/unpatch the objects correctly.

I think the patch does, its only when its been successfully enabled that it goes and disables the previous patches.

Quoted 9 lines

In fact, I think that we should set klp_transition_patch->enabled = false in klp_complete_transition() instead of in __klp_disable_patch(). I mean that flag should reflect whether the ftrace handlers see the patch or not. Therefore it should be always true during the transition. Then we would not need the extra check for klp_transition_patch in coming/going handlers. Well, this should be done in separate patch.
I could prepare it if you want.

Quoted 1 line

I don't quite see why this is needed for this patch series

Yes, please, see the last two sentences of my paragraph. I suggested to do this in a separate patch. I also offered to provide it.

Quoted 2 lines

I think the added nops should be handled the way all the other functions are...

It is not about no_ops. It is about the "enabled" flag in struct klp_patch. This patchset handles this flag differently for replaced patches than we do it for a normally disabled ones.

Well, it is a detail that I wanted to point out. The handling of replaced patches looks fine after all. Feel free to handle the "enabled" flag as you do now.

Quoted 17 lines

Quoted 8 lines

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

Quoted 5 lines

kernel/livepatch/transition.c +2 -0

+				prev_patch->replaced = true;
+				module_put(prev_patch->mod);

In each case, we could do this only when the related no_op functions were not applied using "immediate" flag.

yes.

Quoted 5 lines

I am not 100% sure that it is really necessary. But I would feel more comfortable when we do this after klp_synchronize_transition(); so that any ftrace handler could not see the related functions on the stack.

Quoted 1 line

Which bit are you referring to here?

I mean that we should call module_put() for all the replaced patches later in klp_complete_transition(). In particular, we should do so after we call klp_synchronize_transition() for klp_target_state == KLP_PATCHED variant.

Quoted 26 lines

Quoted 21 lines

Quoted 5 lines

kernel/livepatch/transition.c +3 -0

+			}
+		}
+		klp_unpatch_objects(klp_transition_patch, true);

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

Quoted 10 lines

kernel/livepatch/transition.c +3 -1

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

This code patch made me think about a situation when the "enable" operation was reverted during the transition. Then the target state is KLP_UNPATCHED and no_op stuff is there.

Quoted 4 lines

Well, in this case, we need to keep the no_op stuff until either the the patch is enabled egain and the transition finishes or we need to remove/free it klp_unregister_patch().

Quoted 4 lines

In the case that klp_cancel_transition() calls klp_complete_transition() and we remove the nop functions, we also remove all the regular 'functions', which should be re-added on the function stacks if the patch is re-enabled. So I think this is fine.

But we do not remove no_op when klp_target_state == KLP_PATCHED. And this might never happen when we call klp_reverse_transition() in the middle of the transition.

I see two solutions:

Either we could postpone generating the no_op functions until __klp_enable_patch() call and always free them in klp_finish_transition().

Or we need to check if some no_op functions are still there when __klp_unregister_patch() is called and free them there.

In the long term, we would need the first solution because it would allow to re-enable the replaced patches. But it will be more complicated to reuse the existing code, especially the init functions.

Feel free to keep it easy and implement
the 2nd possibility in this patch(set).

Quoted 52 lines · Petr Mladek

Quoted 45 lines · Jason Baron

Quoted 35 lines · Petr Mladek

Quoted 5 lines · Jason Baron

include/linux/livepatch.h +0 -0

index 8d3df55..ee6d18b 100644

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

Quoted 9 lines

include/linux/livepatch.h +2 -0

@@ -119,10 +121,12 @@ 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:	replace all funcs, reverting functions that are no longer patched
  * @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)
+ * @replaced:	the patch has been replaced an can not be re-enabled

I finally understand why you do this. I forgot that even disabled patches are still in klp_patch list.

This makes some sense for patches that fix different bugs and touch the same function. They should be applied and enabled in the right order because a later patch for the same function must be based on the code from the previous one.

It makes less sense for cummulative patches that we use in kGraft. There basically every patch has the "replace" flag set. If we enable one patch it simply replaces any other one. Then ordering is not important. Each patch is standalone and consistent on its own.

I see two problems with your approach. One is cosmetic. The names of the two flags replace/replaced are too similar. It is quite prone for mistakes ;-)

Quoted 8 lines

Second, we could not remove module where any function is patched usign the "immediate" flag. But people might want to revert to this patch if the last one does not work as expected. After all the main purpose of livepatching is to fix problems without a system reboot. Therefore we should allow to enable the replaced patches even without removing the module.

Quoted 5 lines

If the livepatch has the 'replace' bit set and not the 'immediate' bit, then I believe that we could remove *all* types of previous livepatches even if they have the 'immediate' flag set. That is, because of the consistency model, we are sure that we are running only functions from the cumulative replace patch.

Quoted 8 lines

You are partly right. The catch is if a function was previously patched using the immediate flag and the function is not longer patched by the new cumulative patch with the 'replace' flag. Then we need to create "no_op" for the function and the 'no_op' must inherit the immediate flag set. Then the consistency model does not guarantee that the code from the older patch is not running and we could not allow to remove the older patch.

Agreed - the replace patch should 'inherit' the immediate flag. This raises the question, what if say two patches have the immediate flag set differently for the same function? This would be unlikely since presumably we would set it the same way for all patches. In this case, I think a reasonable thing to do would be to 'inherit' the immediate flag from the immediately proceeding patch, since that's the one we are reverting from.

Quoted 48 lines

Quoted 35 lines

Quoted 35 lines

Quoted 11 lines

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

Quoted 8 lines

Quoted 4 lines

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "the enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

Thanks for pointing this out - it looks like the livepatch code uses wait_for_completion() with special kobj callbacks. Perhaps, there could be a nop callback, but I'd have to look at this more closely...

The completion is usable for the root kobject but you do not free it here. It might be pretty complicated if you need separate completion for all the freed kobjects.

Quoted 3 lines

A solution might be a global atomic counter and a waitqueue. Feel free to ask for more details.

So the issue is that the user might access some of the klp_* data structures via /sysfs after we have already freed them?

if so, this seems to be a common kernel pattern:

kobject_del(kobj);
kobject_put(kobj);

Quoted 72 lines

Quoted 47 lines

Quoted 42 lines

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

Quoted 8 lines

kernel/livepatch/core.c +8 -0

+		}
+		INIT_LIST_HEAD(&obj->func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kobject_put(&obj->kobj);
+		kfree(obj->name);
+		kfree(obj);

Same here.

Quoted 15 lines

kernel/livepatch/core.c +4 -0

+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+
 
 

kernel/livepatch/transition.c +3 -0

index b004a1f..d5e620e 100644
 
 
+			}
+		}
+		klp_unpatch_objects(klp_transition_patch, true);

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

Quoted 10 lines

kernel/livepatch/transition.c +3 -1

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

This code patch made me think about a situation when the "enable" operation was reverted during the transition. Then the target state is KLP_UNPATCHED and no_op stuff is there.

Quoted 4 lines

Well, in this case, we need to keep the no_op stuff until either the the patch is enabled egain and the transition finishes or we need to remove/free it klp_unregister_patch().

Quoted 4 lines

In the case that klp_cancel_transition() calls klp_complete_transition() and we remove the nop functions, we also remove all the regular 'functions', which should be re-added on the function stacks if the patch is re-enabled. So I think this is fine.

But we do not remove no_op when klp_target_state == KLP_PATCHED. And this might never happen when we call klp_reverse_transition() in the middle of the transition.

I see two solutions:

Either we could postpone generating the no_op functions until __klp_enable_patch() call and always free them in klp_finish_transition().

Or we need to check if some no_op functions are still there when __klp_unregister_patch() is called and free them there.

In the long term, we would need the first solution because it would allow to re-enable the replaced patches. But it will be more complicated to reuse the existing code, especially the init functions.

Quoted 3 lines

Feel free to keep it easy and implement
the 2nd possibility in this patch(set).

Indeed I took the 2nd possibility in this patch already - see the call to klp_patch_free_no_ops() in klp_unregister_patch().

Quoted 64 lines · Jason Baron

Quoted 52 lines · Petr Mladek

Quoted 45 lines · Jason Baron

Quoted 35 lines · Petr Mladek

Quoted 5 lines · Jason Baron

include/linux/livepatch.h +0 -0

index 8d3df55..ee6d18b 100644

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

Quoted 9 lines

include/linux/livepatch.h +2 -0

@@ -119,10 +121,12 @@ 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:	replace all funcs, reverting functions that are no longer patched
  * @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)
+ * @replaced:	the patch has been replaced an can not be re-enabled

I finally understand why you do this. I forgot that even disabled patches are still in klp_patch list.

This makes some sense for patches that fix different bugs and touch the same function. They should be applied and enabled in the right order because a later patch for the same function must be based on the code from the previous one.

It makes less sense for cummulative patches that we use in kGraft. There basically every patch has the "replace" flag set. If we enable one patch it simply replaces any other one. Then ordering is not important. Each patch is standalone and consistent on its own.

I see two problems with your approach. One is cosmetic. The names of the two flags replace/replaced are too similar. It is quite prone for mistakes ;-)

Quoted 8 lines

Second, we could not remove module where any function is patched usign the "immediate" flag. But people might want to revert to this patch if the last one does not work as expected. After all the main purpose of livepatching is to fix problems without a system reboot. Therefore we should allow to enable the replaced patches even without removing the module.

Quoted 5 lines

If the livepatch has the 'replace' bit set and not the 'immediate' bit, then I believe that we could remove *all* types of previous livepatches even if they have the 'immediate' flag set. That is, because of the consistency model, we are sure that we are running only functions from the cumulative replace patch.

Quoted 8 lines

You are partly right. The catch is if a function was previously patched using the immediate flag and the function is not longer patched by the new cumulative patch with the 'replace' flag. Then we need to create "no_op" for the function and the 'no_op' must inherit the immediate flag set. Then the consistency model does not guarantee that the code from the older patch is not running and we could not allow to remove the older patch.

Quoted 7 lines

Agreed - the replace patch should 'inherit' the immediate flag. This raises the question, what if say two patches have the immediate flag set differently for the same function? This would be unlikely since presumably we would set it the same way for all patches. In this case, I think a reasonable thing to do would be to 'inherit' the immediate flag from the immediately proceeding patch, since that's the one we are reverting from.

Good question. I would personally use immediate = false if there is a mismatch. It might block the transition but it will not break the consistency model. The consistency and stability is always more important. The user could always either revert the transition. Or there is going to be a way to force it.

IMHO, there are basically two approaches how to use the immediate flag:

Some people might enable it only when it is safe and needed to patch a function that might sleep for long. These people either want to be on the safe side. Or they want to remove disabled patches when possible. Your approach would be fine in this case.

Another group of people might always enable the immediate flag when it is safe. They would disable the immediate flag only when the patch does a semantic change. These people want to keep it easy and avoid potential troubles with a transition (complex code, might get blocked, ...). This is the reason to be conservative. The cumulative patch is going to replace all existing patches. If one of them needed the complex consistency model, we should use it as well.

Quoted 49 lines

Quoted 49 lines

Quoted 36 lines

Quoted 36 lines

Quoted 12 lines

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

Quoted 8 lines

Quoted 4 lines

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "the enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

Thanks for pointing this out - it looks like the livepatch code uses wait_for_completion() with special kobj callbacks. Perhaps, there could be a nop callback, but I'd have to look at this more closely...

The completion is usable for the root kobject but you do not free it here. It might be pretty complicated if you need separate completion for all the freed kobjects.

Quoted 6 lines

Quoted 3 lines

A solution might be a global atomic counter and a waitqueue. Feel free to ask for more details.

So the issue is that the user might access some of the klp_* data structures via /sysfs after we have already freed them?

yes

Quoted 4 lines

if so, this seems to be a common kernel pattern:

kobject_del(kobj);
kobject_put(kobj);

IMHO, this is used for other reason.

kobject_del() removes the object from the hierachy. Therefore it prevents new operations. But it does not wait for the exiting operations to finish. Therefore there still might be users that access the data even after this function finishes.

kobject_put() is enough if you do not mind how the object might be visible. It would remove it once the last reference on the object is removed. See the following code in kobject_cleanup.

	/* remove from sysfs if the caller did not do it */
	if (kobj->state_in_sysfs) {
		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
			 kobject_name(kobj), kobj);
		kobject_del(kobj);
	}

In each case, you must not free the data accessible from the kobject handlers until kobj_type->release method is called.

IMHO, the solution is not that complicated after all. If you are able to distinguish statically and dynamically defined klp_func and klp_obj structures, you might just modify the existing kobject release methods:

atomic_t klp_no_op_release_count;
static DECLARE_WAIT_QUEUE_HEAD(klp_no_op_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_no_op_release_count);
		wake_up(&klp_no_op_release_wait);
	}
}

static void klp_kobj_release_func(struct kobject *kobj)

{
	struct klp_func *func;

	obj = container_of(kobj, struct klp_func, kobj);
	/* Free dynamically allocated functions */
	if (!func->new_func) {
		kfree(func->old_name);
		kfree(func);
		atomic_dec(&klp_no_op_release_count);
		wake_up(&klp_no_op_release_wait);
	}
}

then we could have

void klp_patch_free_no_ops(struct klp_patch *patch)

{
	struct klp_object *obj, *tmp_obj;
	struct klp_func *func, *tmp_func;

	klp_for_each_object(patch, obj) {
		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
					 func_entry) {
			list_del_init(&func->func_entry);
			atomic_inc(&klp_no_op_release_count);
			kobject_put(&func->kobj);
		}
		INIT_LIST_HEAD(&obj->func_list);
	}
	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
		list_del_init(&obj->obj_entry);
		atomic_inc(&klp_no_op_release_count);
		kobject_put(&obj->kobj);
	}
	INIT_LIST_HEAD(&patch->obj_list);

	wait_event(&klp_no_op_release_wait,
		   atomic_read(&klp_no_op_release_count) == 0);
}

Add we should call this function also in klp_complete_transition() to avoid code duplication.

Quoted 72 lines

Quoted 72 lines

Quoted 47 lines

Quoted 42 lines

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

Quoted 8 lines

kernel/livepatch/core.c +8 -0

+		}
+		INIT_LIST_HEAD(&obj->func_list);
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
+		list_del_init(&obj->obj_entry);
+		kobject_put(&obj->kobj);
+		kfree(obj->name);
+		kfree(obj);

Same here.

Quoted 15 lines

kernel/livepatch/core.c +4 -0

+	}
+	INIT_LIST_HEAD(&patch->obj_list);
+}
+
 
 

kernel/livepatch/transition.c +3 -0

index b004a1f..d5e620e 100644
 
 
+			}
+		}
+		klp_unpatch_objects(klp_transition_patch, true);

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

Quoted 10 lines

kernel/livepatch/transition.c +3 -1

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

This code patch made me think about a situation when the "enable" operation was reverted during the transition. Then the target state is KLP_UNPATCHED and no_op stuff is there.

Quoted 4 lines

Well, in this case, we need to keep the no_op stuff until either the the patch is enabled egain and the transition finishes or we need to remove/free it klp_unregister_patch().

Quoted 4 lines

In the case that klp_cancel_transition() calls klp_complete_transition() and we remove the nop functions, we also remove all the regular 'functions', which should be re-added on the function stacks if the patch is re-enabled. So I think this is fine.

But we do not remove no_op when klp_target_state == KLP_PATCHED. And this might never happen when we call klp_reverse_transition() in the middle of the transition.

I see two solutions:

Either we could postpone generating the no_op functions until __klp_enable_patch() call and always free them in klp_finish_transition().

Or we need to check if some no_op functions are still there when __klp_unregister_patch() is called and free them there.

In the long term, we would need the first solution because it would allow to re-enable the replaced patches. But it will be more complicated to reuse the existing code, especially the init functions.

Quoted 6 lines

Quoted 3 lines

Feel free to keep it easy and implement
the 2nd possibility in this patch(set).

Indeed I took the 2nd possibility in this patch already - see the call to klp_patch_free_no_ops() in klp_unregister_patch().

Ah, I have missed this. Then we are on the safe side.

Best Regards,
Petr

Quoted 69 lines · Petr Mladek

Quoted 54 lines · Jason Baron

Quoted 51 lines · Petr Mladek

Quoted 37 lines · Jason Baron

Quoted 36 lines · Petr Mladek

Quoted 11 lines · Jason Baron

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

Quoted 8 lines

Quoted 4 lines

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "the enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

Thanks for pointing this out - it looks like the livepatch code uses wait_for_completion() with special kobj callbacks. Perhaps, there could be a nop callback, but I'd have to look at this more closely...

The completion is usable for the root kobject but you do not free it here. It might be pretty complicated if you need separate completion for all the freed kobjects.

Quoted 6 lines

Quoted 3 lines

A solution might be a global atomic counter and a waitqueue. Feel free to ask for more details.

So the issue is that the user might access some of the klp_* data structures via /sysfs after we have already freed them?

yes

Quoted 4 lines

if so, this seems to be a common kernel pattern:

kobject_del(kobj);
kobject_put(kobj);

Quoted 6 lines

IMHO, this is used for other reason.

kobject_del() removes the object from the hierachy. Therefore it prevents new operations. But it does not wait for the exiting operations to finish. Therefore there still might be users that access the data even after this function finishes.

I looked into this further - and it does appear to wait until all operations finish. In kernfs_drain() the code does:

wait_event(root->deactivate_waitq, atomic_read(&kn->active) == KN_DEACTIVATED_BIAS);

The call stack is:

kobject_del()
sysfs_remove_dir()
kernfs_remove()
__kernfs_remove()
kernfs_drain()

And __kernfs_remove() has already done a 'deactivate' prior:

/* prevent any new usage under @kn by deactivating all nodes */

pos = NULL;

while ((pos = kernfs_next_descendant_post(pos, kn)))

       if (kernfs_active(pos))

               atomic_add(KN_DEACTIVATED_BIAS, &pos->active);

So I *think* doing the kobject_del() first is sufficient. It may be worth some better documented if that is the case...

Quoted 101 lines · Jason Baron

Quoted 69 lines · Petr Mladek

Quoted 54 lines · Jason Baron

Quoted 51 lines · Petr Mladek

Quoted 37 lines · Jason Baron

Quoted 36 lines · Petr Mladek

Quoted 11 lines · Jason Baron

kernel/livepatch/core.c +2 -1

index 6004be3..21cecc5 100644
@@ -600,13 +603,38 @@ 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_patch_free_no_ops(struct klp_patch *patch)
+{

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

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			list_del_init(&func->func_entry);
+			kobject_put(&func->kobj);
+			kfree(func->old_name);
+			kfree(func);

kobject_put() is asynchronous. The structure should be freed using the kobject release method.

The question is how secure this should be. We might want to block other operations with the patch until all the release methods are called. But this might be tricky as there is not a single root kobject that would get destroyed at the end.

A crazy solution would be to define a global atomic counter. It might get increased with each kobject_put() call and descreased in each release method. Then we could wait until it gets zero.

Quoted 8 lines

Quoted 4 lines

It should be safe to wait with klp_mutex taken. Note that this is not possible with patch->kobj() where the "the enable" attribute takes the mutex as well, see enabled_store() in kernel/livepatch/core.c.

Thanks for pointing this out - it looks like the livepatch code uses wait_for_completion() with special kobj callbacks. Perhaps, there could be a nop callback, but I'd have to look at this more closely...

The completion is usable for the root kobject but you do not free it here. It might be pretty complicated if you need separate completion for all the freed kobjects.

Quoted 6 lines

Quoted 3 lines

A solution might be a global atomic counter and a waitqueue. Feel free to ask for more details.

So the issue is that the user might access some of the klp_* data structures via /sysfs after we have already freed them?

yes

Quoted 4 lines

if so, this seems to be a common kernel pattern:

kobject_del(kobj);
kobject_put(kobj);

Quoted 6 lines

IMHO, this is used for other reason.

kobject_del() removes the object from the hierachy. Therefore it prevents new operations. But it does not wait for the exiting operations to finish. Therefore there still might be users that access the data even after this function finishes.

I looked into this further - and it does appear to wait until all operations finish. In kernfs_drain() the code does:

wait_event(root->deactivate_waitq, atomic_read(&kn->active) == KN_DEACTIVATED_BIAS);

The call stack is:

kobject_del()
sysfs_remove_dir()
kernfs_remove()
__kernfs_remove()
kernfs_drain()

And __kernfs_remove() has already done a 'deactivate' prior:

/* prevent any new usage under @kn by deactivating all nodes */

pos = NULL;

while ((pos = kernfs_next_descendant_post(pos, kn)))

Quoted 7 lines

       if (kernfs_active(pos))

               atomic_add(KN_DEACTIVATED_BIAS, &pos->active);

So I *think* doing the kobject_del() first is sufficient. It may be worth some better documented if that is the case...

Sigh, I am confused. Your arguments look convincing. But I still feel a fear. As I said, we had had a long discussion about this long time ago. Unfortunately, I do not remember all the details.

In each case, there is the following mentioned in
Documentation/kobject.txt:

=== cut ===
Once you registered your kobject via kobject_add(), you must never use kfree() to free it directly. The only safe way is to use kobject_put(). It is good practice to always use kobject_put() after kobject_init() to avoid errors creeping in.

This notification is done through a kobject's release() method. Usually such a method has a form like::

    void my_object_release(struct kobject *kobj)
    {
            struct my_object *mine = container_of(kobj, struct my_object, kobj);

            /* Perform any additional cleanup on this object, then... */
            kfree(mine);
    }

One important point cannot be overstated: every kobject must have a release() method, and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met, the code is flawed. Note that the kernel will warn you if you forget to provide a release() method. Do not try to get rid of this warning by providing an "empty" release function; you will be mocked mercilessly by the kobject maintainer if you attempt this.

Note, the name of the kobject is available in the release function, but it must NOT be changed within this callback. Otherwise there will be a memory leak in the kobject core, which makes people unhappy.
=== cut ===

The fact is that if you enable CONFIG_DEBUG_KOBJECT_RELEASE, it will make a deferred access to the struct kobject by intention. See schedule_delayed_work() in kobject_release.

The struct kobject is part of both struct klp_func and struct klp_object. The delayed call will cause an access to an already freed memory if you free the structures immediately after calling kobject_put().

I am not sure why this is. Either we missed something and kernfs_drain() is not enough to avoid extra references of the kobject. Or kobject authors are over paranoid and push people to the only supported and "right design" a hard way.

Anyway, we either need to "fix" kobject implementation or we need to free the structures in the respective release callbacks.