[PATCH 0/4] livepatch: Add garbage collection for shadow variables

[PATCH 0/4] livepatch: Add garbage collection for shadow variables

[Marcos Paulo de Souza]

Hello reviewers,

These patches were originally created by Petr. The patches 3 and 4 were
originally one bigger patch but they are more easily reviewed split.

The patches 1 and 2 are code improvements.
Patch 3 adds a new struct called klp_shadow_type that combines the id, ctor and
dtor of a shadow variable. This patch is needed by patch 4.

Patch 4 adds the garbage collection.

Thanks for reviewing!

Marcos Paulo de Souza (2):
  livepatch/shadow: Introduce klp_shadow_type structure
  livepatch/shadow: Add garbage collection of shadow variables

Petr Mladek (2):
  livepatch/shadow: Separate code to get or use pre-allocated shadow
    variable
  livepatch/shadow: Separate code removing all shadow variables for a
    given id

 include/linux/livepatch.h                     |  50 ++-
 kernel/livepatch/core.c                       |  39 +++
 kernel/livepatch/core.h                       |   1 +
 kernel/livepatch/shadow.c                     | 296 +++++++++++++-----
 kernel/livepatch/transition.c                 |   4 +-
 lib/livepatch/test_klp_shadow_vars.c          | 119 ++++---
 samples/livepatch/livepatch-shadow-fix1.c     |  24 +-
 samples/livepatch/livepatch-shadow-fix2.c     |  34 +-
 .../selftests/livepatch/test-shadow-vars.sh   |   2 +-
 9 files changed, 415 insertions(+), 154 deletions(-)

-- 
2.35.3

[PATCH 1/4] livepatch/shadow: Separate code to get or use pre-allocated shadow variable

[Marcos Paulo de Souza]

From: Petr Mladek <pmladek@suse.com>

Separate code that is used in klp_shadow_get_or_alloc() under klp_mutex.
It splits a long spaghetti function into two. Also it unifies the error
handling. The old used a mix of duplicated code, direct returns,
and goto. The new code has only one unlock, free, and return calls.

Background: The change was needed by an earlier variant of the code adding
	garbage collection of shadow variables. It is not needed in
	the end. But the change still looks like an useful clean up.

It is code refactoring without any functional changes.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/shadow.c | 77 ++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index c2e724d97ddf..67c1313f6831 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -101,41 +101,19 @@ void *klp_shadow_get(void *obj, unsigned long id)
 }
 EXPORT_SYMBOL_GPL(klp_shadow_get);
 
-static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
-				       size_t size, gfp_t gfp_flags,
-				       klp_shadow_ctor_t ctor, void *ctor_data,
-				       bool warn_on_exist)
+static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
+				     struct klp_shadow *new_shadow,
+				     klp_shadow_ctor_t ctor, void *ctor_data,
+				     bool *exist)
 {
-	struct klp_shadow *new_shadow;
 	void *shadow_data;
-	unsigned long flags;
-
-	/* Check if the shadow variable already exists */
-	shadow_data = klp_shadow_get(obj, id);
-	if (shadow_data)
-		goto exists;
-
-	/*
-	 * Allocate a new shadow variable.  Fill it with zeroes by default.
-	 * More complex setting can be done by @ctor function.  But it is
-	 * called only when the buffer is really used (under klp_shadow_lock).
-	 */
-	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
-	if (!new_shadow)
-		return NULL;
 
-	/* Look for <obj, id> again under the lock */
-	spin_lock_irqsave(&klp_shadow_lock, flags);
 	shadow_data = klp_shadow_get(obj, id);
 	if (unlikely(shadow_data)) {
-		/*
-		 * Shadow variable was found, throw away speculative
-		 * allocation.
-		 */
-		spin_unlock_irqrestore(&klp_shadow_lock, flags);
-		kfree(new_shadow);
-		goto exists;
+		*exist = true;
+		return shadow_data;
 	}
+	*exist = false;
 
 	new_shadow->obj = obj;
 	new_shadow->id = id;
@@ -145,8 +123,6 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 
 		err = ctor(obj, new_shadow->data, ctor_data);
 		if (err) {
-			spin_unlock_irqrestore(&klp_shadow_lock, flags);
-			kfree(new_shadow);
 			pr_err("Failed to construct shadow variable <%p, %lx> (%d)\n",
 			       obj, id, err);
 			return NULL;
@@ -156,12 +132,45 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 	/* No <obj, id> found, so attach the newly allocated one */
 	hash_add_rcu(klp_shadow_hash, &new_shadow->node,
 		     (unsigned long)new_shadow->obj);
-	spin_unlock_irqrestore(&klp_shadow_lock, flags);
 
 	return new_shadow->data;
+}
+
+static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
+				       size_t size, gfp_t gfp_flags,
+				       klp_shadow_ctor_t ctor, void *ctor_data,
+				       bool warn_on_exist)
+{
+	struct klp_shadow *new_shadow;
+	void *shadow_data;
+	bool exist;
+	unsigned long flags;
+
+	/* Check if the shadow variable already exists */
+	shadow_data = klp_shadow_get(obj, id);
+	if (shadow_data)
+		return shadow_data;
+
+	/*
+	 * Allocate a new shadow variable.  Fill it with zeroes by default.
+	 * More complex setting can be done by @ctor function.  But it is
+	 * called only when the buffer is really used (under klp_shadow_lock).
+	 */
+	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
+	if (!new_shadow)
+		return NULL;
+
+	/* Look for <obj, id> again under the lock */
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+	shadow_data = __klp_shadow_get_or_use(obj, id, new_shadow,
+					      ctor, ctor_data, &exist);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	/* Throw away unused speculative allocation. */
+	if (!shadow_data || exist)
+		kfree(new_shadow);
 
-exists:
-	if (warn_on_exist) {
+	if (exist && warn_on_exist) {
 		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
 		return NULL;
 	}
-- 
2.35.3

Re: [PATCH 1/4] livepatch/shadow: Separate code to get or use pre-allocated shadow variable

[Josh Poimboeuf]

On Fri, Jul 01, 2022 at 04:48:14PM -0300, Marcos Paulo de Souza wrote:
> From: Petr Mladek <pmladek@suse.com>
> 
> Separate code that is used in klp_shadow_get_or_alloc() under klp_mutex.
> It splits a long spaghetti function into two. Also it unifies the error
> handling. The old used a mix of duplicated code, direct returns,
> and goto. The new code has only one unlock, free, and return calls.
> 
> Background: The change was needed by an earlier variant of the code adding
> 	garbage collection of shadow variables. It is not needed in
> 	the end. But the change still looks like an useful clean up.
> 
> It is code refactoring without any functional changes.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Hi Marcos,

Invalid Signed-off-by chain, it will need your SOB added as well.

Also, when posting patches please add lkml to Cc so they're archived and
visible to the wider community.

-- 
Josh

Re: [PATCH 1/4] livepatch/shadow: Separate code to get or use pre-allocated shadow variable

[Petr Mladek]

On Fri 2022-07-01 16:48:14, Marcos Paulo de Souza wrote:
> From: Petr Mladek <pmladek@suse.com>
> 
> Separate code that is used in klp_shadow_get_or_alloc() under klp_mutex.
> It splits a long spaghetti function into two. Also it unifies the error
> handling. The old used a mix of duplicated code, direct returns,
> and goto. The new code has only one unlock, free, and return calls.
> 
> Background: The change was needed by an earlier variant of the code adding
> 	garbage collection of shadow variables. It is not needed in
> 	the end. But the change still looks like an useful clean up.

Maybe the above paragraph is superfluous. I would remove it after all.

> It is code refactoring without any functional changes.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

(It is fun to review my own RFC patch).

> ---
>  kernel/livepatch/shadow.c | 77 ++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> index c2e724d97ddf..67c1313f6831 100644
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -101,41 +101,19 @@ void *klp_shadow_get(void *obj, unsigned long id)
>  }
>  EXPORT_SYMBOL_GPL(klp_shadow_get);
>  
> -static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
> -				       size_t size, gfp_t gfp_flags,
> -				       klp_shadow_ctor_t ctor, void *ctor_data,
> -				       bool warn_on_exist)
> +static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
> +				     struct klp_shadow *new_shadow,
> +				     klp_shadow_ctor_t ctor, void *ctor_data,
> +				     bool *exist)

I have to admit that the name is a bit misleading. The real meaning
is "get existing" or "use newly allocated one".

I think that the following might better describe the real meaning
in the context where it is used:

     __klp_shadow_get_or_alloc_locked()
     __klp_shadow_get_or_add()

Anyway, it would deserve some comment above the function definition.
For example:

/*
 * Check if the variable already exists. Otherwise, add
 * the pre-allocated one.
 */

>  {
> -	struct klp_shadow *new_shadow;
>  	void *shadow_data;
> -	unsigned long flags;

It would deserve:

	lockdep_assert_held(&klp_shadow_lock);

> -
> -	/* Check if the shadow variable already exists */
> -	shadow_data = klp_shadow_get(obj, id);
> -	if (shadow_data)
> -		goto exists;
> -

Best Regards,
Petr

[PATCH 2/4] livepatch/shadow: Separate code removing all shadow variables for a given id

[Marcos Paulo de Souza]

From: Petr Mladek <pmladek@suse.com>

Allow to remove all shadow variables with already taken klp_shadow_lock.
It will be needed to synchronize this with other operation during
the garbage collection of shadow variables.

It is a code refactoring without any functional changes.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/shadow.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index 67c1313f6831..79b8646b1d4c 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -280,6 +280,20 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
 }
 EXPORT_SYMBOL_GPL(klp_shadow_free);
 
+static void __klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
+{
+	struct klp_shadow *shadow;
+	int i;
+
+	lockdep_assert_held(&klp_shadow_lock);
+
+	/* Delete all <*, id> from hash */
+	hash_for_each(klp_shadow_hash, i, shadow, node) {
+		if (klp_shadow_match(shadow, shadow->obj, id))
+			klp_shadow_free_struct(shadow, dtor);
+	}
+}
+
 /**
  * klp_shadow_free_all() - detach and free all <_, id> shadow variables
  * @id:		data identifier
@@ -291,18 +305,10 @@ EXPORT_SYMBOL_GPL(klp_shadow_free);
  */
 void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
 {
-	struct klp_shadow *shadow;
 	unsigned long flags;
-	int i;
 
 	spin_lock_irqsave(&klp_shadow_lock, flags);
-
-	/* Delete all <_, id> from hash */
-	hash_for_each(klp_shadow_hash, i, shadow, node) {
-		if (klp_shadow_match(shadow, shadow->obj, id))
-			klp_shadow_free_struct(shadow, dtor);
-	}
-
+	__klp_shadow_free_all(id, dtor);
 	spin_unlock_irqrestore(&klp_shadow_lock, flags);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_free_all);
-- 
2.35.3

Re: [PATCH 2/4] livepatch/shadow: Separate code removing all shadow variables for a given id

[Petr Mladek]

On Fri 2022-07-01 16:48:15, Marcos Paulo de Souza wrote:
> From: Petr Mladek <pmladek@suse.com>
> 
> Allow to remove all shadow variables with already taken klp_shadow_lock.
> It will be needed to synchronize this with other operation during
> the garbage collection of shadow variables.
> 
> It is a code refactoring without any functional changes.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

It is pretty straightforward. I am not sure if it makes sense but

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Marcos Paulo de Souza]

The shadow variable type will be used in klp_shadow_alloc/get/free
functions instead of id/ctor/dtor parameters. As a result, all callers
use the same callbacks consistently[*][**].

The structure will be used in the next patch that will manage the
lifetime of shadow variables and execute garbage collection automatically.

[*] From the user POV, it might have been easier to pass $id instead
    of pointer to struct klp_shadow_type.

    The problem is that each livepatch registers its own struct
    klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
    be unclear what callback should be used. They should be compatible.

    This problem is gone when each livepatch explicitly uses its
    own struct klp_shadow_type pointing to its own callbacks.

