[PATCH v7 0/7] livepatch: Atomic replace feature

[PATCH v7 0/7] 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.

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 (2):
  livepatch: Free only structures with initialized kobject
  livepatch: Atomic replace and cumulative patches documentation

 Documentation/livepatch/cumulative-patches.txt |  76 +++++
 include/linux/livepatch.h                      |  59 +++-
 kernel/livepatch/core.c                        | 378 ++++++++++++++++++++++---
 kernel/livepatch/core.h                        |   4 +
 kernel/livepatch/patch.c                       |  29 +-
 kernel/livepatch/patch.h                       |   4 +-
 kernel/livepatch/transition.c                  |  41 ++-
 7 files changed, 543 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/livepatch/cumulative-patches.txt

-- 
2.13.6

[PATCH v7 1/7] 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>
Cc: 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

Re: [PATCH v7 1/7] livepatch: Use lists to manage patches, objects and functions

[Miroslav Benes]

On Tue, 6 Feb 2018, Petr Mladek wrote:

> 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>
> Cc: Miroslav Benes <mbenes@suse.cz>

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

Miroslav

[PATCH v7 2/7] 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 not 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: Miroslav Benes <mbenes@suse.cz>
Cc: Jason Baron <jbaron@akamai.com>
---
 kernel/livepatch/core.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 1d525f4a270a..ef7c3b5f561b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -654,16 +654,17 @@ static struct kobj_type klp_ktype_func = {
 };
 
 /*
- * Free all functions' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
+ * Free all funcs that have the kobject initialized. Otherwise,
+ * nothing is needed.
  */
-static void klp_free_funcs_limited(struct klp_object *obj,
-				   struct klp_func *limit)
+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 */
@@ -678,23 +679,25 @@ static void klp_free_object_loaded(struct klp_object *obj)
 }
 
 /*
- * Free all objects' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
+ * Free all funcs and objects that have the kobject initialized.
+ * Otherwise, nothing is needed.
  */
-static void klp_free_objects_limited(struct klp_patch *patch,
-				     struct klp_object *limit)
+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 +794,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 +840,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

Re: [PATCH v7 2/7] livepatch: Free only structures with initialized kobject

[Miroslav Benes]

On Tue, 6 Feb 2018, Petr Mladek wrote:

> 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 not longer patched.

s/not longer/no longer/

It applies to the other changelogs as well.
 
> 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: Miroslav Benes <mbenes@suse.cz>
> Cc: Jason Baron <jbaron@akamai.com>
> ---
>  kernel/livepatch/core.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 1d525f4a270a..ef7c3b5f561b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -654,16 +654,17 @@ static struct kobj_type klp_ktype_func = {
>  };
>  
>  /*
> - * Free all functions' kobjects in the array up to some limit. When limit is
> - * NULL, all kobjects are freed.
> + * Free all funcs that have the kobject initialized. Otherwise,
> + * nothing is needed.

s/needed/done/ ?

I'd remove the second sentence in that case.

>   */
> -static void klp_free_funcs_limited(struct klp_object *obj,
> -				   struct klp_func *limit)
> +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 */
> @@ -678,23 +679,25 @@ static void klp_free_object_loaded(struct klp_object *obj)
>  }
>  
>  /*
> - * Free all objects' kobjects in the array up to some limit. When limit is
> - * NULL, all kobjects are freed.
> + * Free all funcs and objects that have the kobject initialized.
> + * Otherwise, nothing is needed.

Same here.

>   */
> -static void klp_free_objects_limited(struct klp_patch *patch,
> -				     struct klp_object *limit)
> +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 +794,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;
>  }

I'm slightly worried about not keeping the error handling here but 
offloading it to the caller, because we may forget to update it one day. 
But yes, it keeps things simple and I can live with that.

>  static int klp_init_patch(struct klp_patch *patch)
> @@ -842,7 +840,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);

With those nits fixed

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

Miroslav

