[PATCH v8 0/8] livepatch: Atomic replace feature

[PATCH v8 0/8] livepatch: Atomic replace feature

[Petr Mladek]

The atomic replace allows to create cumulative patches. They
are useful when you maintain many livepatches and want to remove
one that is lower on the stack. In addition it is very useful when
more patches touch the same function and there are dependencies
between them.

I have found one bug in v7. We were not able to initialize NOP
struct klp_func when the patches module is not loaded. It was
because func->new_func was NULL. I have fixed it in separate patch
for an easier review.

Note that the original Jason's patch did not have this problem
because func->new_func was always NULL there. But this required
NOP-specific handling also on other locations, namely
klp_ftrace_handler() and klp_check_stack_func().

Changes against v7:

  + Fixed handling of NOPs for not-yet-loaded modules
  + Made klp_replaced_patches list static [Mirek]
  + Made klp_free_object() public later [Mirek]
  + Fixed several reported typos [Mirek, Joe]
  + Updated documentation according to the feedback [Joe]
  + Added some Acks [Mirek]

Changes against v6:

  + used list_move when disabling replaced patches [Jason]
  + renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek]
  + used klp_is_func_type() in klp_unpatch_object() [Mirek]
  + moved static definition of klp_get_or_add_object() [Mirek]
  + updated comment about synchronization in forced mode [Mirek]
  + added user documentation
  + fixed several typos

Jason Baron (5):
  livepatch: Use lists to manage patches, objects and functions
  livepatch: Initial support for dynamic structures
  livepatch: Allow to unpatch only functions of the given type
  livepatch: Support separate list for replaced patches.
  livepatch: Add atomic replace

Petr Mladek (3):
  livepatch: Free only structures with initialized kobject
  livepatch: Correctly handle atomic replace for not yet loaded modules
  livepatch: Atomic replace and cumulative patches documentation

 Documentation/livepatch/cumulative-patches.txt |  83 ++++++
 include/linux/livepatch.h                      |  59 +++-
 kernel/livepatch/core.c                        | 394 ++++++++++++++++++++++---
 kernel/livepatch/core.h                        |   4 +
 kernel/livepatch/patch.c                       |  31 +-
 kernel/livepatch/patch.h                       |   4 +-
 kernel/livepatch/transition.c                  |  41 ++-
 7 files changed, 566 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/livepatch/cumulative-patches.txt

-- 
2.13.6

[PATCH v8 1/8] livepatch: Use lists to manage patches, objects and functions

[Petr Mladek]

From: Jason Baron <jbaron@akamai.com>

Currently klp_patch contains a pointer to a statically allocated array of
struct klp_object and struct klp_objects contains a pointer to a statically
allocated array of klp_func. In order to allow for the dynamic allocation
of objects and functions, link klp_patch, klp_object, and klp_func together
via linked lists. This allows us to more easily allocate new objects and
functions, while having the iterator be a simple linked list walk.

The static structures are added to the lists early. It allows to add
the dynamically allocated objects before klp_init_object() and
klp_init_func() calls. Therefore it reduces the further changes
to the code.

Also klp_init_*_list() functions are split because they will
be used when adding the dynamically allocated structures.

This patch does not change the existing behavior.

Signed-off-by: Jason Baron <jbaron@akamai.com>
[pmladek@suse.com: Initialize lists before init calls]
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Acked-by: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h | 19 +++++++++++++++++--
 kernel/livepatch/core.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 4754f01c1abb..e5db2ba7e2a5 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/ftrace.h>
 #include <linux/completion.h>
+#include <linux/list.h>
 
 #if IS_ENABLED(CONFIG_LIVEPATCH)
 
@@ -43,6 +44,7 @@
  * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
  * @stack_node:	list node for klp_ops func_stack list
+ * @func_entry:	links struct klp_func to struct klp_object
  * @old_size:	size of the old function
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
@@ -80,6 +82,7 @@ struct klp_func {
 	unsigned long old_addr;
 	struct kobject kobj;
 	struct list_head stack_node;
+	struct list_head func_entry;
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
@@ -117,6 +120,8 @@ struct klp_callbacks {
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
+ * @func_list:	head of list for struct klp_func
+ * @obj_entry:	links struct klp_object to struct klp_patch
  * @patched:	the object's funcs have been added to the klp_ops list
  */
 struct klp_object {
@@ -127,6 +132,8 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head func_list;
+	struct list_head obj_entry;
 	struct module *mod;
 	bool patched;
 };
@@ -137,6 +144,7 @@ struct klp_object {
  * @objs:	object entries for kernel objects to be patched
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
+ * @obj_list:	head of list for struct klp_object
  * @enabled:	the patch is enabled (but operation may be incomplete)
  * @finish:	for waiting till it is safe to remove the patch module
  */
@@ -148,18 +156,25 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
-#define klp_for_each_object(patch, obj) \
+#define klp_for_each_object_static(patch, obj) \
 	for (obj = patch->objs; obj->funcs || obj->name; obj++)
 
-#define klp_for_each_func(obj, func) \
+#define klp_for_each_object(patch, obj)	\
+	list_for_each_entry(obj, &patch->obj_list, obj_entry)
+
+#define klp_for_each_func_static(obj, func) \
 	for (func = obj->funcs; \
 	     func->old_name || func->new_func || func->old_sympos; \
 	     func++)
 
+#define klp_for_each_func(obj, func)	\
+	list_for_each_entry(func, &obj->func_list, func_entry)
+
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3a4656fb7047..1d525f4a270a 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,6 +49,32 @@ static LIST_HEAD(klp_patches);
 
 static struct kobject *klp_root_kobj;
 
+static void klp_init_func_list(struct klp_object *obj, struct klp_func *func)
+{
+	list_add(&func->func_entry, &obj->func_list);
+}
+
+static void klp_init_object_list(struct klp_patch *patch,
+				 struct klp_object *obj)
+{
+	struct klp_func *func;
+
+	list_add(&obj->obj_entry, &patch->obj_list);
+
+	INIT_LIST_HEAD(&obj->func_list);
+	klp_for_each_func_static(obj, func)
+		klp_init_func_list(obj, func);
+}
+
+static void klp_init_patch_list(struct klp_patch *patch)
+{
+	struct klp_object *obj;
+
+	INIT_LIST_HEAD(&patch->obj_list);
+	klp_for_each_object_static(patch, obj)
+		klp_init_object_list(patch, obj);
+}
+
 static bool klp_is_module(struct klp_object *obj)
 {
 	return obj->name;
@@ -794,6 +820,7 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	patch->enabled = false;
 	init_completion(&patch->finish);
+	klp_init_patch_list(patch);
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
 				   klp_root_kobj, "%s", patch->mod->name);
-- 
2.13.6

[PATCH v8 2/8] livepatch: Free only structures with initialized kobject

[Petr Mladek]

We are going to add a feature called atomic replace. It will allow to
create a patch that would replace all already registered patches.
For this, we will need to dynamically create funcs and objects
for functions that are no longer patched.

We will want to reuse the existing init() and free() functions. Up to now,
the free() functions checked a limit and were called only for structures
with initialized kobject. But we will want to call them also for structures
that were allocated but where the kobject was not initialized yet.

This patch removes the limit. It calls klp_free*() functions for all
structures. But only the ones with initialized kobject are freed.
The handling of un-initialized structures will be added later with
the support for dynamic structures.

This patch does not change the existing behavior.

Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Jason Baron <jbaron@akamai.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/core.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 1d525f4a270a..69bde95e76f8 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -653,17 +653,15 @@ static struct kobj_type klp_ktype_func = {
 	.sysfs_ops = &kobj_sysfs_ops,
 };
 
-/*
- * Free all functions' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
- */
-static void klp_free_funcs_limited(struct klp_object *obj,
-				   struct klp_func *limit)
+/* Free all funcs that have the kobject initialized. */
+static void klp_free_funcs(struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	for (func = obj->funcs; func->old_name && func != limit; func++)
-		kobject_put(&func->kobj);
+	klp_for_each_func(obj, func) {
+		if (func->kobj.state_initialized)
+			kobject_put(&func->kobj);
+	}
 }
 
 /* Clean up when a patched object is unloaded */
@@ -677,24 +675,23 @@ static void klp_free_object_loaded(struct klp_object *obj)
 		func->old_addr = 0;
 }
 
-/*
- * Free all objects' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
- */
-static void klp_free_objects_limited(struct klp_patch *patch,
-				     struct klp_object *limit)
+/* Free all funcs and objects that have the kobject initialized. */
+static void klp_free_objects(struct klp_patch *patch)
 {
 	struct klp_object *obj;
 
-	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
-		klp_free_funcs_limited(obj, NULL);
-		kobject_put(&obj->kobj);
+	klp_for_each_object(patch, obj) {
+		klp_free_funcs(obj);
+
+		if (obj->kobj.state_initialized)
+			kobject_put(&obj->kobj);
 	}
 }
 
 static void klp_free_patch(struct klp_patch *patch)
 {
-	klp_free_objects_limited(patch, NULL);
+	klp_free_objects(patch);
+
 	if (!list_empty(&patch->list))
 		list_del(&patch->list);
 }
@@ -791,21 +788,16 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_init_func(obj, func);
 		if (ret)
-			goto free;
+			return ret;
 	}
 
 	if (klp_is_object_loaded(obj)) {
 		ret = klp_init_object_loaded(patch, obj);
 		if (ret)
-			goto free;
+			return ret;
 	}
 
 	return 0;
-
-free:
-	klp_free_funcs_limited(obj, func);
-	kobject_put(&obj->kobj);
-	return ret;
 }
 
 static int klp_init_patch(struct klp_patch *patch)