[**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
    The message must be disabled when called via klp_shadow_free_all()
    because the ordering of freed variables is not well defined there.
    It has to be done using another hack after switching to
    klp_shadow_types.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h                     |  29 +++--
 kernel/livepatch/shadow.c                     | 103 ++++++++---------
 lib/livepatch/test_klp_shadow_vars.c          | 105 ++++++++++--------
 samples/livepatch/livepatch-shadow-fix1.c     |  18 ++-
 samples/livepatch/livepatch-shadow-fix2.c     |  27 +++--
 .../selftests/livepatch/test-shadow-vars.sh   |   2 +-
 6 files changed, 163 insertions(+), 121 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 293e29960c6e..79e7bf3b35f6 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -216,15 +216,26 @@ typedef int (*klp_shadow_ctor_t)(void *obj,
 				 void *ctor_data);
 typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
 
-void *klp_shadow_get(void *obj, unsigned long id);
-void *klp_shadow_alloc(void *obj, unsigned long id,
-		       size_t size, gfp_t gfp_flags,
-		       klp_shadow_ctor_t ctor, void *ctor_data);
-void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
-			      size_t size, gfp_t gfp_flags,
-			      klp_shadow_ctor_t ctor, void *ctor_data);
-void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
-void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
+/**
+ * struct klp_shadow_type - shadow variable type used by the klp_object
+ * @id:		shadow variable type indentifier
+ * @ctor:	custom constructor to initialize the shadow data (optional)
+ * @dtor:	custom callback that can be used to unregister the variable
+ *		and/or free data that the shadow variable points to (optional)
+ */
+struct klp_shadow_type {
+	unsigned long id;
+	klp_shadow_ctor_t ctor;
+	klp_shadow_dtor_t dtor;
+};
+
+void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type);
+void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
+		       size_t size, gfp_t gfp_flags, void *ctor_data);
+void *klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
+			      size_t size, gfp_t gfp_flags, void *ctor_data);
+void klp_shadow_free(void *obj, struct klp_shadow_type *shadow_type);
+void klp_shadow_free_all(struct klp_shadow_type *shadow_type);
 
 struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
 struct klp_state *klp_get_prev_state(unsigned long id);
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index 79b8646b1d4c..9dcbb626046e 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -63,24 +63,24 @@ struct klp_shadow {
  * klp_shadow_match() - verify a shadow variable matches given <obj, id>
  * @shadow:	shadow variable to match
  * @obj:	pointer to parent object
- * @id:		data identifier
+ * @shadow_type: type of the wanted shadow variable
  *
  * Return: true if the shadow variable matches.
  */
 static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
-				unsigned long id)
+				struct klp_shadow_type *shadow_type)
 {
-	return shadow->obj == obj && shadow->id == id;
+	return shadow->obj == obj && shadow->id == shadow_type->id;
 }
 
 /**
  * klp_shadow_get() - retrieve a shadow variable data pointer
  * @obj:	pointer to parent object
- * @id:		data identifier
+ * @shadow_type: type of the wanted shadow variable
  *
  * Return: the shadow variable data element, NULL on failure.
  */
-void *klp_shadow_get(void *obj, unsigned long id)
+void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type)
 {
 	struct klp_shadow *shadow;
 
@@ -89,7 +89,7 @@ void *klp_shadow_get(void *obj, unsigned long id)
 	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
 				   (unsigned long)obj) {
 
-		if (klp_shadow_match(shadow, obj, id)) {
+		if (klp_shadow_match(shadow, obj, shadow_type)) {
 			rcu_read_unlock();
 			return shadow->data;
 		}
@@ -101,14 +101,13 @@ void *klp_shadow_get(void *obj, unsigned long id)
 }
 EXPORT_SYMBOL_GPL(klp_shadow_get);
 
-static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
-				     struct klp_shadow *new_shadow,
-				     klp_shadow_ctor_t ctor, void *ctor_data,
+static void *__klp_shadow_get_or_use(void *obj, struct klp_shadow_type *shadow_type,
+				     struct klp_shadow *new_shadow, void *ctor_data,
 				     bool *exist)
 {
 	void *shadow_data;
 
-	shadow_data = klp_shadow_get(obj, id);
+	shadow_data = klp_shadow_get(obj, shadow_type);
 	if (unlikely(shadow_data)) {
 		*exist = true;
 		return shadow_data;
@@ -116,15 +115,15 @@ static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
 	*exist = false;
 
 	new_shadow->obj = obj;
-	new_shadow->id = id;
+	new_shadow->id = shadow_type->id;
 
-	if (ctor) {
+	if (shadow_type->ctor) {
 		int err;
 
-		err = ctor(obj, new_shadow->data, ctor_data);
+		err = shadow_type->ctor(obj, new_shadow->data, ctor_data);
 		if (err) {
 			pr_err("Failed to construct shadow variable <%p, %lx> (%d)\n",
-			       obj, id, err);
+			       obj, shadow_type->id, err);
 			return NULL;
 		}
 	}
@@ -136,9 +135,8 @@ static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
 	return new_shadow->data;
 }
 
-static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
-				       size_t size, gfp_t gfp_flags,
-				       klp_shadow_ctor_t ctor, void *ctor_data,
+static void *__klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
+				       size_t size, gfp_t gfp_flags, void *ctor_data,
 				       bool warn_on_exist)
 {
 	struct klp_shadow *new_shadow;
@@ -147,7 +145,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 	unsigned long flags;
 
 	/* Check if the shadow variable already exists */
-	shadow_data = klp_shadow_get(obj, id);
+	shadow_data = klp_shadow_get(obj, shadow_type);
 	if (shadow_data)
 		return shadow_data;
 
@@ -156,22 +154,25 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 	 * More complex setting can be done by @ctor function.  But it is
 	 * called only when the buffer is really used (under klp_shadow_lock).
 	 */
-	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
+	new_shadow = kzalloc(size + sizeof(struct klp_shadow), gfp_flags);
 	if (!new_shadow)
 		return NULL;
 
 	/* Look for <obj, id> again under the lock */
 	spin_lock_irqsave(&klp_shadow_lock, flags);
-	shadow_data = __klp_shadow_get_or_use(obj, id, new_shadow,
-					      ctor, ctor_data, &exist);
+	shadow_data = __klp_shadow_get_or_use(obj, shadow_type,
+					      new_shadow, ctor_data, &exist);
 	spin_unlock_irqrestore(&klp_shadow_lock, flags);
 
-	/* Throw away unused speculative allocation. */
+	/*
+	 * Throw away unused speculative allocation if the shadow variable
+	 * exists or if the ctor function failed.
+	 */
 	if (!shadow_data || exist)
 		kfree(new_shadow);
 
 	if (exist && warn_on_exist) {
-		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
+		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, shadow_type->id);
 		return NULL;
 	}
 
@@ -181,10 +182,9 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 /**
  * klp_shadow_alloc() - allocate and add a new shadow variable
  * @obj:	pointer to parent object
- * @id:		data identifier
+ * @shadow_type: type of the wanted shadow variable
  * @size:	size of attached data
  * @gfp_flags:	GFP mask for allocation
- * @ctor:	custom constructor to initialize the shadow data (optional)
  * @ctor_data:	pointer to any data needed by @ctor (optional)
  *
  * Allocates @size bytes for new shadow variable data using @gfp_flags.
@@ -202,22 +202,21 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
  * Return: the shadow variable data element, NULL on duplicate or
  * failure.
  */
-void *klp_shadow_alloc(void *obj, unsigned long id,
-		       size_t size, gfp_t gfp_flags,
-		       klp_shadow_ctor_t ctor, void *ctor_data)
+void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
+		       size_t size, gfp_t gfp_flags, void *ctor_data)
 {
-	return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
-					 ctor, ctor_data, true);
+	return __klp_shadow_get_or_alloc(obj, shadow_type, size,
+					 gfp_flags, ctor_data,
+					 true);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_alloc);
 
 /**
  * klp_shadow_get_or_alloc() - get existing or allocate a new shadow variable
  * @obj:	pointer to parent object
- * @id:		data identifier
+ * @shadow_type: type of the wanted shadow variable
  * @size:	size of attached data
  * @gfp_flags:	GFP mask for allocation
- * @ctor:	custom constructor to initialize the shadow data (optional)
  * @ctor_data:	pointer to any data needed by @ctor (optional)
  *
  * Returns a pointer to existing shadow data if an <obj, id> shadow
@@ -231,35 +230,33 @@ EXPORT_SYMBOL_GPL(klp_shadow_alloc);
  *
  * Return: the shadow variable data element, NULL on failure.
  */
-void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
-			      size_t size, gfp_t gfp_flags,
-			      klp_shadow_ctor_t ctor, void *ctor_data)
+void *klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
+			      size_t size, gfp_t gfp_flags, void *ctor_data)
 {
-	return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
-					 ctor, ctor_data, false);
+	return __klp_shadow_get_or_alloc(obj, shadow_type, size,
+					 gfp_flags, ctor_data,
+					 false);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_get_or_alloc);
 
 static void klp_shadow_free_struct(struct klp_shadow *shadow,
-				   klp_shadow_dtor_t dtor)
+				   struct klp_shadow_type *shadow_type)
 {
 	hash_del_rcu(&shadow->node);
-	if (dtor)
-		dtor(shadow->obj, shadow->data);
+	if (shadow_type->dtor)
+		shadow_type->dtor(shadow->obj, shadow->data);
 	kfree_rcu(shadow, rcu_head);
 }
 
 /**
  * klp_shadow_free() - detach and free a <obj, id> shadow variable
  * @obj:	pointer to parent object
- * @id:		data identifier
- * @dtor:	custom callback that can be used to unregister the variable
- *		and/or free data that the shadow variable points to (optional)
+ * @shadow_type: type of to be freed shadow variable
  *
  * This function releases the memory for this <obj, id> shadow variable
  * instance, callers should stop referencing it accordingly.
  */
-void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
+void klp_shadow_free(void *obj, struct klp_shadow_type *shadow_type)
 {
 	struct klp_shadow *shadow;
 	unsigned long flags;
@@ -270,8 +267,8 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
 	hash_for_each_possible(klp_shadow_hash, shadow, node,
 			       (unsigned long)obj) {
 
-		if (klp_shadow_match(shadow, obj, id)) {
-			klp_shadow_free_struct(shadow, dtor);
+		if (klp_shadow_match(shadow, obj, shadow_type)) {
+			klp_shadow_free_struct(shadow, shadow_type);
 			break;
 		}
 	}
@@ -280,7 +277,7 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
 }
 EXPORT_SYMBOL_GPL(klp_shadow_free);
 
-static void __klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
+static void __klp_shadow_free_all(struct klp_shadow_type *shadow_type)
 {
 	struct klp_shadow *shadow;
 	int i;
@@ -289,26 +286,24 @@ static void __klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
 
 	/* Delete all <*, id> from hash */
 	hash_for_each(klp_shadow_hash, i, shadow, node) {
-		if (klp_shadow_match(shadow, shadow->obj, id))
-			klp_shadow_free_struct(shadow, dtor);
+		if (klp_shadow_match(shadow, shadow->obj, shadow_type))
+			klp_shadow_free_struct(shadow, shadow_type);
 	}
 }
 
 /**
  * klp_shadow_free_all() - detach and free all <_, id> shadow variables
- * @id:		data identifier
- * @dtor:	custom callback that can be used to unregister the variable
- *		and/or free data that the shadow variable points to (optional)
+ * @shadow_type: type of to be freed shadow variables
  *
  * This function releases the memory for all <_, id> shadow variable
  * instances, callers should stop referencing them accordingly.
  */
-void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
+void klp_shadow_free_all(struct klp_shadow_type *shadow_type)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&klp_shadow_lock, flags);
-	__klp_shadow_free_all(id, dtor);
+	__klp_shadow_free_all(shadow_type);
 	spin_unlock_irqrestore(&klp_shadow_lock, flags);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_free_all);
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index b99116490858..ee47c1fae8e2 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -58,58 +58,64 @@ static int ptr_id(void *ptr)
  * to the kernel log for testing verification.  Don't display raw pointers,
  * but use the ptr_id() value instead.
  */
-static void *shadow_get(void *obj, unsigned long id)
+static void *shadow_get(void *obj, struct klp_shadow_type *shadow_type)
 {
 	int **sv;
 
-	sv = klp_shadow_get(obj, id);
+	sv = klp_shadow_get(obj, shadow_type);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx) = PTR%d\n",
-		__func__, ptr_id(obj), id, ptr_id(sv));
+		__func__, ptr_id(obj), shadow_type->id, ptr_id(sv));
 
 	return sv;
 }
 