[PATCH v7 3/7] 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 not 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 not
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>
Cc: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h |  37 +++++++++++-
 kernel/livepatch/core.c   | 139 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 159 insertions(+), 17 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 ef7c3b5f561b..ab3e13b71bfd 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 = {
@@ -654,16 +746,25 @@ static struct kobj_type klp_ktype_func = {
 };
 
 /*
- * Free all funcs that have the kobject initialized. Otherwise,
- * nothing is needed.
+ * 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)
+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);
 	}
 }
 
@@ -679,24 +780,33 @@ static void klp_free_object_loaded(struct klp_object *obj)
 }
 
 /*
- * Free all funcs and objects that have the kobject initialized.
- * Otherwise, nothing is needed.
+ * 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)
+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);
@@ -777,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;
 
@@ -840,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

Re: [PATCH v7 3/7] livepatch: Initial support for dynamic structures

[Miroslav Benes]

On Tue, 6 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 not 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 not
> 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>
> Cc: Miroslav Benes <mbenes@suse.cz>

This patch introduces klp_get_or_add_object(), but it is not used here. 
One of the later patches adds its call site. GCC warns about this. I like 
it being introduced here, because it belongs to the patch. Someone may 
complain though.

Also klp_free_objects() is not declared as static and sparse complains 
about it. Petr, you can make it static here and drop the keyword later.

Otherwise it looks good.

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

Miroslav

[PATCH v7 4/7] 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 not 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      | 29 ++++++++++++++++++++++-------
 kernel/livepatch/patch.h      |  4 ++--
 kernel/livepatch/transition.c |  2 +-
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ab3e13b71bfd..fccb603942f1 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..e5f2117e07b6 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -236,15 +236,29 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+/*
+ * Remove ftrace handler for the given object and function type.
+ *
+ * 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 +272,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 +281,12 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+/* Removes ftrace handler in all objects for the given function type. */
+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 v7 4/7] livepatch: Allow to unpatch only functions of the given type

[Miroslav Benes]

> +/*
> + * Remove ftrace handler for the given object and function type.

Isn't it imprecise? It does not necessarily remove ftrace handler. It 
removes a function of given ftype from func_stack. I'd remove the 
sentence.

> + *
> + * 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 +272,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 +281,12 @@ int klp_patch_object(struct klp_object *obj)
>  	return 0;
>  }
>  
> -void klp_unpatch_objects(struct klp_patch *patch)
> +/* Removes ftrace handler in all objects for the given function type. */

Same here.

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

Miroslav

[PATCH v7 5/7] 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 not 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>
Cc: 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 fccb603942f1..11632fe8716a 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().
+ */
+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

Re: [PATCH v7 5/7] livepatch: Support separate list for replaced patches.

[Miroslav Benes]

On Tue, 6 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.
> 
> 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 not 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>
> Cc: 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 fccb603942f1..11632fe8716a 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().
> + */
> +LIST_HEAD(klp_replaced_patches);

It should be static, I think. It is used only in kernel/livepatch/core.c. 
And sparse complains about it.

With that fixed

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

Miroslav

[PATCH v7 6/7] 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 not 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 we would need to complicated to reuse the init() functions
    and error paths. It would increase the risk of errors because of
    late kobject initialization. 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>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h     |   3 +
 kernel/livepatch/core.c       | 160 +++++++++++++++++++++++++++++++++++++++++-
 kernel/livepatch/core.h       |   4 ++
 kernel/livepatch/transition.c |  39 ++++++++++
 4 files changed, 205 insertions(+), 1 deletion(-)

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 11632fe8716a..d16896fa6a4c 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 not longer accessible. All functions
+ * are redirected using the klp_transition_patch. They use either
+ * a new code or they 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
+ * there 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:
  *
@@ -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

Re: [PATCH v7 6/7] livepatch: Add atomic replace

[Miroslav Benes]

On Tue, 6 Feb 2018, Petr Mladek wrote:

> 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 not 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 we would need to complicated to reuse the init() functions

I cannot parse this.

>     and error paths. It would increase the risk of errors because of
>     late kobject initialization. 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>
> Cc: Miroslav Benes <mbenes@suse.cz>
> Cc: Petr Mladek <pmladek@suse.com>

[...]

> +/*
> + * 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 not longer accessible. All functions

...are no longer... :)

> + * are redirected using the klp_transition_patch. They use either
> + * a new code or they in the original code because of the special

...or they are... ?

> + * nop function patches.
> + */

[...]

> +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
> + * there will not be really needed. But it does not harm and

s/there/which/ ?

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

So again only nits. Otherwise I think the patch does what is supposed to.

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

Miroslav

[PATCH v7 7/7] 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 | 76 ++++++++++++++++++++++++++
 1 file changed, 76 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..5f1f3760b840
--- /dev/null
+++ b/Documentation/livepatch/cumulative-patches.txt
@@ -0,0 +1,76 @@
+===================================
+Atomic Replace & Cumulative Patches
+===================================
+
+There are dependencies between livepatches when more patches modify the same
+function(s). Then any newer livepatch must include changes from the older ones.
+Also the patches must be registered in the right order.
+
+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 cumulative patches that completely replace all older livepatches.
+
+
+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 might
+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.
+
+Ftrace handlers are transparently removed from functions that are not
+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 not longer be enabled. But if the transition
+    was not forced, the older patches might be unregistered, removed
+    and eventually used again.
+
+
+  + Only the (un)patching callbacks from the _new_ cumulative livepatch are
+    proceed. 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 v7 7/7] livepatch: Atomic replace and cumulative patches documentation

[Joe Lawrence]

On Tue, Feb 06, 2018 at 11:34:24AM +0100, Petr Mladek wrote:
> User documentation for the atomic replace feature. It makes it easier
> to maintain livepatches using so-called cumulative patches.

Thanks for adding this doc.  A few minor wording suggestions and typos
below...

> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  Documentation/livepatch/cumulative-patches.txt | 76 ++++++++++++++++++++++++++
>  1 file changed, 76 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..5f1f3760b840
> --- /dev/null
> +++ b/Documentation/livepatch/cumulative-patches.txt
> @@ -0,0 +1,76 @@
> +===================================
> +Atomic Replace & Cumulative Patches
> +===================================
> +
> +There are dependencies between livepatches when more patches modify the same

s/more/multiple

> +function(s). Then any newer livepatch must include changes from the older ones.

If the new patch wants to maintain the original change that is.
(Perhaps that's implied here.)  

Also this might be a good place to formally introduce the "cumulative
patch" as a recurring term.

> +Also the patches must be registered in the right order.
> +
> +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 cumulative patches that completely replace all older livepatches.
> +
> +
> +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 might
> +be enabled even when some earlier patches have not been enabled yet.

"It can be enabled" reads a little more naturally I think.

> +
> +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 not
> +longer modified by the new cumulative patch.
> +

s/not longer/no longer

> +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 not longer be enabled. But if the transition

s/not longer/no longer

and "the transition" refers to the older patch transition, right?  (Not
the cumulative patching transition.)

> +    was not forced, the older patches might be unregistered, removed
> +    and eventually used again.
> +
> +
> +  + Only the (un)patching callbacks from the _new_ cumulative livepatch are
> +    proceed. Any callbacks from the replaced patches are ignored.

s/proceed/executed

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

I wonder if this deserves comment or an example in the callbacks
document, even if its a simple, contrived callback.  (I'll think on
this.)

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

Same here.

-- Joe

Re: [PATCH v7 0/7] livepatch: Atomic replace feature

[Miroslav Benes]

On Tue, 6 Feb 2018, 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.

So I ported my old silly kgraft modules to livepatch and it works well. It 
does not say that much, because the tests are really simple stupid, but at 
least something.

Miroslav