@@ -842,7 +834,7 @@ static int klp_init_patch(struct klp_patch *patch)
 	return 0;
 
 free:
-	klp_free_objects_limited(patch, obj);
+	klp_free_objects(patch);
 
 	mutex_unlock(&klp_mutex);
 
-- 
2.13.6

[PATCH v8 3/8] livepatch: Initial support for dynamic structures

[Petr Mladek]

From: Jason Baron <jbaron@akamai.com>

We are going to add a feature called atomic replace. It will allow to
create a patch that would replace all already registered patches.
For this, we will need to dynamically create funcs and objects
for functions that are no longer patched.

This patch adds basic framework to handle such dynamic structures.

It adds enum klp_func_type that allows to distinguish the dynamically
allocated funcs' structures. Note that objects' structures do not have
a clear type. Namely the static objects' structures might list both static
and dynamic funcs' structures.

The function type is then used to limit klp_free() functions. We will
want to free the dynamic structures separately when they are no longer
needed. At the same time, we also want to make our life easier,
and introduce _any_ type that will allow to process all existing
structures in one go.

We need to be careful here. First, objects' structures must be freed
only when all listed funcs' structures are freed. Also we must avoid
double free. Both problems are solved by removing the freed structures
from the list.

Also note that klp_free*() functions are called also in klp_init_patch()
error path when only some kobjects have been initialized. The other
dynamic structures must be freed immediately by calling the respective
klp_free_*_dynamic() functions.

Finally, the dynamic objects' structures are generic. The respective
klp_allocate_object_dynamic() and klp_free_object_dynamic() can
be implemented here. On the other hand, klp_free_func_dynamic()
is empty. It must be updated when a particular dynamic
klp_func_type is introduced.

Signed-off-by: Jason Baron <jbaron@akamai.com>
[pmladek@suse.com: Converted into a generic API]
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Acked-by: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h |  37 +++++++++++-
 kernel/livepatch/core.c   | 141 +++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 163 insertions(+), 15 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e5db2ba7e2a5..7fb76e7d2693 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -35,12 +35,22 @@
 #define KLP_UNPATCHED	 0
 #define KLP_PATCHED	 1
 
+/*
+ * Function type is used to distinguish dynamically allocated structures
+ * and limit some operations.
+ */
+enum klp_func_type {
+	KLP_FUNC_ANY = -1,	/* Substitute any type */
+	KLP_FUNC_STATIC = 0,    /* Original statically defined structure */
+};
+
 /**
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
  * @new_func:	pointer to the patched function code
  * @old_sympos: a hint indicating which symbol position the old function
  *		can be found (optional)
+ * @ftype:	distinguish static and dynamic structures
  * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
  * @stack_node:	list node for klp_ops func_stack list
@@ -79,6 +89,7 @@ struct klp_func {
 	unsigned long old_sympos;
 
 	/* internal */
+	enum klp_func_type ftype;
 	unsigned long old_addr;
 	struct kobject kobj;
 	struct list_head stack_node;
@@ -164,17 +175,41 @@ struct klp_patch {
 #define klp_for_each_object_static(patch, obj) \
 	for (obj = patch->objs; obj->funcs || obj->name; obj++)
 
+#define klp_for_each_object_safe(patch, obj, tmp_obj)		\
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry)
+
 #define klp_for_each_object(patch, obj)	\
 	list_for_each_entry(obj, &patch->obj_list, obj_entry)
 
+/* Support also dynamically allocated struct klp_object */
 #define klp_for_each_func_static(obj, func) \
 	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
+	     func && (func->old_name || func->new_func || func->old_sympos); \
 	     func++)
 
+#define klp_for_each_func_safe(obj, func, tmp_func)			\
+	list_for_each_entry_safe(func, tmp_func, &obj->func_list, func_entry)
+
 #define klp_for_each_func(obj, func)	\
 	list_for_each_entry(func, &obj->func_list, func_entry)
 
+static inline bool klp_is_object_dynamic(struct klp_object *obj)
+{
+	return !obj->funcs;
+}
+
+static inline bool klp_is_func_dynamic(struct klp_func *func)
+{
+	WARN_ON_ONCE(func->ftype == KLP_FUNC_ANY);
+	return func->ftype != KLP_FUNC_STATIC;
+}
+
+static inline bool klp_is_func_type(struct klp_func *func,
+				    enum klp_func_type ftype)
+{
+	return ftype == KLP_FUNC_ANY || ftype == func->ftype;
+}
+
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 69bde95e76f8..a47c26147c17 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -124,6 +124,26 @@ static bool klp_initialized(void)
 	return !!klp_root_kobj;
 }
 