-static void *shadow_alloc(void *obj, unsigned long id, size_t size,
-			  gfp_t gfp_flags, klp_shadow_ctor_t ctor,
-			  void *ctor_data)
+static void *shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
+			  size_t size, gfp_t gfp_flags, void *ctor_data)
 {
 	int **var = ctor_data;
 	int **sv;
 
-	sv = klp_shadow_alloc(obj, id, size, gfp_flags, ctor, var);
+	sv = klp_shadow_alloc(obj, shadow_type, size, gfp_flags, var);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n",
-		__func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor),
+		__func__, ptr_id(obj), shadow_type->id, size, &gfp_flags, ptr_id(shadow_type->ctor),
 		ptr_id(*var), ptr_id(sv));
 
 	return sv;
 }
 
-static void *shadow_get_or_alloc(void *obj, unsigned long id, size_t size,
-				 gfp_t gfp_flags, klp_shadow_ctor_t ctor,
-				 void *ctor_data)
+static void *shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
+				 size_t size, gfp_t gfp_flags, void *ctor_data)
 {
 	int **var = ctor_data;
 	int **sv;
 
-	sv = klp_shadow_get_or_alloc(obj, id, size, gfp_flags, ctor, var);
+	sv = klp_shadow_get_or_alloc(obj, shadow_type, size, gfp_flags, var);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n",
-		__func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor),
+		__func__, ptr_id(obj), shadow_type->id, size, &gfp_flags, ptr_id(shadow_type->ctor),
 		ptr_id(*var), ptr_id(sv));
 
 	return sv;
 }
 
-static void shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
+static void shadow_free(void *obj, struct klp_shadow_type *shadow_type)
 {
-	klp_shadow_free(obj, id, dtor);
+	klp_shadow_free(obj, shadow_type);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, dtor=PTR%d)\n",
-		__func__, ptr_id(obj), id, ptr_id(dtor));
+		__func__, ptr_id(obj), shadow_type->id, ptr_id(shadow_type->dtor));
 }
 
-static void shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
+/*
+ * With more than one item to free in the list, order is not determined and
+ * shadow_dtor will not be passed to shadow_free_all() which would make the
+ * test fail. (see pass 6)
+ */
+static bool verbose_dtor = true;
+static void shadow_free_all(struct klp_shadow_type *shadow_type)
 {
-	klp_shadow_free_all(id, dtor);
-	pr_info("klp_%s(id=0x%lx, dtor=PTR%d)\n", __func__, id, ptr_id(dtor));
+	verbose_dtor = false;
+	klp_shadow_free_all(shadow_type);
+	verbose_dtor = true;
+	pr_info("klp_%s(id=0x%lx, dtor=PTR%d)\n", __func__, shadow_type->id, ptr_id(shadow_type->dtor));
 }
 
 
@@ -128,17 +134,14 @@ static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data)
 	return 0;
 }
 
-/*
- * With more than one item to free in the list, order is not determined and
- * shadow_dtor will not be passed to shadow_free_all() which would make the
- * test fail. (see pass 6)
- */
 static void shadow_dtor(void *obj, void *shadow_data)
 {
 	int **sv = shadow_data;
 
-	pr_info("%s(obj=PTR%d, shadow_data=PTR%d)\n",
-		__func__, ptr_id(obj), ptr_id(sv));
+	if (verbose_dtor) {
+		pr_info("%s(obj=PTR%d, shadow_data=PTR%d)\n",
+			__func__, ptr_id(obj), ptr_id(sv));
+	}
 }
 
 /* number of objects we simulate that need shadow vars */
@@ -148,6 +151,18 @@ static void shadow_dtor(void *obj, void *shadow_data)
 #define SV_ID1 0x1234
 #define SV_ID2 0x1235
 
+struct klp_shadow_type shadow_type_1 = {
+	.id = SV_ID1,
+	.ctor = shadow_ctor,
+	.dtor = shadow_dtor,
+};
+
+struct klp_shadow_type shadow_type_2 = {
+	.id = SV_ID2,
+	.ctor = shadow_ctor,
+	.dtor = shadow_dtor,
+};
+
 /*
  * The main test case adds/removes new fields (shadow var) to each of these
  * test structure instances. The last group of fields in the struct represent
@@ -179,7 +194,7 @@ static int test_klp_shadow_vars_init(void)
 	 * With an empty shadow variable hash table, expect not to find
 	 * any matches.
 	 */
-	sv = shadow_get(&objs[0], SV_ID1);
+	sv = shadow_get(&objs[0], &shadow_type_1);
 	if (!sv)
 		pr_info("  got expected NULL result\n");
 
@@ -189,13 +204,13 @@ static int test_klp_shadow_vars_init(void)
 		ptr_id(pnfields1[i]);
 
 		if (i % 2) {
-			sv1[i] = shadow_alloc(&objs[i], SV_ID1,
+			sv1[i] = shadow_alloc(&objs[i], &shadow_type_1,
 					sizeof(pnfields1[i]), GFP_KERNEL,
-					shadow_ctor, &pnfields1[i]);
+					&pnfields1[i]);
 		} else {
-			sv1[i] = shadow_get_or_alloc(&objs[i], SV_ID1,
+			sv1[i] = shadow_get_or_alloc(&objs[i], &shadow_type_1,
 					sizeof(pnfields1[i]), GFP_KERNEL,
-					shadow_ctor, &pnfields1[i]);
+					&pnfields1[i]);
 		}
 		if (!sv1[i]) {
 			ret = -ENOMEM;
@@ -204,8 +219,9 @@ static int test_klp_shadow_vars_init(void)
 
 		pnfields2[i] = &nfields2[i];
 		ptr_id(pnfields2[i]);
-		sv2[i] = shadow_alloc(&objs[i], SV_ID2, sizeof(pnfields2[i]),
-					GFP_KERNEL, shadow_ctor, &pnfields2[i]);
+		sv2[i] = shadow_alloc(&objs[i], &shadow_type_2,
+				      sizeof(pnfields2[i]),
+				      GFP_KERNEL, &pnfields2[i]);
 		if (!sv2[i]) {
 			ret = -ENOMEM;
 			goto out;
@@ -215,7 +231,7 @@ static int test_klp_shadow_vars_init(void)
 	/* pass 2: verify we find allocated svars and where they point to */
 	for (i = 0; i < NUM_OBJS; i++) {
 		/* check the "char" svar for all objects */
-		sv = shadow_get(&objs[i], SV_ID1);
+		sv = shadow_get(&objs[i], &shadow_type_1);
 		if (!sv) {
 			ret = -EINVAL;
 			goto out;
@@ -225,7 +241,7 @@ static int test_klp_shadow_vars_init(void)
 				ptr_id(sv1[i]), ptr_id(*sv1[i]));
 
 		/* check the "int" svar for all objects */
-		sv = shadow_get(&objs[i], SV_ID2);
+		sv = shadow_get(&objs[i], &shadow_type_2);
 		if (!sv) {
 			ret = -EINVAL;
 			goto out;
@@ -240,8 +256,9 @@ static int test_klp_shadow_vars_init(void)
 		pndup[i] = &nfields1[i];
 		ptr_id(pndup[i]);
 
-		sv = shadow_get_or_alloc(&objs[i], SV_ID1, sizeof(pndup[i]),
-					GFP_KERNEL, shadow_ctor, &pndup[i]);
+		sv = shadow_get_or_alloc(&objs[i], &shadow_type_1,
+					 sizeof(pndup[i]),
+					 GFP_KERNEL, &pndup[i]);
 		if (!sv) {
 			ret = -EINVAL;
 			goto out;
@@ -253,15 +270,15 @@ static int test_klp_shadow_vars_init(void)
 
 	/* pass 4: free <objs[*], SV_ID1> pairs of svars, verify removal */
 	for (i = 0; i < NUM_OBJS; i++) {
-		shadow_free(&objs[i], SV_ID1, shadow_dtor); /* 'char' pairs */
-		sv = shadow_get(&objs[i], SV_ID1);
+		shadow_free(&objs[i], &shadow_type_1); /* 'char' pairs */
+		sv = shadow_get(&objs[i], &shadow_type_1);
 		if (!sv)
 			pr_info("  got expected NULL result\n");
 	}
 
 	/* pass 5: check we still find <objs[*], SV_ID2> svar pairs */
 	for (i = 0; i < NUM_OBJS; i++) {
-		sv = shadow_get(&objs[i], SV_ID2);	/* 'int' pairs */
+		sv = shadow_get(&objs[i], &shadow_type_2); /* 'int' pairs */
 		if (!sv) {
 			ret = -EINVAL;
 			goto out;
@@ -272,9 +289,9 @@ static int test_klp_shadow_vars_init(void)
 	}
 
 	/* pass 6: free all the <objs[*], SV_ID2> svar pairs too. */
-	shadow_free_all(SV_ID2, NULL);		/* 'int' pairs */
+	shadow_free_all(&shadow_type_2);		/* 'int' pairs */
 	for (i = 0; i < NUM_OBJS; i++) {
-		sv = shadow_get(&objs[i], SV_ID2);
+		sv = shadow_get(&objs[i], &shadow_type_2);
 		if (!sv)
 			pr_info("  got expected NULL result\n");
 	}
@@ -283,8 +300,8 @@ static int test_klp_shadow_vars_init(void)
 
 	return 0;
 out:
-	shadow_free_all(SV_ID1, NULL);		/* 'char' pairs */
-	shadow_free_all(SV_ID2, NULL);		/* 'int' pairs */
+	shadow_free_all(&shadow_type_1); /* 'char' pairs */
+	shadow_free_all(&shadow_type_2); /* 'int' pairs */
 	free_ptr_list();
 
 	return ret;
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 6701641bf12d..0cc7d1e4b4bc 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -32,6 +32,8 @@
 /* Shadow variable enums */
 #define SV_LEAK		1
 
+static struct klp_shadow_type shadow_leak_type;
+
 /* Allocate new dummies every second */
 #define ALLOC_PERIOD	1
 /* Check for expired dummies after a few new ones have been allocated */
@@ -84,8 +86,8 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
 	if (!leak)
 		goto err_leak;
 
-	shadow_leak = klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
-				       shadow_leak_ctor, &leak);
+	shadow_leak = klp_shadow_alloc(d, &shadow_leak_type, sizeof(leak),
+				       GFP_KERNEL, &leak);
 	if (!shadow_leak) {
 		pr_err("%s: failed to allocate shadow variable for the leaking pointer: dummy @ %p, leak @ %p\n",
 		       __func__, d, leak);
@@ -124,15 +126,21 @@ static void livepatch_fix1_dummy_free(struct dummy *d)
 	 * not exist (ie, dummy structures allocated before this livepatch
 	 * was loaded.)
 	 */
-	shadow_leak = klp_shadow_get(d, SV_LEAK);
+	shadow_leak = klp_shadow_get(d, &shadow_leak_type);
 	if (shadow_leak)
-		klp_shadow_free(d, SV_LEAK, livepatch_fix1_dummy_leak_dtor);
+		klp_shadow_free(d, &shadow_leak_type);
 	else
 		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
 
 	kfree(d);
 }
 
+static struct klp_shadow_type shadow_leak_type = {
+	.id = SV_LEAK,
+	.ctor = shadow_leak_ctor,
+	.dtor = livepatch_fix1_dummy_leak_dtor,
+};
+
 static struct klp_func funcs[] = {
 	{
 		.old_name = "dummy_alloc",
@@ -164,7 +172,7 @@ static int livepatch_shadow_fix1_init(void)
 static void livepatch_shadow_fix1_exit(void)
 {
 	/* Cleanup any existing SV_LEAK shadow variables */
-	klp_shadow_free_all(SV_LEAK, livepatch_fix1_dummy_leak_dtor);
+	klp_shadow_free_all(&shadow_leak_type);
 }
 
 module_init(livepatch_shadow_fix1_init);
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index 361046a4f10c..840100555152 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -33,6 +33,9 @@
 #define SV_LEAK		1
 #define SV_COUNTER	2
 
+static struct klp_shadow_type shadow_leak_type;
+static struct klp_shadow_type shadow_counter_type;
+
 struct dummy {
 	struct list_head list;
 	unsigned long jiffies_expire;
@@ -47,9 +50,8 @@ static bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
 	 * already have a SV_COUNTER shadow variable, then attach a
 	 * new one.
 	 */
-	shadow_count = klp_shadow_get_or_alloc(d, SV_COUNTER,
-				sizeof(*shadow_count), GFP_NOWAIT,
-				NULL, NULL);
+	shadow_count = klp_shadow_get_or_alloc(d, &shadow_counter_type,
+				sizeof(*shadow_count), GFP_NOWAIT, NULL);
 	if (shadow_count)
 		*shadow_count += 1;
 
@@ -72,9 +74,9 @@ static void livepatch_fix2_dummy_free(struct dummy *d)
 	int *shadow_count;
 
 	/* Patch: copy the memory leak patch from the fix1 module. */
-	shadow_leak = klp_shadow_get(d, SV_LEAK);
+	shadow_leak = klp_shadow_get(d, &shadow_leak_type);
 	if (shadow_leak)
-		klp_shadow_free(d, SV_LEAK, livepatch_fix2_dummy_leak_dtor);
+		klp_shadow_free(d, &shadow_leak_type);
 	else
 		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
 
@@ -82,16 +84,25 @@ static void livepatch_fix2_dummy_free(struct dummy *d)
 	 * Patch: fetch the SV_COUNTER shadow variable and display
 	 * the final count.  Detach the shadow variable.
 	 */
-	shadow_count = klp_shadow_get(d, SV_COUNTER);
+	shadow_count = klp_shadow_get(d, &shadow_counter_type);
 	if (shadow_count) {
 		pr_info("%s: dummy @ %p, check counter = %d\n",
 			__func__, d, *shadow_count);
-		klp_shadow_free(d, SV_COUNTER, NULL);
+		klp_shadow_free(d, &shadow_counter_type);
 	}
 
 	kfree(d);
 }
 
+static struct klp_shadow_type shadow_leak_type = {
+	.id = SV_LEAK,
+	.dtor = livepatch_fix2_dummy_leak_dtor,
+};
+
+static struct klp_shadow_type shadow_counter_type = {
+	.id = SV_COUNTER,
+};
+
 static struct klp_func funcs[] = {
 	{
 		.old_name = "dummy_check",
@@ -123,7 +134,7 @@ static int livepatch_shadow_fix2_init(void)
 static void livepatch_shadow_fix2_exit(void)
 {
 	/* Cleanup any existing SV_COUNTER shadow variables */
-	klp_shadow_free_all(SV_COUNTER, NULL);
+	klp_shadow_free_all(&shadow_leak_type);
 }
 
 module_init(livepatch_shadow_fix2_init);
diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh
index e04cb354f56b..01ef65bc1f0c 100755
--- a/tools/testing/selftests/livepatch/test-shadow-vars.sh
+++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh
@@ -67,7 +67,7 @@ $MOD_TEST: klp_shadow_get(obj=PTR9, id=0x1235) = PTR11
 $MOD_TEST:   got expected PTR11 -> PTR10 result
 $MOD_TEST: klp_shadow_get(obj=PTR14, id=0x1235) = PTR16
 $MOD_TEST:   got expected PTR16 -> PTR15 result
-$MOD_TEST: klp_shadow_free_all(id=0x1235, dtor=PTR0)
+$MOD_TEST: klp_shadow_free_all(id=0x1235, dtor=PTR17)
 $MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1235) = PTR0
 $MOD_TEST:   got expected NULL result
 $MOD_TEST: klp_shadow_get(obj=PTR9, id=0x1235) = PTR0
-- 
2.35.3

Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Josh Poimboeuf]

On Fri, Jul 01, 2022 at 04:48:16PM -0300, Marcos Paulo de Souza wrote:
> The shadow variable type will be used in klp_shadow_alloc/get/free
> functions instead of id/ctor/dtor parameters. As a result, all callers
> use the same callbacks consistently[*][**].
> 
> The structure will be used in the next patch that will manage the
> lifetime of shadow variables and execute garbage collection automatically.
> 
> [*] From the user POV, it might have been easier to pass $id instead
>     of pointer to struct klp_shadow_type.
> 
>     The problem is that each livepatch registers its own struct
>     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
>     be unclear what callback should be used. They should be compatible.
> 
>     This problem is gone when each livepatch explicitly uses its
>     own struct klp_shadow_type pointing to its own callbacks.
> 
> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
>     The message must be disabled when called via klp_shadow_free_all()
>     because the ordering of freed variables is not well defined there.
>     It has to be done using another hack after switching to
>     klp_shadow_types.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Invalid SOB chain, see

  Documentation/process/submitting-patches.rst

-- 
Josh

Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Petr Mladek]

On Fri 2022-07-01 16:48:16, Marcos Paulo de Souza wrote:
> The shadow variable type will be used in klp_shadow_alloc/get/free
> functions instead of id/ctor/dtor parameters. As a result, all callers
> use the same callbacks consistently[*][**].
> 
> The structure will be used in the next patch that will manage the
> lifetime of shadow variables and execute garbage collection automatically.
> 
> [*] From the user POV, it might have been easier to pass $id instead
>     of pointer to struct klp_shadow_type.
>
>     The problem is that each livepatch registers its own struct
>     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
>     be unclear what callback should be used. They should be compatible.

This probably is not clear enough. The following might be better:

[*] From the user POV, it might have been easier to pass $id instead
    of pointer to struct klp_shadow_type.

    It would require registering the klp_shadow_type so that
    the klp_shadow API could find ctor/dtor for the given id.
    It actually will be needed for the garbage collection anyway
    because it will define the lifetime of the variables.

    The bigger problem is that the same klp_shadow_type might be
    used by more livepatch modules. Each livepatch module need
    to duplicate the definition of klp_shadow_type and ctor/dtor
    callbacks. The klp_shadow API would need to choose one registered
    copy.

    They definitions should be compatible and they should stay as long
    as the type is registered. But it still feels more safe when
    klp_shadow API callers use struct klp_shadow_type and ctor/dtor
    callbacks defined in the same livepatch module.

>     This problem is gone when each livepatch explicitly uses its
>     own struct klp_shadow_type pointing to its own callbacks.
> 
> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
>     The message must be disabled when called via klp_shadow_free_all()
>     because the ordering of freed variables is not well defined there.
>     It has to be done using another hack after switching to
>     klp_shadow_types.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

This should be:

Co-developed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -156,22 +154,25 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
>  	 * More complex setting can be done by @ctor function.  But it is
>  	 * called only when the buffer is really used (under klp_shadow_lock).
>  	 */
> -	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> +	new_shadow = kzalloc(size + sizeof(struct klp_shadow), gfp_flags);
>  	if (!new_shadow)
>  		return NULL;
>  
>  	/* Look for <obj, id> again under the lock */
>  	spin_lock_irqsave(&klp_shadow_lock, flags);
> -	shadow_data = __klp_shadow_get_or_use(obj, id, new_shadow,
> -					      ctor, ctor_data, &exist);
> +	shadow_data = __klp_shadow_get_or_use(obj, shadow_type,
> +					      new_shadow, ctor_data, &exist);
>  	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>  
> -	/* Throw away unused speculative allocation. */
> +	/*
> +	 * Throw away unused speculative allocation if the shadow variable
> +	 * exists or if the ctor function failed.
> +	 */

The ordering is confusing because it does not match the code. Please,
either use:

	 * Throw away the unused speculative allocation if ctor() failed
	 * or the variable already existed.

or just keep the original comment.

>  	if (!shadow_data || exist)
>  		kfree(new_shadow);
>  
>  	if (exist && warn_on_exist) {
> -		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, shadow_type->id);
>  		return NULL;
>  	}

Best Regards,
Petr

Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Joe Lawrence]

On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> The shadow variable type will be used in klp_shadow_alloc/get/free
> functions instead of id/ctor/dtor parameters. As a result, all callers
> use the same callbacks consistently[*][**].
> 
> The structure will be used in the next patch that will manage the
> lifetime of shadow variables and execute garbage collection automatically.
> 
> [*] From the user POV, it might have been easier to pass $id instead
>     of pointer to struct klp_shadow_type.
> 
>     The problem is that each livepatch registers its own struct
>     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
>     be unclear what callback should be used. They should be compatible.
> 
>     This problem is gone when each livepatch explicitly uses its
>     own struct klp_shadow_type pointing to its own callbacks.
> 
> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
>     The message must be disabled when called via klp_shadow_free_all()
>     because the ordering of freed variables is not well defined there.
>     It has to be done using another hack after switching to
>     klp_shadow_types.
> 

Is the ordering problem new to this patchset?  Shadow variables are
still saved in klp_shadow_hash and I think the only change in this patch
is that we need to compare through shadow_type and not id directly.  Or
does patch 4/4 change behavior here?  Just curious, otherwise this patch
is pretty straightforward.

> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/livepatch.h                     |  29 +++--
>  kernel/livepatch/shadow.c                     | 103 ++++++++---------
>  lib/livepatch/test_klp_shadow_vars.c          | 105 ++++++++++--------
>  samples/livepatch/livepatch-shadow-fix1.c     |  18 ++-
>  samples/livepatch/livepatch-shadow-fix2.c     |  27 +++--
>  .../selftests/livepatch/test-shadow-vars.sh   |   2 +-
>  6 files changed, 163 insertions(+), 121 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 293e29960c6e..79e7bf3b35f6 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -216,15 +216,26 @@ typedef int (*klp_shadow_ctor_t)(void *obj,
>  				 void *ctor_data);
>  typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
>  
> -void *klp_shadow_get(void *obj, unsigned long id);
> -void *klp_shadow_alloc(void *obj, unsigned long id,
> -		       size_t size, gfp_t gfp_flags,
> -		       klp_shadow_ctor_t ctor, void *ctor_data);
> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> -			      size_t size, gfp_t gfp_flags,
> -			      klp_shadow_ctor_t ctor, void *ctor_data);
> -void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> -void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
> +/**
> + * struct klp_shadow_type - shadow variable type used by the klp_object
> + * @id:		shadow variable type indentifier
> + * @ctor:	custom constructor to initialize the shadow data (optional)
> + * @dtor:	custom callback that can be used to unregister the variable
> + *		and/or free data that the shadow variable points to (optional)
> + */
> +struct klp_shadow_type {
> +	unsigned long id;
> +	klp_shadow_ctor_t ctor;
> +	klp_shadow_dtor_t dtor;
> +};
> +
> +void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type);
> +void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
> +		       size_t size, gfp_t gfp_flags, void *ctor_data);
> +void *klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
> +			      size_t size, gfp_t gfp_flags, void *ctor_data);
> +void klp_shadow_free(void *obj, struct klp_shadow_type *shadow_type);
> +void klp_shadow_free_all(struct klp_shadow_type *shadow_type);
>  
>  struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
>  struct klp_state *klp_get_prev_state(unsigned long id);
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> index 79b8646b1d4c..9dcbb626046e 100644
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -63,24 +63,24 @@ struct klp_shadow {
>   * klp_shadow_match() - verify a shadow variable matches given <obj, id>
>   * @shadow:	shadow variable to match
>   * @obj:	pointer to parent object
> - * @id:		data identifier
> + * @shadow_type: type of the wanted shadow variable
>   *
>   * Return: true if the shadow variable matches.
>   */
>  static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> -				unsigned long id)
> +				struct klp_shadow_type *shadow_type)
>  {
> -	return shadow->obj == obj && shadow->id == id;
> +	return shadow->obj == obj && shadow->id == shadow_type->id;

Not sure if I'm being paranoid, but is there any problem if the user
registers two klp_shadow_types with the same id?  I can't find any
obvious logic problems with that, but I don't think the API prevents
this confusing possibility.

>  [ ... snip ... ]
>  

Regards,

-- 
Joe

Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Joe Lawrence]