+static struct klp_object *klp_find_object(struct klp_patch *patch,
+					  struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	bool mod = klp_is_module(old_obj);
+
+	klp_for_each_object(patch, obj) {
+		if (mod) {
+			if (klp_is_module(obj) &&
+			    strcmp(old_obj->name, obj->name) == 0) {
+				return obj;
+			}
+		} else if (!klp_is_module(obj)) {
+			return obj;
+		}
+	}
+
+	return NULL;
+}
+
 struct klp_find_arg {
 	const char *objname;
 	const char *name;
@@ -621,6 +641,66 @@ static struct attribute *klp_patch_attrs[] = {
 	NULL
 };
 
+/*
+ * Dynamically allocated objects and functions.
+ */
+static void klp_free_func_dynamic(struct klp_func *func)
+{
+}
+
+static void klp_free_object_dynamic(struct klp_object *obj)
+{
+	kfree(obj->name);
+	kfree(obj);
+}
+
+static struct klp_object *klp_alloc_object_dynamic(const char *name)
+{
+	struct klp_object *obj;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	if (name) {
+		obj->name = kstrdup(name, GFP_KERNEL);
+		if (!obj->name) {
+			kfree(obj);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	return obj;
+}
+
+static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
+						struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+
+	obj = klp_find_object(patch, old_obj);
+	if (obj)
+		return obj;
+
+	obj = klp_alloc_object_dynamic(old_obj->name);
+	if (IS_ERR(obj))
+		return obj;
+
+	klp_init_object_list(patch, obj);
+	return obj;
+}
+
+/*
+ * Patch release framework must support the following scenarios:
+ *
+ *   + Asynchonous release is used when kobjects are initialized.
+ *
+ *   + Direct release is used in error paths for structures that
+ *     have not had kobj initialized yet.
+ *
+ *   + Allow to release dynamic structures of the given type when
+ *     they are not longer needed.
+ */
 static void klp_kobj_release_patch(struct kobject *kobj)
 {
 	struct klp_patch *patch;
@@ -637,6 +717,12 @@ static struct kobj_type klp_ktype_patch = {
 
 static void klp_kobj_release_object(struct kobject *kobj)
 {
+	struct klp_object *obj;
+
+	obj = container_of(kobj, struct klp_object, kobj);
+
+	if (klp_is_object_dynamic(obj))
+		klp_free_object_dynamic(obj);
 }
 
 static struct kobj_type klp_ktype_object = {
@@ -646,6 +732,12 @@ static struct kobj_type klp_ktype_object = {
 
 static void klp_kobj_release_func(struct kobject *kobj)
 {
+	struct klp_func *func;
+
+	func = container_of(kobj, struct klp_func, kobj);
+
+	if (klp_is_func_dynamic(func))
+		klp_free_func_dynamic(func);
 }
 
 static struct kobj_type klp_ktype_func = {
@@ -653,14 +745,26 @@ static struct kobj_type klp_ktype_func = {
 	.sysfs_ops = &kobj_sysfs_ops,
 };
 
-/* Free all funcs that have the kobject initialized. */
-static void klp_free_funcs(struct klp_object *obj)
+/*
+ * Free all funcs of the given ftype. Use the kobject when it has already
+ * been initialized. Otherwise, do it directly.
+ */
+static void klp_free_funcs(struct klp_object *obj,
+			   enum klp_func_type ftype)
 {
-	struct klp_func *func;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_func_safe(obj, func, tmp_func) {
+		if (!klp_is_func_type(func, ftype))
+			continue;
+
+		/* Avoid double free and allow to detect empty objects. */
+		list_del(&func->func_entry);
 
-	klp_for_each_func(obj, func) {
 		if (func->kobj.state_initialized)
 			kobject_put(&func->kobj);
+		else if (klp_is_func_dynamic(func))
+			klp_free_func_dynamic(func);
 	}
 }
 
@@ -675,22 +779,34 @@ static void klp_free_object_loaded(struct klp_object *obj)
 		func->old_addr = 0;
 }
 
-/* Free all funcs and objects that have the kobject initialized. */
-static void klp_free_objects(struct klp_patch *patch)
+/*
+ * Free all linked funcs of the given ftype. Then free empty objects.
+ * Use the kobject when it has already been initialized. Otherwise,
+ * do it directly.
+ */
+static void klp_free_objects(struct klp_patch *patch, enum klp_func_type ftype)
 {
-	struct klp_object *obj;
+	struct klp_object *obj, *tmp_obj;
 
-	klp_for_each_object(patch, obj) {
-		klp_free_funcs(obj);
+	klp_for_each_object_safe(patch, obj, tmp_obj) {
+		klp_free_funcs(obj, ftype);
+
+		if (!list_empty(&obj->func_list))
+			continue;
+
+		/* Avoid freeing the object twice. */
+		list_del(&obj->obj_entry);
 
 		if (obj->kobj.state_initialized)
 			kobject_put(&obj->kobj);
+		else if (klp_is_object_dynamic(obj))
+			klp_free_object_dynamic(obj);
 	}
 }
 
 static void klp_free_patch(struct klp_patch *patch)
 {
-	klp_free_objects(patch);
+	klp_free_objects(patch, KLP_FUNC_ANY);
 
 	if (!list_empty(&patch->list))
 		list_del(&patch->list);
@@ -771,9 +887,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	int ret;
 	const char *name;
 
-	if (!obj->funcs)
-		return -EINVAL;
-
 	obj->patched = false;
 	obj->mod = NULL;
 
@@ -834,7 +947,7 @@ static int klp_init_patch(struct klp_patch *patch)
 	return 0;
 
 free:
-	klp_free_objects(patch);
+	klp_free_objects(patch, KLP_FUNC_ANY);
 
 	mutex_unlock(&klp_mutex);
 
-- 
2.13.6

[PATCH v8 4/8] livepatch: Allow to unpatch only functions of the given type

[Petr Mladek]

From: Jason Baron <jbaron@akamai.com>

We are going to add a feature called atomic replace. It will allow to
create a patch that would replace all already registered patches.
For this, we will need to dynamically create funcs and objects
for functions that are no longer patched.

The dynamically allocated objects will not longer be needed once
the patch is applied.

This patch allows to unpatch functions of the given type. It might
cause that the obj->patched flag is true even when some listed
functions are not longer patched. This is fine as long as the
unpatched funcs' structures are removed right after. It will
be the case. Anyway, it is safe. In the worst case, it will
not be possible to enable the disabled functions.

Signed-off-by: Jason Baron <jbaron@akamai.com>
[pmladek@suse.com: Split and modified to use the generic ftype]
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/core.c       |  2 +-
 kernel/livepatch/patch.c      | 26 +++++++++++++++++++-------
 kernel/livepatch/patch.h      |  4 ++--
 kernel/livepatch/transition.c |  2 +-
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a47c26147c17..ab1f6a371fc8 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1060,7 +1060,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 
 				pr_notice("reverting patch '%s' on unloading module '%s'\n",
 					  patch->mod->name, obj->mod->name);
-				klp_unpatch_object(obj);
+				klp_unpatch_object(obj, KLP_FUNC_ANY);
 
 				klp_post_unpatch_callback(obj);
 			}
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 82d584225dc6..54b3c379bb0f 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -236,15 +236,27 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+/*
+ * It keeps obj->patched flag true when any listed function is still patched.
+ * The caller is responsible for removing the unpatched functions to
+ * make the flag clean again.
+ */
+void klp_unpatch_object(struct klp_object *obj, enum klp_func_type ftype)
 {
 	struct klp_func *func;
+	bool patched = false;
 
-	klp_for_each_func(obj, func)
-		if (func->patched)
+	klp_for_each_func(obj, func) {
+		if (!func->patched)
+			continue;
+
+		if (klp_is_func_type(func, ftype))
 			klp_unpatch_func(func);
+		else
+			patched = true;
+	}
 
-	obj->patched = false;
+	obj->patched = patched;
 }
 
 int klp_patch_object(struct klp_object *obj)
@@ -258,7 +270,7 @@ int klp_patch_object(struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, KLP_FUNC_ANY);
 			return ret;
 		}
 	}
@@ -267,11 +279,11 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+void klp_unpatch_objects(struct klp_patch *patch, enum klp_func_type ftype)
 {
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, ftype);
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index e72d8250d04b..885f644add4c 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -28,7 +28,7 @@ struct klp_ops {
 struct klp_ops *klp_find_ops(unsigned long old_addr);
 
 int klp_patch_object(struct klp_object *obj);
-void klp_unpatch_object(struct klp_object *obj);
-void klp_unpatch_objects(struct klp_patch *patch);
+void klp_unpatch_object(struct klp_object *obj, enum klp_func_type ftype);
+void klp_unpatch_objects(struct klp_patch *patch, enum klp_func_type ftype);
 
 #endif /* _LIVEPATCH_PATCH_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7c6631e693bc..6917100fbe79 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -92,7 +92,7 @@ static void klp_complete_transition(void)
 		 * 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, KLP_FUNC_ANY);
 
 		/*
 		 * Make sure klp_ftrace_handler() can no longer see functions
-- 
2.13.6

Re: [PATCH v8 4/8] livepatch: Allow to unpatch only functions of the given type

[Miroslav Benes]

On Wed, 21 Feb 2018, Petr Mladek wrote:

> From: Jason Baron <jbaron@akamai.com>
> 
> We are going to add a feature called atomic replace. It will allow to
> create a patch that would replace all already registered patches.
> For this, we will need to dynamically create funcs and objects
> for functions that are no longer patched.
> 
> The dynamically allocated objects will not longer be needed once
> the patch is applied.
> 
> This patch allows to unpatch functions of the given type. It might
> cause that the obj->patched flag is true even when some listed
> functions are not longer patched. This is fine as long as the
> unpatched funcs' structures are removed right after. It will
> be the case. Anyway, it is safe. In the worst case, it will
> not be possible to enable the disabled functions.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> [pmladek@suse.com: Split and modified to use the generic ftype]
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Miroslav Benes <mbenes@suse.cz>

Acked-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

[PATCH v8 5/8] livepatch: Support separate list for replaced patches.

[Petr Mladek]

From: Jason Baron <jbaron@akamai.com>

We are going to add a feature called atomic replace. It will allow to
create a patch that would replace all already registered patches.

The replaced patches will stay registered because they are typically
unregistered by some package uninstall scripts. But we will remove
these patches from @klp_patches list to keep the enabled patch
on the bottom of the stack. Otherwise, we would need to implement
rather complex logic for moving the patches on the stack. Also
it would complicate implementation of the atomic replace feature.
It is not worth it.

As a result, we will have patches that are registered but that
are no longer usable. Let's get prepared for this and use
a better descriptive name for klp_is_patch_registered() function.

Also create separate list for the replaced patches and allow to
unregister them. Alternative solution would be to add a flag
into struct klp_patch. Note that patch->kobj.state_initialized
is not safe because it can be cleared outside klp_mutex.

This patch does not change the existing behavior.

Signed-off-by: Jason Baron <jbaron@akamai.com>
[pmladek@suse.com: Split and renamed klp_is_patch_usable()]
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Acked-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/core.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ab1f6a371fc8..fd0296859ff4 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -47,6 +47,13 @@ DEFINE_MUTEX(klp_mutex);
 
 static LIST_HEAD(klp_patches);
 
+/*
+ * List of 'replaced' patches that have been replaced by a patch that has the
+ * 'replace' bit set. When they are added to this list, they are disabled and
+ * can not be re-enabled, but they can be unregistered().
+ */
+static LIST_HEAD(klp_replaced_patches);
+
 static struct kobject *klp_root_kobj;
 
 static void klp_init_func_list(struct klp_object *obj, struct klp_func *func)
@@ -108,17 +115,28 @@ static void klp_find_object_module(struct klp_object *obj)
 	mutex_unlock(&module_mutex);
 }
 
-static bool klp_is_patch_registered(struct klp_patch *patch)
+static bool klp_is_patch_in_list(struct klp_patch *patch,
+				 struct list_head *head)
 {
 	struct klp_patch *mypatch;
 
-	list_for_each_entry(mypatch, &klp_patches, list)
+	list_for_each_entry(mypatch, head, list)
 		if (mypatch == patch)
 			return true;
 
 	return false;
 }
 
+static bool klp_is_patch_usable(struct klp_patch *patch)
+{
+	return klp_is_patch_in_list(patch, &klp_patches);
+}
+
+static bool klp_is_patch_replaced(struct klp_patch *patch)
+{
+	return klp_is_patch_in_list(patch, &klp_replaced_patches);
+}
+
 static bool klp_initialized(void)
 {
 	return !!klp_root_kobj;
@@ -375,7 +393,7 @@ int klp_disable_patch(struct klp_patch *patch)
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_usable(patch)) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -475,7 +493,7 @@ int klp_enable_patch(struct klp_patch *patch)
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_usable(patch)) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -516,7 +534,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_usable(patch)) {
 		/*
 		 * Module with the patch could either disappear meanwhile or is
 		 * not properly initialized yet.
@@ -971,7 +989,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_usable(patch) && !klp_is_patch_replaced(patch)) {
 		ret = -EINVAL;
 		goto err;
 	}
-- 
2.13.6

[PATCH v8 6/8] livepatch: Add atomic replace

[Petr Mladek]

From: Jason Baron <jbaron@akamai.com>

Sometimes we would like to revert a particular fix. Currently, this
is not easy because we want to keep all other fixes active and we
could revert only the last applied patch.

One solution would be to apply new patch that implemented all
the reverted functions like in the original code. It would work
as expected but there will be unnecessary redirections. In addition,
it would also require knowing which functions need to be reverted at
build time.

Another problem is when there are many patches that touch the same
functions. There might be dependencies between patches that are
not enforced on the kernel side. Also it might be pretty hard to
actually prepare the patch and ensure compatibility with
the other patches.

A better solution would be to create cumulative patch and say that
it replaces all older ones.

This patch adds a new "replace" flag to struct klp_patch. When it is
enabled, a set of 'nop' klp_func will be dynamically created for all
functions that are already being patched but that will no longer be
modified by the new patch. They are temporarily used as a new target
during the patch transition.

There are used several simplifications:

  + nops' structures are generated already when the patch is registered.
    All registered patches are taken into account, even the disabled ones.
    As a result, there might be more nops than are really needed when
    the patch is enabled and some disabled patches were removed before.
    But we are on the safe side and it simplifies the implementation.
    Especially we could reuse the existing init() functions. Also freeing
    is easier because the same nops are created and removed only once.

    Alternative solution would be to create nops when the patch is enabled.
    But then any reusing of the init() functions and error paths would be
    complicated. Also it would increase the risk of errors because of
    late kobject initialization. Finally, it would need tricky waiting
    for freed kobjects when finalizing a reverted enable transaction.

  + The replaced patches are removed from the stack and cannot longer
    be enabled directly. Otherwise, we would need to implement a more
    complex logic of handling the stack of patches. It might be hard
    to come with a reasonable semantic.

    A fallback is to remove (rmmod) the replaced patches and register
    (insmod) them again.

  + Nops are handled like normal function patches. It reduces changes
    in the existing code.

    It would be possible to copy internal values when they are allocated
    and make short cuts in init() functions. It would be possible to use
    the fact that old_func and new_func point to the same function and
    do not init new_func and new_size at all. It would be possible to
    detect nop func in ftrace handler and just leave. But all these would
    just complicate the code and maintenance.

  + The callbacks from the replaced patches are not called. It would be
    pretty hard to define a reasonable semantic and implement it.

    It might even be counter-productive. The new patch is cumulative.
    It is supposed to include most of the changes from older patches.
    In most cases, it will not want to call pre_unpatch() post_unpatch()
    callbacks from the replaced patches. It would disable/break things
    for no good reasons. Also it should be easier to handle various
    scenarios in a single script in the new patch than think about
    interactions caused by running many scripts from older patches.
    No to say that the old scripts even would not expect to be called
    in this situation.

Signed-off-by: Jason Baron <jbaron@akamai.com>
[pmladek@suse.com: Split, reuse existing code, simplified]
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Acked-by: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h     |   3 +
 kernel/livepatch/core.c       | 162 +++++++++++++++++++++++++++++++++++++++++-
 kernel/livepatch/core.h       |   4 ++
 kernel/livepatch/transition.c |  39 ++++++++++
 4 files changed, 206 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 7fb76e7d2693..ed598d849029 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -42,6 +42,7 @@
 enum klp_func_type {
 	KLP_FUNC_ANY = -1,	/* Substitute any type */
 	KLP_FUNC_STATIC = 0,    /* Original statically defined structure */
+	KLP_FUNC_NOP,		/* Dynamically allocated NOP function patch */
 };
 
 /**
@@ -153,6 +154,7 @@ struct klp_object {
  * struct klp_patch - patch structure for live patching
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
+ * @replace:	replace all already registered patches
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
  * @obj_list:	head of list for struct klp_object
@@ -163,6 +165,7 @@ struct klp_patch {
 	/* external */
 	struct module *mod;
 	struct klp_object *objs;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index fd0296859ff4..ad508a86b2f9 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -142,6 +142,21 @@ static bool klp_initialized(void)
 	return !!klp_root_kobj;
 }
 
+static struct klp_func *klp_find_func(struct klp_object *obj,
+				      struct klp_func *old_func)
+{
+	struct klp_func *func;
+
+	klp_for_each_func(obj, func) {
+		if ((strcmp(old_func->old_name, func->old_name) == 0) &&
+		    (old_func->old_sympos == func->old_sympos)) {
+			return func;
+		}
+	}
+
+	return NULL;
+}
+
 static struct klp_object *klp_find_object(struct klp_patch *patch,
 					  struct klp_object *old_obj)
 {
@@ -342,6 +357,39 @@ static int klp_write_object_relocations(struct module *pmod,
 	return ret;
 }
 
+/*
+ * This function removes replaced patches from both func_stack
+ * and klp_patches stack.
+ *
+ * We could be pretty aggressive here. It is called in situation
+ * when these structures are no longer accessible. All functions
+ * are redirected using the klp_transition_patch. They use either
+ * a new code or they are in the original code because of the special
+ * nop function patches.
+ */
+void klp_throw_away_replaced_patches(struct klp_patch *new_patch,
+				     bool keep_module)
+{
+	struct klp_patch *old_patch, *tmp_patch;
+
+	list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) {
+		if (old_patch == new_patch)
+			return;
+
+		klp_unpatch_objects(old_patch, KLP_FUNC_ANY);
+		old_patch->enabled = false;
+
+		/*
+		 * Replaced patches could not get re-enabled to keep
+		 * the code sane.
+		 */
+		list_move(&old_patch->list, &klp_replaced_patches);
+
+		if (!keep_module)
+			module_put(old_patch->mod);
+	}
+}
+
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -537,7 +585,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (!klp_is_patch_usable(patch)) {
 		/*
 		 * Module with the patch could either disappear meanwhile or is
-		 * not properly initialized yet.
+		 * not properly initialized yet or the patch was just replaced.
 		 */
 		ret = -EINVAL;
 		goto err;
@@ -662,8 +710,16 @@ static struct attribute *klp_patch_attrs[] = {
 /*
  * Dynamically allocated objects and functions.
  */
+static void klp_free_func_nop(struct klp_func *func)
+{
+	kfree(func->old_name);
+	kfree(func);
+}
+
 static void klp_free_func_dynamic(struct klp_func *func)
 {
+	if (func->ftype == KLP_FUNC_NOP)
+		klp_free_func_nop(func);
 }
 
 static void klp_free_object_dynamic(struct klp_object *obj)
@@ -708,6 +764,102 @@ static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
 	return obj;
 }
 
+static struct klp_func *klp_alloc_func_nop(struct klp_func *old_func,
+					   struct klp_object *obj)
+{
+	struct klp_func *func;
+
+	func = kzalloc(sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return ERR_PTR(-ENOMEM);
+
+	if (old_func->old_name) {
+		func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
+		if (!func->old_name) {
+			kfree(func);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+	func->old_sympos = old_func->old_sympos;
+	/* NOP func is the same as using the original implementation. */
+	func->new_func = (void *)old_func->old_addr;
+	func->ftype = KLP_FUNC_NOP;
+
+	return func;
+}
+
+static int klp_add_func_nop(struct klp_object *obj,
+			    struct klp_func *old_func)
+{
+	struct klp_func *func;
+
+	func = klp_find_func(obj, old_func);
+
+	if (func)
+		return 0;
+
+	func = klp_alloc_func_nop(old_func, obj);
+	if (IS_ERR(func))
+		return PTR_ERR(func);
+
+	klp_init_func_list(obj, func);
+
+	return 0;
+}
+
+static int klp_add_object_nops(struct klp_patch *patch,
+			       struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	struct klp_func *old_func;
+	int err = 0;
+
+	obj = klp_get_or_add_object(patch, old_obj);
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
+
+	klp_for_each_func(old_obj, old_func) {
+		err = klp_add_func_nop(obj, old_func);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/*
+ * Add 'nop' functions which simply return to the caller to run
+ * the original function. The 'nop' functions are added to a
+ * patch to facilitate a 'replace' mode
+ *
+ * The nops are generated for all patches on the stack when
+ * the new patch is initialized. It is safe even though some
+ * older patches might get disabled and removed before the
+ * new one is enabled. In the worst case, there might be nops
+ * which will not be really needed. But it does not harm and
+ * simplifies the implementation a lot. Especially we could
+ * use the init functions as is.
+ */
+static int klp_add_nops(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+	struct klp_object *old_obj;
+	int err = 0;
+
+	if (WARN_ON(!patch->replace))
+		return -EINVAL;
+
+	list_for_each_entry(old_patch, &klp_patches, list) {
+		klp_for_each_object(old_patch, old_obj) {
+			err = klp_add_object_nops(patch, old_obj);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Patch release framework must support the following scenarios:
  *
@@ -802,7 +954,7 @@ static void klp_free_object_loaded(struct klp_object *obj)
  * Use the kobject when it has already been initialized. Otherwise,
  * do it directly.
  */
-static void klp_free_objects(struct klp_patch *patch, enum klp_func_type ftype)
+void klp_free_objects(struct klp_patch *patch, enum klp_func_type ftype)
 {
 	struct klp_object *obj, *tmp_obj;
 
@@ -952,6 +1104,12 @@ static int klp_init_patch(struct klp_patch *patch)
 		return ret;
 	}
 
+	if (patch->replace) {
+		ret = klp_add_nops(patch);
+		if (ret)
+			goto free;
+	}
+
 	klp_for_each_object(patch, obj) {
 		ret = klp_init_object(patch, obj);
 		if (ret)
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 48a83d4364cf..43184a5318d8 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -6,6 +6,10 @@
 
 extern struct mutex klp_mutex;
 
+void klp_throw_away_replaced_patches(struct klp_patch *new_patch,
+				     bool keep_module);
+void klp_free_objects(struct klp_patch *patch, enum klp_func_type ftype);
+
 static inline bool klp_is_object_loaded(struct klp_object *obj)
 {
 	return !obj->name || obj->mod;
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 6917100fbe79..d6af190865d2 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -87,6 +87,36 @@ static void klp_complete_transition(void)
 		 klp_transition_patch->mod->name,
 		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
 
+	/*
+	 * For replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook.
+	 */
+	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
+		/*
+		 * Make sure that no ftrace handler accesses any older patch
+		 * on the stack.  This might happen when the user forced the
+		 * transaction while some running tasks were still falling
+		 * back to the old code.  There might even still be ftrace
+		 * handlers that have not seen the last patch on the stack yet.
+		 *
+		 * It probably is not necessary because of the rcu-safe access.
+		 * But better be safe than sorry.
+		 */
+		if (klp_forced)
+			klp_synchronize_transition();
+
+		klp_throw_away_replaced_patches(klp_transition_patch,
+						klp_forced);
+
+		/*
+		 * There is no need to synchronize the transition after removing
+		 * nops. They must be the last on the func_stack. Ftrace
+		 * gurantees that nobody will stay in the trampoline after
+		 * the ftrace handler is unregistered.
+		 */
+		klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
+	}
+
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*
 		 * All tasks have transitioned to KLP_UNPATCHED so we can now
@@ -143,6 +173,15 @@ static void klp_complete_transition(void)
 	if (!klp_forced && klp_target_state == KLP_UNPATCHED)
 		module_put(klp_transition_patch->mod);
 
+	/*
+	 * We do not need to wait until the objects are really freed.
+	 * The patch must be on the bottom of the stack. Therefore it
+	 * will never replace anything else. The only important thing
+	 * is that we wait when the patch is being unregistered.
+	 */
+	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED)
+		klp_free_objects(klp_transition_patch, KLP_FUNC_NOP);
+
 	klp_target_state = KLP_UNDEFINED;
 	klp_transition_patch = NULL;
 }
-- 
2.13.6

[PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Petr Mladek]

The atomic replace feature uses dynamically allocated struct klp_func to
handle functions that will not longer be patched. These structures are
of the type KLP_FUNC_NOP. They cause the ftrace handler to jump to
the original code. But the address of the original code is not known
until the patched module is loaded.

This patch allows the late initialization. Also it adds a sanity check
into the ftrace handler.

Alternative solution would be to do not set the address at all. The ftrace
handler could just return to the original code when NOP struct klp_func
is used. But this would require another changes. For example, in the stack
checking. Note that NOP structures might be available even when the patch
is being disabled. This would happen when the patch enable transition is
reverted.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c  | 16 ++++++++++++++--
 kernel/livepatch/patch.c |  5 +++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ad508a86b2f9..da1438d47d83 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -945,8 +945,12 @@ static void klp_free_object_loaded(struct klp_object *obj)
 
 	obj->mod = NULL;
 
-	klp_for_each_func(obj, func)
+	klp_for_each_func(obj, func) {
 		func->old_addr = 0;
+
+		if (klp_is_func_type(func, KLP_FUNC_NOP))
+			func->new_func = NULL;
+	}
 }
 
 /*
@@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
 
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
-	if (!func->old_name || !func->new_func)
+	if (!func->old_name)
+		return -EINVAL;
+
+	/* NOPs do not know the address until the patched module is loaded. */
+	if (!func->new_func &&
+	    (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
 		return -EINVAL;
 
 	INIT_LIST_HEAD(&func->stack_node);
@@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 			return -ENOENT;
 		}
 
+		if (klp_is_func_type(func, KLP_FUNC_NOP))
+			func->new_func = (void *)func->old_addr;
+
 		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
 						  &func->new_size, NULL);
 		if (!ret) {
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 54b3c379bb0f..1f5c3eea9ee1 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -118,7 +118,12 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	/* Survive ugly mistakes, for example, when handling NOPs. */
+	if (WARN_ON_ONCE(!func->new_func))
+		goto unlock;
+
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
+
 unlock:
 	preempt_enable_notrace();
 }
-- 
2.13.6

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Miroslav Benes]

On Wed, 21 Feb 2018, Petr Mladek wrote:

> The atomic replace feature uses dynamically allocated struct klp_func to
> handle functions that will not longer be patched. These structures are

s/not longer/no longer/, but "handle functions that will not be patched 
any longer" may be even better.

> of the type KLP_FUNC_NOP. They cause the ftrace handler to jump to
> the original code. But the address of the original code is not known
> until the patched module is loaded.
> 
> This patch allows the late initialization. Also it adds a sanity check
> into the ftrace handler.
> 
> Alternative solution would be to do not set the address at all. The ftrace

"Alternative solution would be not to set the address at all." ?

> handler could just return to the original code when NOP struct klp_func
> is used. But this would require another changes. For example, in the stack
> checking. Note that NOP structures might be available even when the patch
> is being disabled. This would happen when the patch enable transition is
> reverted.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c  | 16 ++++++++++++++--
>  kernel/livepatch/patch.c |  5 +++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ad508a86b2f9..da1438d47d83 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -945,8 +945,12 @@ static void klp_free_object_loaded(struct klp_object *obj)
>  
>  	obj->mod = NULL;
>  
> -	klp_for_each_func(obj, func)
> +	klp_for_each_func(obj, func) {
>  		func->old_addr = 0;
> +
> +		if (klp_is_func_type(func, KLP_FUNC_NOP))
> +			func->new_func = NULL;
> +	}
>  }
>  
>  /*
> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
>  
>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  {
> -	if (!func->old_name || !func->new_func)
> +	if (!func->old_name)
> +		return -EINVAL;
> +
> +	/* NOPs do not know the address until the patched module is loaded. */
> +	if (!func->new_func &&
> +	    (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
>  		return -EINVAL;

If we changed the order of klp_init_func() and klp_init_object_loaded() 
calls in klp_init_object(), the hunk would not be needed. Is that correct? 
But that would require more changes elsewhere, so it's not worth it, I 
think.

>  	INIT_LIST_HEAD(&func->stack_node);
> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  			return -ENOENT;
>  		}
>  
> +		if (klp_is_func_type(func, KLP_FUNC_NOP))
> +			func->new_func = (void *)func->old_addr;

Is there a reason why you left the same assignment in 
klp_alloc_func_nop()? This one is enough, no?

Miroslav

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Petr Mladek]

On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> On Wed, 21 Feb 2018, Petr Mladek wrote:
> > This patch allows the late initialization.
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index ad508a86b2f9..da1438d47d83 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >  
> >  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  {
> > -	if (!func->old_name || !func->new_func)
> > +	if (!func->old_name)
> > +		return -EINVAL;
> > +
> > +	/* NOPs do not know the address until the patched module is loaded. */
> > +	if (!func->new_func &&
> > +	    (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
> >  		return -EINVAL;
> 
> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> calls in klp_init_object(), the hunk would not be needed. Is that correct? 

Not really. klp_init_object_loaded() would set func->new_func only
when the object was loaded. But we want to proceed here and create
the kobject for NOPs even when it was not loaded.


> >  	INIT_LIST_HEAD(&func->stack_node);
> > @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> >  			return -ENOENT;
> >  		}
> >  
> > +		if (klp_is_func_type(func, KLP_FUNC_NOP))
> > +			func->new_func = (void *)func->old_addr;
> 
> Is there a reason why you left the same assignment in 
> klp_alloc_func_nop()? This one is enough, no?

Good point! I am going to replace the obsolete assignment
with a comment in v8.

Best Regards,
Petr

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Joe Lawrence]

On 03/01/2018 05:28 AM, Petr Mladek wrote:
> On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
>> On Wed, 21 Feb 2018, Petr Mladek wrote:
>>> This patch allows the late initialization.
>>>
>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>> index ad508a86b2f9..da1438d47d83 100644
>>> --- a/kernel/livepatch/core.c
>>> +++ b/kernel/livepatch/core.c
>>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
>>>  
>>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>>>  {
>>> -	if (!func->old_name || !func->new_func)
>>> +	if (!func->old_name)
>>> +		return -EINVAL;
>>> +
>>> +	/* NOPs do not know the address until the patched module is loaded. */
>>> +	if (!func->new_func &&
>>> +	    (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
>>>  		return -EINVAL;
>>
>> If we changed the order of klp_init_func() and klp_init_object_loaded() 
>> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> 
> Not really. klp_init_object_loaded() would set func->new_func only
> when the object was loaded. But we want to proceed here and create
> the kobject for NOPs even when it was not loaded.
> 
> 
>>>  	INIT_LIST_HEAD(&func->stack_node);
>>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>>>  			return -ENOENT;
>>>  		}
>>>  
>>> +		if (klp_is_func_type(func, KLP_FUNC_NOP))
>>> +			func->new_func = (void *)func->old_addr;
>>
>> Is there a reason why you left the same assignment in 
>> klp_alloc_func_nop()? This one is enough, no?
> 
> Good point! I am going to replace the obsolete assignment
> with a comment in v8.

Hi Petr, Miroslav,

I don't think the assignment in klp_alloc_func_nop() was necessarily
redundant.  It was removed in v9 and that breaks my atomic replace
sample module when I try to load it.  (Perhaps the sample patch has
issues, but here are my debug notes):

To recap:

  patch 1 - modifies cmdline_proc_show()
  patch 2 - modifies only meminfo_proc_show()

when I load patch 2 with .replace=1, klp_add_nops() is called and this
adds a nop function to patch 2 so it reverts cmdline_proc_show():

klp_init_patch()
  if (patch->replace)
    klp_add_nops()
      list_for_each_entry(old_patch, &klp_patches, list) {
        klp_for_each_object(old_patch, old_obj) {
          klp_add_object_nops()
            klp_add_func_nop()
              klp_alloc_func_nop()

the patch continues initialization and I hit the second -EINVAL
condition on that nop function in klp_init_func():

  klp_init_object
    klp_init_func

(from added debug):
[   48.456980] livepatch: func->old_name=cmdline_proc_show
[   48.457620] livepatch: func->new_func=          (null)
[   48.458042] livepatch: klp_is_func_type(func, KLP_FUNC_NOP)=1
[   48.458573] livepatch: klp_is_object_loaded(obj)=1

If I restore the assignment of func->new_func to klp_alloc_func_nop()
then the replacement patch properly loads.  (Reordering the code may
have similar effect?)

I think this problem is contained to only replacement patches that need
the nop-revert feature... if the replacement patch provides a new
function definition, then it shouldn't be affected.

Man, we need a regression test suite for all these cases :)

-- Joe

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Miroslav Benes]

On Fri, 2 Mar 2018, Joe Lawrence wrote:

> On 03/01/2018 05:28 AM, Petr Mladek wrote:
> > On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> >> On Wed, 21 Feb 2018, Petr Mladek wrote:
> >>> This patch allows the late initialization.
> >>>
> >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >>> index ad508a86b2f9..da1438d47d83 100644
> >>> --- a/kernel/livepatch/core.c
> >>> +++ b/kernel/livepatch/core.c
> >>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >>>  
> >>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >>>  {
> >>> -	if (!func->old_name || !func->new_func)
> >>> +	if (!func->old_name)
> >>> +		return -EINVAL;
> >>> +
> >>> +	/* NOPs do not know the address until the patched module is loaded. */
> >>> +	if (!func->new_func &&
> >>> +	    (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
> >>>  		return -EINVAL;
> >>
> >> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> >> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> > 
> > Not really. klp_init_object_loaded() would set func->new_func only
> > when the object was loaded. But we want to proceed here and create
> > the kobject for NOPs even when it was not loaded.
> > 
> > 
> >>>  	INIT_LIST_HEAD(&func->stack_node);
> >>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> >>>  			return -ENOENT;
> >>>  		}
> >>>  
> >>> +		if (klp_is_func_type(func, KLP_FUNC_NOP))
> >>> +			func->new_func = (void *)func->old_addr;
> >>
> >> Is there a reason why you left the same assignment in 
> >> klp_alloc_func_nop()? This one is enough, no?
> > 
> > Good point! I am going to replace the obsolete assignment
> > with a comment in v8.
> 
> Hi Petr, Miroslav,
> 
> I don't think the assignment in klp_alloc_func_nop() was necessarily
> redundant.  It was removed in v9 and that breaks my atomic replace
> sample module when I try to load it.  (Perhaps the sample patch has
> issues, but here are my debug notes):
> 
> To recap:
> 
>   patch 1 - modifies cmdline_proc_show()
>   patch 2 - modifies only meminfo_proc_show()
> 
> when I load patch 2 with .replace=1, klp_add_nops() is called and this
> adds a nop function to patch 2 so it reverts cmdline_proc_show():
> 
> klp_init_patch()
>   if (patch->replace)
>     klp_add_nops()
>       list_for_each_entry(old_patch, &klp_patches, list) {
>         klp_for_each_object(old_patch, old_obj) {
>           klp_add_object_nops()
>             klp_add_func_nop()
>               klp_alloc_func_nop()
> 
> the patch continues initialization and I hit the second -EINVAL
> condition on that nop function in klp_init_func():
> 
>   klp_init_object
>     klp_init_func
> 
> (from added debug):
> [   48.456980] livepatch: func->old_name=cmdline_proc_show
> [   48.457620] livepatch: func->new_func=          (null)
> [   48.458042] livepatch: klp_is_func_type(func, KLP_FUNC_NOP)=1
> [   48.458573] livepatch: klp_is_object_loaded(obj)=1
> 
> If I restore the assignment of func->new_func to klp_alloc_func_nop()
> then the replacement patch properly loads.  (Reordering the code may
> have similar effect?)

Ok, so it was not redundant. Or... I think it should be redundant, but we 
need to define the if condition in klp_init_func() properly.
 
> I think this problem is contained to only replacement patches that need
> the nop-revert feature... if the replacement patch provides a new
> function definition, then it shouldn't be affected.
> 
> Man, we need a regression test suite for all these cases :)

Any volunteer?

Miroslav

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Josh Poimboeuf]

On Mon, Mar 05, 2018 at 10:54:16AM +0100, Miroslav Benes wrote:
> > I think this problem is contained to only replacement patches that need
> > the nop-revert feature... if the replacement patch provides a new
> > function definition, then it shouldn't be affected.
> > 
> > Man, we need a regression test suite for all these cases :)
> 
> Any volunteer?

I'd strongly prefer that we get such selftests in place before merging
any more major features...

-- 
Josh

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

[Petr Mladek]

On Mon 2018-03-05 10:54:16, Miroslav Benes wrote:
> On Fri, 2 Mar 2018, Joe Lawrence wrote:
> 
> > On 03/01/2018 05:28 AM, Petr Mladek wrote:
> > > On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> > >> On Wed, 21 Feb 2018, Petr Mladek wrote:
> > >>> This patch allows the late initialization.
> > >>>
> > >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > >>> index ad508a86b2f9..da1438d47d83 100644
> > >>> --- a/kernel/livepatch/core.c
> > >>> +++ b/kernel/livepatch/core.c
> > >>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> > >>>  
> > >>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > >>>  {
> > >>> -	if (!func->old_name || !func->new_func)
> > >>> +	if (!func->old_name)
> > >>> +		return -EINVAL;
> > >>> +
> > >>> +	/* NOPs do not know the address until the patched module is loaded. */
> > >>> +	if (!func->new_func &&
> > >>> +	    (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
> > >>>  		return -EINVAL;
> > >>
> > >> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> > >> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> > > 
> > > Not really. klp_init_object_loaded() would set func->new_func only
> > > when the object was loaded. But we want to proceed here and create
> > > the kobject for NOPs even when it was not loaded.

Anyway, the above check have to be updated after we removed the
redundant func->new_func assignment in klp_alloc_func_nop().

func->new_func is always NULL for NOPs in klp_init_func().
It is set later in klp_init_object_loaded().

I am going to use the following check in v10:

	/*
	 * NOPs get the address later. The the patched module must be loaded,
	 * see klp_init_object_loaded().
	 */
	if (!func->new_func && !klp_is_func_type(func, KLP_FUNC_NOP))
		return -EINVAL;

> > > 
> > >>>  	INIT_LIST_HEAD(&func->stack_node);
> > >>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> > >>>  			return -ENOENT;
> > >>>  		}
> > >>>  
> > >>> +		if (klp_is_func_type(func, KLP_FUNC_NOP))
> > >>> +			func->new_func = (void *)func->old_addr;
> > >>
> > >> Is there a reason why you left the same assignment in 
> > >> klp_alloc_func_nop()? This one is enough, no?
> > > 
> > > Good point! I am going to replace the obsolete assignment
> > > with a comment in v8.
> > 
> > Hi Petr, Miroslav,
> > 
> > I don't think the assignment in klp_alloc_func_nop() was necessarily
> > redundant.  It was removed in v9 and that breaks my atomic replace
> > sample module when I try to load it.  (Perhaps the sample patch has
> > issues, but here are my debug notes):

Sigh, I hate "trivial" last minute clean ups ;-)


> Ok, so it was not redundant. Or... I think it should be redundant, but we 
> need to define the if condition in klp_init_func() properly.

Exactly, see above.


> > I think this problem is contained to only replacement patches that need
> > the nop-revert feature... if the replacement patch provides a new
> > function definition, then it shouldn't be affected.
> > 
> > Man, we need a regression test suite for all these cases :)

It would be great...

> Any volunteer?

?

Best Regards,
Petr

[PATCH v8 8/8] livepatch: Atomic replace and cumulative patches documentation

[Petr Mladek]

User documentation for the atomic replace feature. It makes it easier
to maintain livepatches using so-called cumulative patches.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/livepatch/cumulative-patches.txt | 83 ++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/livepatch/cumulative-patches.txt

diff --git a/Documentation/livepatch/cumulative-patches.txt b/Documentation/livepatch/cumulative-patches.txt
new file mode 100644
index 000000000000..c041fc1bd259
--- /dev/null
+++ b/Documentation/livepatch/cumulative-patches.txt
@@ -0,0 +1,83 @@
+===================================
+Atomic Replace & Cumulative Patches
+===================================
+
+There might be dependencies between livepatches. If multiple patches need
+to do different changes to the same function(s) then we need to define
+an order in which the patches will be installed. And function implementations
+from any newer livepatch must be done on top of the older ones.
+
+This might become a maintenance nightmare. Especially if anyone would want
+to remove a patch that is in the middle of the stack.
+
+An elegant solution comes with the feature called "Atomic Replace". It allows
+to create so called "Cumulative Patches". They include all wanted changes
+from all older livepatches and completely replace them in one transition.
+
+Usage
+-----
+
+The atomic replace can be enabled by setting "replace" flag in struct klp_patch,
+for example:
+
+	static struct klp_patch patch = {
+		.mod = THIS_MODULE,
+		.objs = objs,
+		.replace = true,
+	};
+
+Such a patch is added on top of the livepatch stack when registered. It can
+be enabled even when some earlier patches have not been enabled yet.
+
+All processes are then migrated to use the code only from the new patch.
+Once the transition is finished, all older patches are removed from the stack
+of patches. Even the older not-enabled patches mentioned above.
+
+Ftrace handlers are transparently removed from functions that are no
+longer modified by the new cumulative patch.
+
+As a result, the livepatch author might maintain sources only for one
+cumulative patch. It helps to keep the patch consistent while adding or
+removing various fixes or features.
+
+
+Limitations:
+------------
+
+  + Replaced patches can no longer be enabled. But if the transition
+    to the cumulative patch was not forced, the kernel modules with
+    the older livepatches can be removed and eventually added again.
+
+    A good practice is to set .replace flag in any released livepatch.
+    Then re-adding an older livepatch is equivalent to downgrading
+    to that patch. This is safe as long as the livepatches do _not_ do
+    extra modifications in (un)patching callbacks or in the module_init()
+    or module_exit() functions, see below.
+
+
+  + Only the (un)patching callbacks from the _new_ cumulative livepatch are
+    executed. Any callbacks from the replaced patches are ignored.
+
+    By other words, the cumulative patch is responsible for doing any actions
+    that are necessary to properly replace any older patch.
+
+    As a result, it might be dangerous to replace newer cumulative patches by
+    older ones. The old livepatches might not provide the necessary callbacks.
+
+    This might be seen as a limitation in some scenarios. But it makes the life
+    easier in many others. Only the new cumulative livepatch knows what
+    fixes/features are added/removed and what special actions are necessary
+    for a smooth transition.
+
+    In each case, it would be a nightmare to think about the order of
+    the various callbacks and their interactions if the callbacks from all
+    enabled patches were called.
+
+
+  + There is no special handling of shadow variables. Livepatch authors
+    must create their own rules how to pass them from one cumulative
+    patch to the other. Especially they should not blindly remove them
+    in module_exit() functions.
+
+    A good practice might be to remove shadow variables in the post-unpatch
+    callback. It is called only when the livepatch is properly disabled.
-- 
2.13.6

Re: [PATCH v8 8/8] livepatch: Atomic replace and cumulative patches documentation

[Miroslav Benes]

On Wed, 21 Feb 2018, Petr Mladek wrote:

> User documentation for the atomic replace feature. It makes it easier
> to maintain livepatches using so-called cumulative patches.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

Joe, are you planning to extend shadow vars documentation you mentioned 
before (v7), or do you want Petr to do it?

Thanks,
Miroslav

Re: [PATCH v8 8/8] livepatch: Atomic replace and cumulative patches documentation

[Joe Lawrence]

On 02/23/2018 05:41 AM, Miroslav Benes wrote:
> On Wed, 21 Feb 2018, Petr Mladek wrote:
> 
>> User documentation for the atomic replace feature. It makes it easier
>> to maintain livepatches using so-called cumulative patches.
>>
>> Signed-off-by: Petr Mladek <pmladek@suse.com>
> 
> Acked-by: Miroslav Benes <mbenes@suse.cz>
> 
> Joe, are you planning to extend shadow vars documentation you mentioned 
> before (v7), or do you want Petr to do it?

I created a sample cumulative patch for testing (I can post later),
but I had a quick question regarding the callbacks...

>From v8 of the patch:

  + Only the (un)patching callbacks from the _new_ cumulative livepatch are
    executed. Any callbacks from the replaced patches are ignored.

maybe I'm not testing this as intended, but I'm not seeing this
behavior with the latest version of the patch.  See below.


Init
====

Setup dynamic debug messages:

  % echo "file kernel/livepatch/* +p" > /sys/kernel/debug/dynamic_debug/control
  % echo "func klp_try_switch_task -p" >> /sys/kernel/debug/dynamic_debug/control


Test 1
======

Copy the original callbacks demo, leave the atomic .replace feature off:

diff -upr samples/livepatch/livepatch-callbacks-{demo.c,demo2.c}
--- samples/livepatch/livepatch-callbacks-demo.c        2018-02-23 09:42:10.370223095 -0500
+++ samples/livepatch/livepatch-callbacks-demo2.c       2018-02-23 11:38:08.372557153 -0500
@@ -191,6 +191,7 @@ static struct klp_object objs[] = {
 static struct klp_patch patch = {
        .mod = THIS_MODULE,
        .objs = objs,
+       .replace = false,
 };
 
 static int livepatch_callbacks_demo_init(void)


Load the two modules, disable and unload:

  % dmesg -C
  % insmod samples/livepatch/livepatch-callbacks-demo.ko
  % insmod samples/livepatch/livepatch-callbacks-demo2.ko
  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
  % rmmod samples/livepatch/livepatch-callbacks-demo.ko

All callbacks are called for both livepatch modules (expected):

  % dmesg
  [ 5755.608999] livepatch: enabling patch 'livepatch_callbacks_demo'
  [ 5755.609014] livepatch: 'livepatch_callbacks_demo': initializing patching transition
> [ 5755.609054] livepatch_callbacks_demo: pre_patch_callback: vmlinux
  [ 5755.609055] livepatch: 'livepatch_callbacks_demo': starting patching transition
  [ 5756.704163] livepatch: 'livepatch_callbacks_demo': completing patching transition
> [ 5756.704259] livepatch_callbacks_demo: post_patch_callback: vmlinux
  [ 5756.704260] livepatch: 'livepatch_callbacks_demo': patching complete
  [ 5760.231035] livepatch: enabling patch 'livepatch_callbacks_demo2'
  [ 5760.231038] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
> [ 5760.231077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
  [ 5760.231078] livepatch: 'livepatch_callbacks_demo2': starting patching transition
  [ 5761.696067] livepatch: 'livepatch_callbacks_demo2': completing patching transition
> [ 5761.696166] livepatch_callbacks_demo2: post_patch_callback: vmlinux
  [ 5761.696166] livepatch: 'livepatch_callbacks_demo2': patching complete
  [ 5765.738278] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
> [ 5765.738319] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
  [ 5765.738319] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
  [ 5767.712143] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
> [ 5767.712502] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
  [ 5767.712503] livepatch: 'livepatch_callbacks_demo2': unpatching complete
  [ 5770.909701] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> [ 5770.909743] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
  [ 5770.909743] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
  [ 5772.704108] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
> [ 5772.704215] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
  [ 5772.704216] livepatch: 'livepatch_callbacks_demo': unpatching complete


Test 2
======

Make the second livepatch a cumulative one:

diff -upr samples/livepatch/livepatch-callbacks-{demo.c,demo2.c}
--- samples/livepatch/livepatch-callbacks-demo.c        2018-02-23 09:42:10.370223095 -0500
+++ samples/livepatch/livepatch-callbacks-demo2.c       2018-02-23 11:16:20.557557153 -0500
@@ -191,6 +191,7 @@ static struct klp_object objs[] = {
 static struct klp_patch patch = {
        .mod = THIS_MODULE,
        .objs = objs,
+       .replace = true,
 };
 
 static int livepatch_callbacks_demo_init(void)

Load the two modules, disable and unload:

  % dmesg -C
  % insmod samples/livepatch/livepatch-callbacks-demo.ko
  % insmod samples/livepatch/livepatch-callbacks-demo2.ko
  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
  % rmmod samples/livepatch/livepatch-callbacks-demo.ko

Pre and post patch callbacks are called for both modules (expected), but
*no* unpatch callbacks are executed:

  % dmesg
  [ 5442.244332] livepatch: enabling patch 'livepatch_callbacks_demo'
  [ 5442.244334] livepatch: 'livepatch_callbacks_demo': initializing patching transition
> [ 5442.244372] livepatch_callbacks_demo: pre_patch_callback: vmlinux
  [ 5442.244372] livepatch: 'livepatch_callbacks_demo': starting patching transition
  [ 5444.704089] livepatch: 'livepatch_callbacks_demo': completing patching transition
> [ 5444.704183] livepatch_callbacks_demo: post_patch_callback: vmlinux
  [ 5444.704184] livepatch: 'livepatch_callbacks_demo': patching complete
  [ 5448.567820] livepatch: enabling patch 'livepatch_callbacks_demo2'
  [ 5448.567823] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
> [ 5448.567860] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
  [ 5448.567861] livepatch: 'livepatch_callbacks_demo2': starting patching transition
  [ 5451.744131] livepatch: 'livepatch_callbacks_demo2': completing patching transition
> [ 5451.744212] livepatch_callbacks_demo2: post_patch_callback: vmlinux
  [ 5451.744213] livepatch: 'livepatch_callbacks_demo2': patching complete
  [ 5455.002339] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
  [ 5455.002378] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
  [ 5456.736068] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
  [ 5456.736132] livepatch: 'livepatch_callbacks_demo2': unpatching complete


BTW, should we just add this test-case to the doc / samples?

-- Joe

Re: [PATCH v8 8/8] livepatch: Atomic replace and cumulative patches documentation

[Miroslav Benes]

On Fri, 23 Feb 2018, Joe Lawrence wrote:

> On 02/23/2018 05:41 AM, Miroslav Benes wrote:
> > On Wed, 21 Feb 2018, Petr Mladek wrote:
> > 
> >> User documentation for the atomic replace feature. It makes it easier
> >> to maintain livepatches using so-called cumulative patches.
> >>
> >> Signed-off-by: Petr Mladek <pmladek@suse.com>
> > 
> > Acked-by: Miroslav Benes <mbenes@suse.cz>
> > 
> > Joe, are you planning to extend shadow vars documentation you mentioned 
> > before (v7), or do you want Petr to do it?
> 
> I created a sample cumulative patch for testing (I can post later),
> but I had a quick question regarding the callbacks...
> 
> >From v8 of the patch:
> 
>   + Only the (un)patching callbacks from the _new_ cumulative livepatch are
>     executed. Any callbacks from the replaced patches are ignored.
> 
> maybe I'm not testing this as intended, but I'm not seeing this
> behavior with the latest version of the patch.  See below.

No, you found a bug, I think. I didn't test callbacks and missed this 
during the review. 
 
> Test 2
> ======
> 
> Make the second livepatch a cumulative one:
> 
> diff -upr samples/livepatch/livepatch-callbacks-{demo.c,demo2.c}
> --- samples/livepatch/livepatch-callbacks-demo.c        2018-02-23 09:42:10.370223095 -0500
> +++ samples/livepatch/livepatch-callbacks-demo2.c       2018-02-23 11:16:20.557557153 -0500
> @@ -191,6 +191,7 @@ static struct klp_object objs[] = {
>  static struct klp_patch patch = {
>         .mod = THIS_MODULE,
>         .objs = objs,
> +       .replace = true,
>  };

The problem is that there are no new/patch functions defined for vmlinux 
and livepatch-callbacks-mod objects. There are only callbacks. Only 
livepatch-callbacks-busymod module has a replacement function. I'll focus 
on vmlinux only. 

No nop functions are added by klp_add_nops(). vmlinux object is empty. 
Then it is destroyed in 
klp_complete_transition()->klp_free_objects(klp_transition_patch, 
KLP_FUNC_NOP). In the end only livepatch-callbacks-busymod is preserved 
regardless the callbacks.

Had you load livepatch-callbacks-busymod module, its callback would be 
called.

Petr, we cannot free the "nop-only" objects if there is a callback defined 
there (an unpatch callback).
  
>  static int livepatch_callbacks_demo_init(void)
> 
> Load the two modules, disable and unload:
> 
>   % dmesg -C
>   % insmod samples/livepatch/livepatch-callbacks-demo.ko
>   % insmod samples/livepatch/livepatch-callbacks-demo2.ko
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
>   % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
>   % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> 
> Pre and post patch callbacks are called for both modules (expected), but
> *no* unpatch callbacks are executed:
> 
>   % dmesg
>   [ 5442.244332] livepatch: enabling patch 'livepatch_callbacks_demo'
>   [ 5442.244334] livepatch: 'livepatch_callbacks_demo': initializing patching transition
> > [ 5442.244372] livepatch_callbacks_demo: pre_patch_callback: vmlinux
>   [ 5442.244372] livepatch: 'livepatch_callbacks_demo': starting patching transition
>   [ 5444.704089] livepatch: 'livepatch_callbacks_demo': completing patching transition
> > [ 5444.704183] livepatch_callbacks_demo: post_patch_callback: vmlinux
>   [ 5444.704184] livepatch: 'livepatch_callbacks_demo': patching complete
>   [ 5448.567820] livepatch: enabling patch 'livepatch_callbacks_demo2'
>   [ 5448.567823] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
> > [ 5448.567860] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
>   [ 5448.567861] livepatch: 'livepatch_callbacks_demo2': starting patching transition
>   [ 5451.744131] livepatch: 'livepatch_callbacks_demo2': completing patching transition
> > [ 5451.744212] livepatch_callbacks_demo2: post_patch_callback: vmlinux
>   [ 5451.744213] livepatch: 'livepatch_callbacks_demo2': patching complete
>   [ 5455.002339] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
>   [ 5455.002378] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
>   [ 5456.736068] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
>   [ 5456.736132] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> 
> 
> BTW, should we just add this test-case to the doc / samples?

Why not. I think we need to transform the samples into a testsuite or 
selftests, because their role has changed.

Thanks for the report. The atomic replace patch set has become a nightmare 
for me :). So many tiny corner cases everywhere and I'm not capable to 
hold every detail in my brain during the review.

Miroslav

Re: [PATCH v8 0/8] livepatch: Atomic replace feature

[Miroslav Benes]

> I have found one bug in v7. We were not able to initialize NOP
> struct klp_func when the patches module is not loaded. It was
> because func->new_func was NULL. I have fixed it in separate patch
> for an easier review.

This is embarassing. I'd swear I tested this. At least it was a part of my 
tests. So I made a mistake somewhere :/

Thanks for fixing it.

Miroslav

Re: [PATCH v8 0/8] livepatch: Atomic replace feature

[Evgenii Shatokhin]

Hi,

Petr, Jason - thanks a lot for working on this series, first of all! And 
especially for your patience.

On 21.02.2018 16:29, Petr Mladek wrote:
> The atomic replace allows to create cumulative patches. They
> are useful when you maintain many livepatches and want to remove
> one that is lower on the stack. In addition it is very useful when
> more patches touch the same function and there are dependencies
> between them.

I have experimented with this updated patchset a bit.
It looks like replace operation fails if there is a loaded but disabled 
patch.

Suppose there are 2 binary patches changing the same kernel functions. 
Load patch1, then disable it (echo 0 > 
/sys/kernel/livepatch/patch1/enabled), then try load patch2 with atomic 
replace. I get "Device or resource busy" error at this point.

It seems, __klp_enable_patch() returns -EBUSY for some reason. I haven't 
dug deeper into that code yet.

A workaround is simple: just unload all disabled patches before trying 
to load patch2 with replace. Still, the behavior is quite strange.

> 
> I have found one bug in v7. We were not able to initialize NOP
> struct klp_func when the patches module is not loaded. It was
> because func->new_func was NULL. I have fixed it in separate patch
> for an easier review.
> 
> Note that the original Jason's patch did not have this problem
> because func->new_func was always NULL there. But this required
> NOP-specific handling also on other locations, namely
> klp_ftrace_handler() and klp_check_stack_func().
> 
> Changes against v7:
> 
>    + Fixed handling of NOPs for not-yet-loaded modules
>    + Made klp_replaced_patches list static [Mirek]
>    + Made klp_free_object() public later [Mirek]
>    + Fixed several reported typos [Mirek, Joe]
>    + Updated documentation according to the feedback [Joe]
>    + Added some Acks [Mirek]
> 
> Changes against v6:
> 
>    + used list_move when disabling replaced patches [Jason]
>    + renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek]
>    + used klp_is_func_type() in klp_unpatch_object() [Mirek]
>    + moved static definition of klp_get_or_add_object() [Mirek]
>    + updated comment about synchronization in forced mode [Mirek]
>    + added user documentation
>    + fixed several typos
> 
> Jason Baron (5):
>    livepatch: Use lists to manage patches, objects and functions
>    livepatch: Initial support for dynamic structures
>    livepatch: Allow to unpatch only functions of the given type
>    livepatch: Support separate list for replaced patches.
>    livepatch: Add atomic replace
> 
> Petr Mladek (3):
>    livepatch: Free only structures with initialized kobject
>    livepatch: Correctly handle atomic replace for not yet loaded modules
>    livepatch: Atomic replace and cumulative patches documentation
> 
>   Documentation/livepatch/cumulative-patches.txt |  83 ++++++
>   include/linux/livepatch.h                      |  59 +++-
>   kernel/livepatch/core.c                        | 394 ++++++++++++++++++++++---
>   kernel/livepatch/core.h                        |   4 +
>   kernel/livepatch/patch.c                       |  31 +-
>   kernel/livepatch/patch.h                       |   4 +-
>   kernel/livepatch/transition.c                  |  41 ++-
>   7 files changed, 566 insertions(+), 50 deletions(-)
>   create mode 100644 Documentation/livepatch/cumulative-patches.txt
> 

Regards,
Evgenii

Re: [PATCH v8 0/8] livepatch: Atomic replace feature

[Miroslav Benes]

On Mon, 5 Mar 2018, Evgenii Shatokhin wrote:

> Hi,

Hi,
 
> > The atomic replace allows to create cumulative patches. They
> > are useful when you maintain many livepatches and want to remove
> > one that is lower on the stack. In addition it is very useful when
> > more patches touch the same function and there are dependencies
> > between them.
> 
> I have experimented with this updated patchset a bit.
> It looks like replace operation fails if there is a loaded but disabled patch.
> 
> Suppose there are 2 binary patches changing the same kernel functions. Load
> patch1, then disable it (echo 0 > /sys/kernel/livepatch/patch1/enabled), then
> try load patch2 with atomic replace. I get "Device or resource busy" error at
> this point.
> 
> It seems, __klp_enable_patch() returns -EBUSY for some reason. I haven't dug
> deeper into that code yet.

Yes, it is "enforce stacking" check in __klp_enable_patch():

        /* enforce stacking: only the first disabled patch can be enabled */
        if (patch->list.prev != &klp_patches &&
            !list_prev_entry(patch, list)->enabled)
                return -EBUSY;

So not connected to this patch set. We've had the behaviour for quite some 
time.

Miroslav
 
> A workaround is simple: just unload all disabled patches before trying to load
> patch2 with replace. Still, the behavior is quite strange.

Re: [PATCH v8 0/8] livepatch: Atomic replace feature

[Petr Mladek]

On Mon 2018-03-05 13:54:38, Miroslav Benes wrote:
> On Mon, 5 Mar 2018, Evgenii Shatokhin wrote:
> 
> > Hi,
> 
> Hi,
>  
> > > The atomic replace allows to create cumulative patches. They
> > > are useful when you maintain many livepatches and want to remove
> > > one that is lower on the stack. In addition it is very useful when
> > > more patches touch the same function and there are dependencies
> > > between them.
> > 
> > I have experimented with this updated patchset a bit.
> > It looks like replace operation fails if there is a loaded but disabled patch.
> > 
> > Suppose there are 2 binary patches changing the same kernel functions. Load
> > patch1, then disable it (echo 0 > /sys/kernel/livepatch/patch1/enabled), then
> > try load patch2 with atomic replace. I get "Device or resource busy" error at
> > this point.
> > 
> > It seems, __klp_enable_patch() returns -EBUSY for some reason. I haven't dug
> > deeper into that code yet.
> 
> Yes, it is "enforce stacking" check in __klp_enable_patch():
> 
>         /* enforce stacking: only the first disabled patch can be enabled */
>         if (patch->list.prev != &klp_patches &&
>             !list_prev_entry(patch, list)->enabled)
>                 return -EBUSY;
> 
> So not connected to this patch set. We've had the behaviour for quite some 
> time.

We actually wanted to enabled this for the patches with the replace
flag. It is even described in
Documentation/livepatch/cumulative-patches.txt.

Of course, you could always unregister all the disabled patches that
block the new one. But it might be inconvenient.

I do not see any big reason why this should be disabled:

  + klp_add_nops() takes into account even disabled patches.
    In the worst case, we might enable some NOPs that are
    not really needed.

  + klp_throw_away_replaced_patches() removes all patches down
    the stack.

I am going to enable this in a separate patch in v10.

Best Regards,
Petr