On 8/25/22 10:50 AM, Joe Lawrence wrote:
> On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
>> The shadow variable type will be used in klp_shadow_alloc/get/free
>> functions instead of id/ctor/dtor parameters. As a result, all callers
>> use the same callbacks consistently[*][**].
>>
>> The structure will be used in the next patch that will manage the
>> lifetime of shadow variables and execute garbage collection automatically.
>>
>> [*] From the user POV, it might have been easier to pass $id instead
>>     of pointer to struct klp_shadow_type.
>>
>>     The problem is that each livepatch registers its own struct
>>     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
>>     be unclear what callback should be used. They should be compatible.
>>
>>     This problem is gone when each livepatch explicitly uses its
>>     own struct klp_shadow_type pointing to its own callbacks.
>>
>> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
>>     The message must be disabled when called via klp_shadow_free_all()
>>     because the ordering of freed variables is not well defined there.
>>     It has to be done using another hack after switching to
>>     klp_shadow_types.
>>
> 
> Is the ordering problem new to this patchset?  Shadow variables are
> still saved in klp_shadow_hash and I think the only change in this patch
> is that we need to compare through shadow_type and not id directly.  Or
> does patch 4/4 change behavior here?  Just curious, otherwise this patch
> is pretty straightforward.
> 
>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>> Signed-off-by: Petr Mladek <pmladek@suse.com>
>> ---
>>  include/linux/livepatch.h                     |  29 +++--
>>  kernel/livepatch/shadow.c                     | 103 ++++++++---------
>>  lib/livepatch/test_klp_shadow_vars.c          | 105 ++++++++++--------
>>  samples/livepatch/livepatch-shadow-fix1.c     |  18 ++-
>>  samples/livepatch/livepatch-shadow-fix2.c     |  27 +++--
>>  .../selftests/livepatch/test-shadow-vars.sh   |   2 +-
>>  6 files changed, 163 insertions(+), 121 deletions(-)
>>
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 293e29960c6e..79e7bf3b35f6 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -216,15 +216,26 @@ typedef int (*klp_shadow_ctor_t)(void *obj,
>>  				 void *ctor_data);
>>  typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
>>  
>> -void *klp_shadow_get(void *obj, unsigned long id);
>> -void *klp_shadow_alloc(void *obj, unsigned long id,
>> -		       size_t size, gfp_t gfp_flags,
>> -		       klp_shadow_ctor_t ctor, void *ctor_data);
>> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
>> -			      size_t size, gfp_t gfp_flags,
>> -			      klp_shadow_ctor_t ctor, void *ctor_data);
>> -void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
>> -void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
>> +/**
>> + * struct klp_shadow_type - shadow variable type used by the klp_object
>> + * @id:		shadow variable type indentifier
>> + * @ctor:	custom constructor to initialize the shadow data (optional)
>> + * @dtor:	custom callback that can be used to unregister the variable
>> + *		and/or free data that the shadow variable points to (optional)
>> + */
>> +struct klp_shadow_type {
>> +	unsigned long id;
>> +	klp_shadow_ctor_t ctor;
>> +	klp_shadow_dtor_t dtor;
>> +};
>> +
>> +void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type);
>> +void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
>> +		       size_t size, gfp_t gfp_flags, void *ctor_data);
>> +void *klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
>> +			      size_t size, gfp_t gfp_flags, void *ctor_data);
>> +void klp_shadow_free(void *obj, struct klp_shadow_type *shadow_type);
>> +void klp_shadow_free_all(struct klp_shadow_type *shadow_type);
>>  
>>  struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
>>  struct klp_state *klp_get_prev_state(unsigned long id);
>> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
>> index 79b8646b1d4c..9dcbb626046e 100644
>> --- a/kernel/livepatch/shadow.c
>> +++ b/kernel/livepatch/shadow.c
>> @@ -63,24 +63,24 @@ struct klp_shadow {
>>   * klp_shadow_match() - verify a shadow variable matches given <obj, id>
>>   * @shadow:	shadow variable to match
>>   * @obj:	pointer to parent object
>> - * @id:		data identifier
>> + * @shadow_type: type of the wanted shadow variable
>>   *
>>   * Return: true if the shadow variable matches.
>>   */
>>  static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
>> -				unsigned long id)
>> +				struct klp_shadow_type *shadow_type)
>>  {
>> -	return shadow->obj == obj && shadow->id == id;
>> +	return shadow->obj == obj && shadow->id == shadow_type->id;
> 
> Not sure if I'm being paranoid, but is there any problem if the user
> registers two klp_shadow_types with the same id?  I can't find any
> obvious logic problems with that, but I don't think the API prevents
> this confusing possibility.
> 

Ah n/m, I think I see now that I'm reading patch 4/4, it's
klp_shadow_type_get_reg() is going to look for an existing
shadow_type_reg->id first.

-- 
Joe

Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Petr Mladek]

On Thu 2022-08-25 10:54:18, Joe Lawrence wrote:
> On 8/25/22 10:50 AM, Joe Lawrence wrote:
> > On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> >> The shadow variable type will be used in klp_shadow_alloc/get/free
> >> functions instead of id/ctor/dtor parameters. As a result, all callers
> >> use the same callbacks consistently[*][**].
> >>
> >> The structure will be used in the next patch that will manage the
> >> lifetime of shadow variables and execute garbage collection automatically.
> >>
> >> [*] From the user POV, it might have been easier to pass $id instead
> >>     of pointer to struct klp_shadow_type.
> >>
> >>     The problem is that each livepatch registers its own struct
> >>     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
> >>     be unclear what callback should be used. They should be compatible.
> >>
> >>     This problem is gone when each livepatch explicitly uses its
> >>     own struct klp_shadow_type pointing to its own callbacks.
> >>
> >> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
> >>     The message must be disabled when called via klp_shadow_free_all()
> >>     because the ordering of freed variables is not well defined there.
> >>     It has to be done using another hack after switching to
> >>     klp_shadow_types.
> >>
> > 
> > Is the ordering problem new to this patchset?  Shadow variables are
> > still saved in klp_shadow_hash and I think the only change in this patch
> > is that we need to compare through shadow_type and not id directly.  Or
> > does patch 4/4 change behavior here?  Just curious, otherwise this patch
> > is pretty straightforward.
> > 
> >> --- a/include/linux/livepatch.h
> >> +++ b/include/linux/livepatch.h
> >> @@ -216,15 +216,26 @@ typedef int (*klp_shadow_ctor_t)(void *obj,
> >>  				 void *ctor_data);
> >>  typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
> >>  
> >> -void *klp_shadow_get(void *obj, unsigned long id);
> >> -void *klp_shadow_alloc(void *obj, unsigned long id,
> >> -		       size_t size, gfp_t gfp_flags,
> >> -		       klp_shadow_ctor_t ctor, void *ctor_data);
> >> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> >> -			      size_t size, gfp_t gfp_flags,
> >> -			      klp_shadow_ctor_t ctor, void *ctor_data);
> >> -void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> >> -void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
> >> +/**
> >> + * struct klp_shadow_type - shadow variable type used by the klp_object
> >> + * @id:		shadow variable type indentifier
> >> + * @ctor:	custom constructor to initialize the shadow data (optional)
> >> + * @dtor:	custom callback that can be used to unregister the variable
> >> + *		and/or free data that the shadow variable points to (optional)
> >> + */
> >> +struct klp_shadow_type {
> >> +	unsigned long id;
> >> +	klp_shadow_ctor_t ctor;
> >> +	klp_shadow_dtor_t dtor;
> >> +};
> >> +
> >> +void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type);
> >> +void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
> >> +		       size_t size, gfp_t gfp_flags, void *ctor_data);
> >> +void *klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
> >> +			      size_t size, gfp_t gfp_flags, void *ctor_data);
> >> +void klp_shadow_free(void *obj, struct klp_shadow_type *shadow_type);
> >> +void klp_shadow_free_all(struct klp_shadow_type *shadow_type);
> >>  
> >>  struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
> >>  struct klp_state *klp_get_prev_state(unsigned long id);
> >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> >> index 79b8646b1d4c..9dcbb626046e 100644
> >> --- a/kernel/livepatch/shadow.c
> >> +++ b/kernel/livepatch/shadow.c
> >> @@ -63,24 +63,24 @@ struct klp_shadow {
> >>   * klp_shadow_match() - verify a shadow variable matches given <obj, id>
> >>   * @shadow:	shadow variable to match
> >>   * @obj:	pointer to parent object
> >> - * @id:		data identifier
> >> + * @shadow_type: type of the wanted shadow variable
> >>   *
> >>   * Return: true if the shadow variable matches.
> >>   */
> >>  static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> >> -				unsigned long id)
> >> +				struct klp_shadow_type *shadow_type)
> >>  {
> >> -	return shadow->obj == obj && shadow->id == id;
> >> +	return shadow->obj == obj && shadow->id == shadow_type->id;
> > 
> > Not sure if I'm being paranoid, but is there any problem if the user
> > registers two klp_shadow_types with the same id?  I can't find any
> > obvious logic problems with that, but I don't think the API prevents
> > this confusing possibility.

Great question!

> Ah n/m, I think I see now that I'm reading patch 4/4, it's
> klp_shadow_type_get_reg() is going to look for an existing
> shadow_type_reg->id first.

The purpose of klp_shadow_type_get_reg() is a bit different.
It decides whether the given klp_shadow_type is registered
for the first time or we just need to increase the refcount.

It means that it is actually possible to register two
different klp_shadow_types using the same ID.

Well, I am not sure if we could do better. We could not compare
pointers of @ctor and @dtor callbacks. The very same klp_shadow_type
can be registered from more livepatches and each livepatch need
to have its own implementation of @ctor and @dtor. It is the purpose
of the refcounting.

In each case, it does not make things worse. The previous API
allowed to combine any ID with any @ctor or @dtor.

One idea. We could define the type by "name" and "id". The @name
might be created by stringification of the structure that defines
the klp_shadow_type. It might be checked during registration
and unregistration. But I am not sure if it is worth the effort.

Best Regards,
Petr

Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Petr Mladek]

On Thu 2022-08-25 10:50:20, Joe Lawrence wrote:
> On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> > The shadow variable type will be used in klp_shadow_alloc/get/free
> > functions instead of id/ctor/dtor parameters. As a result, all callers
> > use the same callbacks consistently[*][**].
> > 
> > The structure will be used in the next patch that will manage the
> > lifetime of shadow variables and execute garbage collection automatically.
> > 
> > [*] From the user POV, it might have been easier to pass $id instead
> >     of pointer to struct klp_shadow_type.
> > 
> >     The problem is that each livepatch registers its own struct
> >     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
> >     be unclear what callback should be used. They should be compatible.
> > 
> >     This problem is gone when each livepatch explicitly uses its
> >     own struct klp_shadow_type pointing to its own callbacks.
> > 
> > [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
> >     The message must be disabled when called via klp_shadow_free_all()
> >     because the ordering of freed variables is not well defined there.
> >     It has to be done using another hack after switching to
> >     klp_shadow_types.
> > 
> 
> Is the ordering problem new to this patchset?  Shadow variables are
> still saved in klp_shadow_hash and I think the only change in this patch
> is that we need to compare through shadow_type and not id directly.  Or
> does patch 4/4 change behavior here?  Just curious, otherwise this patch
> is pretty straightforward.

The problem is old. klp_shadow_free_all() uses hash_for_each(). It
iterates the hashes sorted by the hash value. The tested arrays are
on stack. The address of the stack is different in every run.
As a result, the hash is always different and the pointers
are sorted in different order.

Best Regards,
Petr

[PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Marcos Paulo de Souza]

The life of shadow variables is not completely trivial to maintain.
They might be used by more livepatches and more livepatched objects.
They should stay as long as there is any user.

In practice, it requires to implement reference counting in callbacks
of all users. They should register all the user and remove the shadow
variables only when there is no user left.

This patch hides the reference counting into the klp_shadow API.
The counter is connected with the shadow variable @id. It requires
an API to take and release the reference. The release function also
calls the related dtor() when defined.

An easy solution would be to add some get_ref()/put_ref() API.
But it would need to get called from pre()/post_un() callbacks.
It might be easy to forget a callback and make it wrong.

A more safe approach is to associate the klp_shadow_type with
klp_objects that use the shadow variables. The livepatch core
code might then handle the reference counters on background.

The shadow variable type might then be added into a new @shadow_types
member of struct klp_object. They will get then automatically registered
and unregistered when the object is being livepatched. The registration
increments the reference count. Unregistration decreases the reference
count. All shadow variables of the given type are freed when the reference
count reaches zero.

All klp_shadow_alloc/get/free functions also checks whether the requested
type is registered. It will help to catch missing registration and might
also help to catch eventual races.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h                 |  21 ++++
 kernel/livepatch/core.c                   |  39 +++++++
 kernel/livepatch/core.h                   |   1 +
 kernel/livepatch/shadow.c                 | 124 ++++++++++++++++++++++
 kernel/livepatch/transition.c             |   4 +-
 lib/livepatch/test_klp_shadow_vars.c      |  18 +++-
 samples/livepatch/livepatch-shadow-fix1.c |   8 +-
 samples/livepatch/livepatch-shadow-fix2.c |   9 +-
 8 files changed, 214 insertions(+), 10 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 79e7bf3b35f6..cb65de831684 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -100,11 +100,14 @@ struct klp_callbacks {
 	bool post_unpatch_enabled;
 };
 
+struct klp_shadow_type;
+
 /**
  * struct klp_object - kernel object structure for live patching
  * @name:	module name (or NULL for vmlinux)
  * @funcs:	function entries for functions to be patched in the object
  * @callbacks:	functions to be executed pre/post (un)patching
+ * @shadow_types: shadow variable types used by the livepatch for the klp_object
  * @kobj:	kobject for sysfs resources
  * @func_list:	dynamic list of the function entries
  * @node:	list node for klp_patch obj_list
@@ -118,6 +121,7 @@ struct klp_object {
 	const char *name;
 	struct klp_func *funcs;
 	struct klp_callbacks callbacks;
+	struct klp_shadow_type **shadow_types;
 
 	/* internal */
 	struct kobject kobj;
@@ -222,13 +226,30 @@ typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
  * @ctor:	custom constructor to initialize the shadow data (optional)
  * @dtor:	custom callback that can be used to unregister the variable
  *		and/or free data that the shadow variable points to (optional)
+ * @registered: flag indicating if the variable was successfully registered
+ *
+ * All shadow variables used by the livepatch for the related klp_object
+ * must be listed here so that they are registered when the livepatch
+ * and the module is loaded. Otherwise, it will not be possible to
+ * allocated them.
  */
 struct klp_shadow_type {
 	unsigned long id;
 	klp_shadow_ctor_t ctor;
 	klp_shadow_dtor_t dtor;
+
+	/* internal */
+	bool registered;
 };
 
+#define klp_for_each_shadow_type(obj, shadow_type, i)			\
+	for (shadow_type = obj->shadow_types ? obj->shadow_types[0] : NULL, i = 1; \
+	     shadow_type; \
+	     shadow_type = obj->shadow_types[i++])
+
+int klp_shadow_register(struct klp_shadow_type *shadow_type);
+void klp_shadow_unregister(struct klp_shadow_type *shadow_type);
+
 void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type);
 void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
 		       size_t size, gfp_t gfp_flags, void *ctor_data);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc475e62279d..1d59b5652561 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -910,6 +910,30 @@ static int klp_init_patch(struct klp_patch *patch)
 	return 0;
 }
 
+void klp_unregister_shadow_types(struct klp_object *obj)
+{
+	struct klp_shadow_type *shadow_type;
+	int i;
+
+	klp_for_each_shadow_type(obj, shadow_type, i) {
+		klp_shadow_unregister(shadow_type);
+	}
+}
+
+static int klp_register_shadow_types(struct klp_object *obj)
+{
+	struct klp_shadow_type *shadow_type;
+	int i, ret;
+
+	klp_for_each_shadow_type(obj, shadow_type, i) {
+		ret = klp_shadow_register(shadow_type);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -970,6 +994,13 @@ static int __klp_enable_patch(struct klp_patch *patch)
 		if (!klp_is_object_loaded(obj))
 			continue;
 
+		ret = klp_register_shadow_types(obj);
+		if (ret) {
+			pr_warn("failed to register shadow types for object '%s'\n",
+				klp_is_module(obj) ? obj->name : "vmlinux");
+			goto err;
+		}
+
 		ret = klp_pre_patch_callback(obj);
 		if (ret) {
 			pr_warn("pre-patch callback failed for object '%s'\n",
@@ -1154,6 +1185,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 			klp_unpatch_object(obj);
 
 			klp_post_unpatch_callback(obj);
+			klp_unregister_shadow_types(obj);
 
 			klp_free_object_loaded(obj);
 			break;
@@ -1200,6 +1232,13 @@ int klp_module_coming(struct module *mod)
 			pr_notice("applying patch '%s' to loading module '%s'\n",
 				  patch->mod->name, obj->mod->name);
 
+			ret = klp_register_shadow_types(obj);
+			if (ret) {
+				pr_warn("failed to register shadow types for object '%s'\n",
+					obj->name);
+				goto err;
+			}
+
 			ret = klp_pre_patch_callback(obj);
 			if (ret) {
 				pr_warn("pre-patch callback failed for object '%s'\n",
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 38209c7361b6..0b68f2407a82 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -13,6 +13,7 @@ extern struct list_head klp_patches;
 #define klp_for_each_patch(patch)	\
 	list_for_each_entry(patch, &klp_patches, list)
 
+void klp_unregister_shadow_types(struct klp_object *obj);
 void klp_free_patch_async(struct klp_patch *patch);
 void klp_free_replaced_patches_async(struct klp_patch *new_patch);
 void klp_unpatch_replaced_patches(struct klp_patch *new_patch);
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index 9dcbb626046e..73a2715900d6 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -34,6 +34,7 @@
 #include <linux/hashtable.h>
 #include <linux/slab.h>
 #include <linux/livepatch.h>
+#include "core.h"
 
 static DEFINE_HASHTABLE(klp_shadow_hash, 12);
 
@@ -59,6 +60,22 @@ struct klp_shadow {
 	char data[];
 };
 
+/**
+ * struct klp_shadow_type_reg - information about a registered shadow
+ *	variable type
+ * @id:		shadow variable type indentifier
+ * @count:	reference counter
+ * @list:	list node for list of registered shadow variable types
+ */
+struct klp_shadow_type_reg {
+	unsigned long id;
+	int ref_cnt;
+	struct list_head list;
+};
+
+/* List of registered shadow variable types */
+static LIST_HEAD(klp_shadow_types);
+
 /**
  * klp_shadow_match() - verify a shadow variable matches given <obj, id>
  * @shadow:	shadow variable to match
@@ -84,6 +101,13 @@ void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type)
 {
 	struct klp_shadow *shadow;
 
+	/* Just the best effort. Can't take @klp_shadow_lock here. */
+	if (!shadow_type->registered) {
+		pr_err("Trying to get shadow variable of non-registered type: %lu\n",
+		       shadow_type->id);
+		return NULL;
+	}
+
 	rcu_read_lock();
 
 	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
@@ -307,3 +331,103 @@ void klp_shadow_free_all(struct klp_shadow_type *shadow_type)
 	spin_unlock_irqrestore(&klp_shadow_lock, flags);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_free_all);
+
+static struct klp_shadow_type_reg *
+klp_shadow_type_get_reg(struct klp_shadow_type *shadow_type)
+{
+	struct klp_shadow_type_reg *shadow_type_reg;
+	lockdep_assert_held(&klp_shadow_lock);
+
+	list_for_each_entry(shadow_type_reg, &klp_shadow_types, list) {
+		if (shadow_type_reg->id == shadow_type->id)
+			return shadow_type_reg;
+	}
+
+	return NULL;
+}
+
+/**
+ * klp_shadow_register() - register self for using a given data identifier
+ * @shadow_type:	shadow type to be registered
+ *
+ * Tell the system that the related module (livepatch) is going to use a given
+ * shadow variable ID. It allows to check and maintain lifetime of shadow
+ * variables.
+ *
+ * Return: 0 on suceess, -ENOMEM when there is not enough memory.
+ */
+int klp_shadow_register(struct klp_shadow_type *shadow_type)
+{
+	struct klp_shadow_type_reg *shadow_type_reg;
+	struct klp_shadow_type_reg *new_shadow_type_reg;
+
+	new_shadow_type_reg =
+		kzalloc(sizeof(struct klp_shadow_type_reg), GFP_KERNEL);
+	if (!new_shadow_type_reg)
+		return -ENOMEM;
+
+	spin_lock_irq(&klp_shadow_lock);
+
+	if (shadow_type->registered) {
+		pr_err("Trying to register shadow variable type that is already registered: %lu",
+		       shadow_type->id);
+		kfree(new_shadow_type_reg);
+		goto out;
+	}
+
+	shadow_type_reg = klp_shadow_type_get_reg(shadow_type);
+	if (!shadow_type_reg) {
+		shadow_type_reg = new_shadow_type_reg;
+		shadow_type_reg->id = shadow_type->id;
+		list_add(&shadow_type_reg->list, &klp_shadow_types);
+	} else {
+		kfree(new_shadow_type_reg);
+	}
+
+	shadow_type_reg->ref_cnt++;
+	shadow_type->registered = true;
+out:
+	spin_unlock_irq(&klp_shadow_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_register);
+
+/**
+ * klp_shadow_unregister() - unregister the give shadow variable type
+ * @shadow_type:	shadow type to be unregistered
+ *
+ * Tell the system that a given shadow variable ID is not longer be used by
+ * the caller (livepatch module). All existing shadow variables are freed
+ * when it was the last registered user.
+ */
+void klp_shadow_unregister(struct klp_shadow_type *shadow_type)
+{
+	struct klp_shadow_type_reg *shadow_type_reg;
+
+	spin_lock_irq(&klp_shadow_lock);
+
+	if (!shadow_type->registered) {
+		pr_err("Trying to unregister shadow variable type that is not registered: %lu",
+		       shadow_type->id);
+		goto out;
+	}
+
+	shadow_type_reg = klp_shadow_type_get_reg(shadow_type);
+	if (!shadow_type_reg) {
+		pr_err("Can't find shadow variable type registration: %lu", shadow_type->id);
+		goto out;
+	}
+
+	shadow_type->registered = false;
+	shadow_type_reg->ref_cnt--;
+
+	if (!shadow_type_reg->ref_cnt) {
+		__klp_shadow_free_all(shadow_type);
+		list_del(&shadow_type_reg->list);
+		kfree(shadow_type_reg);
+	}
+out:
+	spin_unlock_irq(&klp_shadow_lock);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_unregister);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 5d03a2ad1066..26f688c026c6 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -123,8 +123,10 @@ static void klp_complete_transition(void)
 			continue;
 		if (klp_target_state == KLP_PATCHED)
 			klp_post_patch_callback(obj);
-		else if (klp_target_state == KLP_UNPATCHED)
+		else if (klp_target_state == KLP_UNPATCHED) {
 			klp_post_unpatch_callback(obj);
+			klp_unregister_shadow_types(obj);
+		}
 	}
 
 	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index ee47c1fae8e2..6de1f6d11bcf 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -188,6 +188,17 @@ static int test_klp_shadow_vars_init(void)
 	int ret;
 	int i;
 
+	/* Registered manually since we don't have a klp_object instance. */
+	ret = klp_shadow_register(&shadow_type_1);
+	if (ret)
+		return ret;
+
+	ret = klp_shadow_register(&shadow_type_2);
+	if (ret) {
+		klp_shadow_unregister(&shadow_type_1);
+		return ret;
+	}
+
 	ptr_id(NULL);
 
 	/*
@@ -296,12 +307,9 @@ static int test_klp_shadow_vars_init(void)
 			pr_info("  got expected NULL result\n");
 	}
 
-	free_ptr_list();
-
-	return 0;
 out:
-	shadow_free_all(&shadow_type_1); /* 'char' pairs */
-	shadow_free_all(&shadow_type_2); /* 'int' pairs */
+	klp_shadow_unregister(&shadow_type_1); /* 'char' pairs */
+	klp_shadow_unregister(&shadow_type_2); /* 'int' pairs */
 	free_ptr_list();
 
 	return ret;
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 0cc7d1e4b4bc..6718df9ec14b 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -141,6 +141,11 @@ static struct klp_shadow_type shadow_leak_type = {
 	.dtor = livepatch_fix1_dummy_leak_dtor,
 };
 
+struct klp_shadow_type *shadow_types[] = {
+	&shadow_leak_type,
+	NULL
+};
+
 static struct klp_func funcs[] = {
 	{
 		.old_name = "dummy_alloc",
@@ -156,6 +161,7 @@ static struct klp_object objs[] = {
 	{
 		.name = "livepatch_shadow_mod",
 		.funcs = funcs,
+		.shadow_types = shadow_types,
 	}, { }
 };
 
@@ -171,8 +177,6 @@ static int livepatch_shadow_fix1_init(void)
 
 static void livepatch_shadow_fix1_exit(void)
 {
-	/* Cleanup any existing SV_LEAK shadow variables */
-	klp_shadow_free_all(&shadow_leak_type);
 }
 
 module_init(livepatch_shadow_fix1_init);
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index 840100555152..290c7e3f96b0 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -103,6 +103,12 @@ static struct klp_shadow_type shadow_counter_type = {
 	.id = SV_COUNTER,
 };
 
+struct klp_shadow_type *shadow_types[] = {
+	&shadow_leak_type,
+	&shadow_counter_type,
+	NULL
+};
+
 static struct klp_func funcs[] = {
 	{
 		.old_name = "dummy_check",
@@ -118,6 +124,7 @@ static struct klp_object objs[] = {
 	{
 		.name = "livepatch_shadow_mod",
 		.funcs = funcs,
+		.shadow_types = shadow_types,
 	}, { }
 };
 
@@ -133,8 +140,6 @@ static int livepatch_shadow_fix2_init(void)
 
 static void livepatch_shadow_fix2_exit(void)
 {
-	/* Cleanup any existing SV_COUNTER shadow variables */
-	klp_shadow_free_all(&shadow_leak_type);
 }
 
 module_init(livepatch_shadow_fix2_init);
-- 
2.35.3

Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Petr Mladek]

On Fri 2022-07-01 16:48:17, Marcos Paulo de Souza wrote:
> The life of shadow variables is not completely trivial to maintain.
> They might be used by more livepatches and more livepatched objects.
> They should stay as long as there is any user.
> 
> In practice, it requires to implement reference counting in callbacks
> of all users. They should register all the user and remove the shadow

s/all the user/all users/

> variables only when there is no user left.
> 
> This patch hides the reference counting into the klp_shadow API.
> The counter is connected with the shadow variable @id. It requires
> an API to take and release the reference. The release function also
> calls the related dtor() when defined.
> 
> An easy solution would be to add some get_ref()/put_ref() API.
> But it would need to get called from pre()/post_un() callbacks.
> It might be easy to forget a callback and make it wrong.
> 
> A more safe approach is to associate the klp_shadow_type with
> klp_objects that use the shadow variables. The livepatch core
> code might then handle the reference counters on background.
> 
> The shadow variable type might then be added into a new @shadow_types
> member of struct klp_object. They will get then automatically registered
> and unregistered when the object is being livepatched. The registration
> increments the reference count. Unregistration decreases the reference
> count. All shadow variables of the given type are freed when the reference
> count reaches zero.
> 
> All klp_shadow_alloc/get/free functions also checks whether the requested
> type is registered. It will help to catch missing registration and might
> also help to catch eventual races.
> 
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -222,13 +226,30 @@ typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
>   * @ctor:	custom constructor to initialize the shadow data (optional)
>   * @dtor:	custom callback that can be used to unregister the variable
>   *		and/or free data that the shadow variable points to (optional)
> + * @registered: flag indicating if the variable was successfully registered
> + *
> + * All shadow variables used by the livepatch for the related klp_object
> + * must be listed here so that they are registered when the livepatch
> + * and the module is loaded. Otherwise, it will not be possible to
> + * allocated them.

s/allocated/allocate/

>   */
>  struct klp_shadow_type {
>  	unsigned long id;
>  	klp_shadow_ctor_t ctor;
>  	klp_shadow_dtor_t dtor;
> +
> +	/* internal */
> +	bool registered;
>  };
>  
> +#define klp_for_each_shadow_type(obj, shadow_type, i)			\
> +	for (shadow_type = obj->shadow_types ? obj->shadow_types[0] : NULL, i = 1; \
> +	     shadow_type; \
> +	     shadow_type = obj->shadow_types[i++])
> +

> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -307,3 +331,103 @@ void klp_shadow_free_all(struct klp_shadow_type *shadow_type)
>  	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(klp_shadow_free_all);
> +
> +static struct klp_shadow_type_reg *
> +klp_shadow_type_get_reg(struct klp_shadow_type *shadow_type)
> +{
> +	struct klp_shadow_type_reg *shadow_type_reg;
> +	lockdep_assert_held(&klp_shadow_lock);
> +
> +	list_for_each_entry(shadow_type_reg, &klp_shadow_types, list) {
> +		if (shadow_type_reg->id == shadow_type->id)
> +			return shadow_type_reg;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * klp_shadow_register() - register self for using a given data identifier

This is a relic from a former version. It should be something like:

 * klp_shadow_register() - register the given shadow variable type


> + * @shadow_type:	shadow type to be registered
> + *
> + * Tell the system that the related module (livepatch) is going to use a given
> + * shadow variable ID. It allows to check and maintain lifetime of shadow
> + * variables.

Same here. It should be:

 * Tell the system that the given shadow variable ID is going to be used by
 * the caller (livepatch module). It allows to check and maintain the lifetime
 * of shadow variables.

> + *
> + * Return: 0 on suceess, -ENOMEM when there is not enough memory.
> + */
> +int klp_shadow_register(struct klp_shadow_type *shadow_type)
> +{
> +	struct klp_shadow_type_reg *shadow_type_reg;
> +	struct klp_shadow_type_reg *new_shadow_type_reg;
> +

The code looks good to me. With the above changes:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Joe Lawrence]

On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> The life of shadow variables is not completely trivial to maintain.
> They might be used by more livepatches and more livepatched objects.
> They should stay as long as there is any user.
> 
> In practice, it requires to implement reference counting in callbacks
> of all users. They should register all the user and remove the shadow
> variables only when there is no user left.
> 
> This patch hides the reference counting into the klp_shadow API.
> The counter is connected with the shadow variable @id. It requires
> an API to take and release the reference. The release function also
> calls the related dtor() when defined.
> 
> An easy solution would be to add some get_ref()/put_ref() API.
> But it would need to get called from pre()/post_un() callbacks.
> It might be easy to forget a callback and make it wrong.
> 
> A more safe approach is to associate the klp_shadow_type with
> klp_objects that use the shadow variables. The livepatch core
> code might then handle the reference counters on background.
> 
> The shadow variable type might then be added into a new @shadow_types
> member of struct klp_object. They will get then automatically registered
> and unregistered when the object is being livepatched. The registration
> increments the reference count. Unregistration decreases the reference
> count. All shadow variables of the given type are freed when the reference
> count reaches zero.
> 
> All klp_shadow_alloc/get/free functions also checks whether the requested
> type is registered. It will help to catch missing registration and might
> also help to catch eventual races.
> 

If I understand the patchset correctly, the last patch consolidated
id/ctor/dtor into klp_shadow_type structure, then this one formally
associates the new structure with a klp_object so that it bumps up /
down a simple reference count automatically.  That count is now checked
before calling the dtor/removal of any matching shadow variable.  So far
so good.

How does this play out for the following scenario:

- atomic replace livepatches, accumulative versions
- livepatch v1 : registers a klp_shadow_type (id=1), creates a few
shadow vars
- livepatch v2 : also uses klp_shadow_type (id=1) and creates a few
shadow vars

Since the first livepatch registered the klp_shadow_type, its ctor/dtor
pair will be used for all id=1 shadow variables, including those created
by the subsequent livepatches, right?


> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/livepatch.h                 |  21 ++++
>  kernel/livepatch/core.c                   |  39 +++++++
>  kernel/livepatch/core.h                   |   1 +
>  kernel/livepatch/shadow.c                 | 124 ++++++++++++++++++++++
>  kernel/livepatch/transition.c             |   4 +-
>  lib/livepatch/test_klp_shadow_vars.c      |  18 +++-
>  samples/livepatch/livepatch-shadow-fix1.c |   8 +-
>  samples/livepatch/livepatch-shadow-fix2.c |   9 +-
>  8 files changed, 214 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 79e7bf3b35f6..cb65de831684 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -100,11 +100,14 @@ struct klp_callbacks {
>  	bool post_unpatch_enabled;
>  };
>  
> +struct klp_shadow_type;
> +
>  /**
>   * struct klp_object - kernel object structure for live patching
>   * @name:	module name (or NULL for vmlinux)
>   * @funcs:	function entries for functions to be patched in the object
>   * @callbacks:	functions to be executed pre/post (un)patching
> + * @shadow_types: shadow variable types used by the livepatch for the klp_object
>   * @kobj:	kobject for sysfs resources
>   * @func_list:	dynamic list of the function entries
>   * @node:	list node for klp_patch obj_list
> @@ -118,6 +121,7 @@ struct klp_object {
>  	const char *name;
>  	struct klp_func *funcs;
>  	struct klp_callbacks callbacks;
> +	struct klp_shadow_type **shadow_types;
>  

Hmm.  The implementation of shadow_types inside klp_object might be
difficult to integrate into kpatch-build.  For kpatches, we do utilize
the kernel's shadow variable API directly, but at kpatch author time we
don't have any of klp_patch objects in hand -- those are generated by
template after the binary before/after comparison is completed.

My first thought is that kpatch-build might associate any shadow
variables in foo.c with its parent object (vmlinux, foo.ko, etc.),
however that may not always be 100% accurate.

In fact, we have occasionally used shadow variables with <id, 0> to
create one off variables not associated with specific code or data
structures, but rather the presence of livepatch.  This is (too) briefly
mentioned in "Other use-cases" section of the shadow variable Documentation.

-- 
Joe

Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Petr Mladek]

On Thu 2022-08-25 12:26:25, Joe Lawrence wrote:
> On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> > The life of shadow variables is not completely trivial to maintain.
> > They might be used by more livepatches and more livepatched objects.
> > They should stay as long as there is any user.
> > 
> > In practice, it requires to implement reference counting in callbacks
> > of all users. They should register all the user and remove the shadow
> > variables only when there is no user left.
> > 
> > This patch hides the reference counting into the klp_shadow API.
> > The counter is connected with the shadow variable @id. It requires
> > an API to take and release the reference. The release function also
> > calls the related dtor() when defined.
> > 
> > An easy solution would be to add some get_ref()/put_ref() API.
> > But it would need to get called from pre()/post_un() callbacks.
> > It might be easy to forget a callback and make it wrong.
> > 
> > A more safe approach is to associate the klp_shadow_type with
> > klp_objects that use the shadow variables. The livepatch core
> > code might then handle the reference counters on background.
> > 
> > The shadow variable type might then be added into a new @shadow_types
> > member of struct klp_object. They will get then automatically registered
> > and unregistered when the object is being livepatched. The registration
> > increments the reference count. Unregistration decreases the reference
> > count. All shadow variables of the given type are freed when the reference
> > count reaches zero.
> > 
> > All klp_shadow_alloc/get/free functions also checks whether the requested
> > type is registered. It will help to catch missing registration and might
> > also help to catch eventual races.
> > 
> 
> If I understand the patchset correctly, the last patch consolidated
> id/ctor/dtor into klp_shadow_type structure, then this one formally
> associates the new structure with a klp_object so that it bumps up /
> down a simple reference count automatically.  That count is now checked
> before calling the dtor/removal of any matching shadow variable.  So far
> so good.
> 
> How does this play out for the following scenario:
> 
> - atomic replace livepatches, accumulative versions
> - livepatch v1 : registers a klp_shadow_type (id=1), creates a few
> shadow vars
> - livepatch v2 : also uses klp_shadow_type (id=1) and creates a few
> shadow vars
> 
> Since the first livepatch registered the klp_shadow_type, its ctor/dtor
> pair will be used for all id=1 shadow variables, including those created
> by the subsequent livepatches, right?

The klp_shadow_*alloc() and klp_shdow_free*() APIs pass
struct *klp_shadow_type instead of the ID. The structure is
defined in each livepatch that is using this shadow variable.
As a result, each livepatch code is using @ctor/@dtor defined
in its own livepatch module. It actually behaves the same as before.

I though about storing pointers to @ctors/@dtors of all
registered klp_shadow_types from all livepatches. But it
would hard to choose which one should be used.

Note that some implementation might disappear when the livepatch
is replaced and the module removed.

So, it is easier when each caller passes its own structure
with its own implementation. They should be compatible anyway.

> 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  include/linux/livepatch.h                 |  21 ++++
> >  kernel/livepatch/core.c                   |  39 +++++++
> >  kernel/livepatch/core.h                   |   1 +
> >  kernel/livepatch/shadow.c                 | 124 ++++++++++++++++++++++
> >  kernel/livepatch/transition.c             |   4 +-
> >  lib/livepatch/test_klp_shadow_vars.c      |  18 +++-
> >  samples/livepatch/livepatch-shadow-fix1.c |   8 +-
> >  samples/livepatch/livepatch-shadow-fix2.c |   9 +-
> >  8 files changed, 214 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 79e7bf3b35f6..cb65de831684 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -100,11 +100,14 @@ struct klp_callbacks {
> >  	bool post_unpatch_enabled;
> >  };
> >  
> > +struct klp_shadow_type;
> > +
> >  /**
> >   * struct klp_object - kernel object structure for live patching
> >   * @name:	module name (or NULL for vmlinux)
> >   * @funcs:	function entries for functions to be patched in the object
> >   * @callbacks:	functions to be executed pre/post (un)patching
> > + * @shadow_types: shadow variable types used by the livepatch for the klp_object
> >   * @kobj:	kobject for sysfs resources
> >   * @func_list:	dynamic list of the function entries
> >   * @node:	list node for klp_patch obj_list
> > @@ -118,6 +121,7 @@ struct klp_object {
> >  	const char *name;
> >  	struct klp_func *funcs;
> >  	struct klp_callbacks callbacks;
> > +	struct klp_shadow_type **shadow_types;
> >  
> 
> Hmm.  The implementation of shadow_types inside klp_object might be
> difficult to integrate into kpatch-build.  For kpatches, we do utilize
> the kernel's shadow variable API directly, but at kpatch author time we
> don't have any of klp_patch objects in hand -- those are generated by
> template after the binary before/after comparison is completed.

I am sorry but I am not much familiar with kPatch. But I am surprised.
It should be similar to klp_callbacks. If it was possible to define
struct klp_callbacks for a particular struct klp_object then it
should be possible to define struct klp_shadow_types ** similar way.

> My first thought is that kpatch-build might associate any shadow
> variables in foo.c with its parent object (vmlinux, foo.ko, etc.),
> however that may not always be 100% accurate.

I think that I would need to get more familiar with kPatch...

> In fact, we have occasionally used shadow variables with <id, 0> to
> create one off variables not associated with specific code or data
> structures, but rather the presence of livepatch.  This is (too) briefly
> mentioned in "Other use-cases" section of the shadow variable Documentation.

I think that we do this as well. AFAIK, we are going to associate them
with the klp_object for "vmlinux".

Best Regards,
Petr

Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Petr Mladek]

On Mon 2022-10-24 17:09:08, Petr Mladek wrote:
> On Thu 2022-08-25 12:26:25, Joe Lawrence wrote:
> > On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> > > The life of shadow variables is not completely trivial to maintain.
> > > They might be used by more livepatches and more livepatched objects.
> > > They should stay as long as there is any user.
> > > 
> > > In practice, it requires to implement reference counting in callbacks
> > > of all users. They should register all the user and remove the shadow
> > > variables only when there is no user left.
> > > 
> > > This patch hides the reference counting into the klp_shadow API.
> > > The counter is connected with the shadow variable @id. It requires
> > > an API to take and release the reference. The release function also
> > > calls the related dtor() when defined.
> > > 
> > > An easy solution would be to add some get_ref()/put_ref() API.
> > > But it would need to get called from pre()/post_un() callbacks.
> > > It might be easy to forget a callback and make it wrong.
> > > 
> > > A more safe approach is to associate the klp_shadow_type with
> > > klp_objects that use the shadow variables. The livepatch core
> > > code might then handle the reference counters on background.
> > > 
> > > The shadow variable type might then be added into a new @shadow_types
> > > member of struct klp_object. They will get then automatically registered
> > > and unregistered when the object is being livepatched. The registration
> > > increments the reference count. Unregistration decreases the reference
> > > count. All shadow variables of the given type are freed when the reference
> > > count reaches zero.
> > > 
> > > All klp_shadow_alloc/get/free functions also checks whether the requested
> > > type is registered. It will help to catch missing registration and might
> > > also help to catch eventual races.
> > > 
> > > --- a/include/linux/livepatch.h
> > > +++ b/include/linux/livepatch.h
> > > @@ -100,11 +100,14 @@ struct klp_callbacks {
> > >  	bool post_unpatch_enabled;
> > >  };
> > >  
> > > +struct klp_shadow_type;
> > > +
> > >  /**
> > >   * struct klp_object - kernel object structure for live patching
> > >   * @name:	module name (or NULL for vmlinux)
> > >   * @funcs:	function entries for functions to be patched in the object
> > >   * @callbacks:	functions to be executed pre/post (un)patching
> > > + * @shadow_types: shadow variable types used by the livepatch for the klp_object
> > >   * @kobj:	kobject for sysfs resources
> > >   * @func_list:	dynamic list of the function entries
> > >   * @node:	list node for klp_patch obj_list
> > > @@ -118,6 +121,7 @@ struct klp_object {
> > >  	const char *name;
> > >  	struct klp_func *funcs;
> > >  	struct klp_callbacks callbacks;
> > > +	struct klp_shadow_type **shadow_types;
> > >  
> > 
> > Hmm.  The implementation of shadow_types inside klp_object might be
> > difficult to integrate into kpatch-build.  For kpatches, we do utilize
> > the kernel's shadow variable API directly, but at kpatch author time we
> > don't have any of klp_patch objects in hand -- those are generated by
> > template after the binary before/after comparison is completed.
> 
> I am sorry but I am not much familiar with kPatch. But I am surprised.
> It should be similar to klp_callbacks. If it was possible to define
> struct klp_callbacks for a particular struct klp_object then it
> should be possible to define struct klp_shadow_types ** similar way.

Note that adding the used klp_shadow_types into struct klp_object
is not strictly required.

Alternative solution is to register/unregister the used types using
klp_callbacks or module init()/exit() callbacks. This approach
is used in lib/livepatch/test_klp_shadow_vars.c.

I believe that this would be usable for kpatch-build.
You needed to remove not-longer used shadow variables
using these callbacks even before this patchset. I would
consider it a bug if you did not remove them. The new API
just allows to do this a safe way.

Best Regards,
Petr

Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Joe Lawrence]

On 11/1/22 7:02 AM, Petr Mladek wrote:
> On Mon 2022-10-24 17:09:08, Petr Mladek wrote:
>> On Thu 2022-08-25 12:26:25, Joe Lawrence wrote:
>>> On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
>>>> The life of shadow variables is not completely trivial to maintain.
>>>> They might be used by more livepatches and more livepatched objects.
>>>> They should stay as long as there is any user.
>>>>
>>>> In practice, it requires to implement reference counting in callbacks
>>>> of all users. They should register all the user and remove the shadow
>>>> variables only when there is no user left.
>>>>
>>>> This patch hides the reference counting into the klp_shadow API.
>>>> The counter is connected with the shadow variable @id. It requires
>>>> an API to take and release the reference. The release function also
>>>> calls the related dtor() when defined.
>>>>
>>>> An easy solution would be to add some get_ref()/put_ref() API.
>>>> But it would need to get called from pre()/post_un() callbacks.
>>>> It might be easy to forget a callback and make it wrong.
>>>>
>>>> A more safe approach is to associate the klp_shadow_type with
>>>> klp_objects that use the shadow variables. The livepatch core
>>>> code might then handle the reference counters on background.
>>>>
>>>> The shadow variable type might then be added into a new @shadow_types
>>>> member of struct klp_object. They will get then automatically registered
>>>> and unregistered when the object is being livepatched. The registration
>>>> increments the reference count. Unregistration decreases the reference
>>>> count. All shadow variables of the given type are freed when the reference
>>>> count reaches zero.
>>>>
>>>> All klp_shadow_alloc/get/free functions also checks whether the requested
>>>> type is registered. It will help to catch missing registration and might
>>>> also help to catch eventual races.
>>>>
>>>> --- a/include/linux/livepatch.h
>>>> +++ b/include/linux/livepatch.h
>>>> @@ -100,11 +100,14 @@ struct klp_callbacks {
>>>>  	bool post_unpatch_enabled;
>>>>  };
>>>>  
>>>> +struct klp_shadow_type;
>>>> +
>>>>  /**
>>>>   * struct klp_object - kernel object structure for live patching
>>>>   * @name:	module name (or NULL for vmlinux)
>>>>   * @funcs:	function entries for functions to be patched in the object
>>>>   * @callbacks:	functions to be executed pre/post (un)patching
>>>> + * @shadow_types: shadow variable types used by the livepatch for the klp_object
>>>>   * @kobj:	kobject for sysfs resources
>>>>   * @func_list:	dynamic list of the function entries
>>>>   * @node:	list node for klp_patch obj_list
>>>> @@ -118,6 +121,7 @@ struct klp_object {
>>>>  	const char *name;
>>>>  	struct klp_func *funcs;
>>>>  	struct klp_callbacks callbacks;
>>>> +	struct klp_shadow_type **shadow_types;
>>>>  
>>>
>>> Hmm.  The implementation of shadow_types inside klp_object might be
>>> difficult to integrate into kpatch-build.  For kpatches, we do utilize
>>> the kernel's shadow variable API directly, but at kpatch author time we
>>> don't have any of klp_patch objects in hand -- those are generated by
>>> template after the binary before/after comparison is completed.
>>
>> I am sorry but I am not much familiar with kPatch. But I am surprised.
>> It should be similar to klp_callbacks. If it was possible to define
>> struct klp_callbacks for a particular struct klp_object then it
>> should be possible to define struct klp_shadow_types ** similar way.

Right.  For klp_callbacks, the kpatch author would logically provide the
callback code right in the compilation unit for said klp_object.
Additionally, kpatch uses callback macros to provide hints in the ELF
file so that kpatch-build can collect up the callbacks for each
klp_object to plug its the livepatch template.

In the case of shadow variables, we might use a similar hint mechanism.
 Also, we could implement additional sanity checking to detect if the
kpatch author is trying to access klp_object A's shadow_types from
klp_object B.  If there is a legit use case for that, then I would think
the klp_object better be vmlinux, else the kpatch may introduce
use-after-frees.

> Note that adding the used klp_shadow_types into struct klp_object
> is not strictly required.
> 
> Alternative solution is to register/unregister the used types using
> klp_callbacks or module init()/exit() callbacks. This approach
> is used in lib/livepatch/test_klp_shadow_vars.c.
> 
> I believe that this would be usable for kpatch-build.
> You needed to remove not-longer used shadow variables
> using these callbacks even before this patchset. I would
> consider it a bug if you did not remove them. The new API
> just allows to do this a safe way.
> 

Ah, let me dig into that example for alternative usage.  At first
glance, it looks like you're right -- kpatch already supports callbacks,
so just (un)register the shadow variables here.  I'll be back with more
info hopefully later this week.

Thanks,

-- 
Joe

Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Joe Lawrence]

On 11/2/22 8:51 AM, Joe Lawrence wrote:
> On 11/1/22 7:02 AM, Petr Mladek wrote:
>> Note that adding the used klp_shadow_types into struct klp_object
>> is not strictly required.
>>
>> Alternative solution is to register/unregister the used types using
>> klp_callbacks or module init()/exit() callbacks. This approach
>> is used in lib/livepatch/test_klp_shadow_vars.c.
>>
>> I believe that this would be usable for kpatch-build.
>> You needed to remove not-longer used shadow variables
>> using these callbacks even before this patchset. I would
>> consider it a bug if you did not remove them. The new API
>> just allows to do this a safe way.
>>
> 
> Ah, let me dig into that example for alternative usage.  At first
> glance, it looks like you're right -- kpatch already supports callbacks,
> so just (un)register the shadow variables here.  I'll be back with more
> info hopefully later this week.
> 

(Un)registering the shadow types from the callbacks worked out fine.  It
adds some verbosity to our shadow variable usage, but I think most of
that is unavoidable as the API is changing anyway.

Adding the shadow type into kpatch klp_objects (or the klp_patch) would
require a bit more work on our part, but doesn't need to hold up
upstream review up this patchset.

Thanks,

-- 
Joe