[PATCH v2 0/4] livepatch: Followup changes for the atomic replace patchset

[PATCH v2 0/4] livepatch: Followup changes for the atomic replace patchset

[Petr Mladek]

This patchset implements ideas that were mentioned and postponed during
the review of the atomic replace patchset.

The patches apply on top of livepatching.git, branch
origin/for-5.1/atomic-replace.


Changes against v1:

  + Added Joe's patch that fixed ptr_id() error code [Joe]
  + Did proper error handling in the shadow variable sefttest [All]
  + Removed the controversial patch that was removing patch->enabled flag [All].
  + Fixed few typo's [Joe]
  + Added available Acks.


Joe Lawrence (1):
  livepatch: return -ENOMEM on ptr_id() allocation failure

Petr Mladek (3):
  livepatch: Introduce klp_for_each_patch macro
  livepatch: Proper error handling in the shadow variables selftest
  livepatch: Module coming and going callbacks can proceed with all
    listed patches

 kernel/livepatch/core.c              | 34 ++++++++++------------------------
 kernel/livepatch/core.h              |  6 ++++++
 kernel/livepatch/transition.c        |  2 +-
 lib/livepatch/test_klp_shadow_vars.c | 24 +++++++++++++++++++++++-
 4 files changed, 40 insertions(+), 26 deletions(-)

-- 
2.13.7

[PATCH v2 1/4] livepatch: Introduce klp_for_each_patch macro

[Petr Mladek]

There are already macros to iterate over struct klp_func and klp_object.

Add also klp_for_each_patch(). But make it internal because also
klp_patches list is internal.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 kernel/livepatch/core.c       | 8 ++++----
 kernel/livepatch/core.h       | 6 ++++++
 kernel/livepatch/transition.c | 2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index adca5cf07f7e..b716a6289204 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -554,7 +554,7 @@ static int klp_add_nops(struct klp_patch *patch)
 	struct klp_patch *old_patch;
 	struct klp_object *old_obj;
 
-	list_for_each_entry(old_patch, &klp_patches, list) {
+	klp_for_each_patch(old_patch) {
 		klp_for_each_object(old_patch, old_obj) {
 			int err;
 
@@ -1089,7 +1089,7 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch)
 {
 	struct klp_patch *old_patch, *tmp_patch;
 
-	list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) {
+	klp_for_each_patch_safe(old_patch, tmp_patch) {
 		if (old_patch == new_patch)
 			return;
 
@@ -1133,7 +1133,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 	struct klp_patch *patch;
 	struct klp_object *obj;
 
-	list_for_each_entry(patch, &klp_patches, list) {
+	klp_for_each_patch(patch) {
 		if (patch == limit)
 			break;
 
@@ -1180,7 +1180,7 @@ int klp_module_coming(struct module *mod)
 	 */
 	mod->klp_alive = true;
 
-	list_for_each_entry(patch, &klp_patches, list) {
+	klp_for_each_patch(patch) {
 		klp_for_each_object(patch, obj) {
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index e6200f38701f..ec43a40b853f 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -7,6 +7,12 @@
 extern struct mutex klp_mutex;
 extern struct list_head klp_patches;
 
+#define klp_for_each_patch_safe(patch, tmp_patch)		\
+	list_for_each_entry_safe(patch, tmp_patch, &klp_patches, list)
+
+#define klp_for_each_patch(patch)	\
+	list_for_each_entry(patch, &klp_patches, list)
+
 void klp_free_patch_start(struct klp_patch *patch);
 void klp_discard_replaced_patches(struct klp_patch *new_patch);
 void klp_discard_nops(struct klp_patch *new_patch);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 300273819674..a3a6f32c6fd0 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -642,6 +642,6 @@ void klp_force_transition(void)
 	for_each_possible_cpu(cpu)
 		klp_update_patch_state(idle_task(cpu));
 
-	list_for_each_entry(patch, &klp_patches, list)
+	klp_for_each_patch(patch)
 		patch->forced = true;
 }
-- 
2.13.7

[PATCH v2 2/4] livepatch: return -ENOMEM on ptr_id() allocation failure

[Petr Mladek]

From: Joe Lawrence <joe.lawrence@redhat.com>

Fixes the following smatch warning:

  lib/livepatch/test_klp_shadow_vars.c:47 ptr_id() warn: returning -1 instead of -ENOMEM is sloppy

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/livepatch/test_klp_shadow_vars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index 02f892f941dc..f5441c193166 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -44,7 +44,7 @@ static int ptr_id(void *ptr)
 
 	sp = kmalloc(sizeof(*sp), GFP_ATOMIC);
 	if (!sp)
-		return -1;
+		return -ENOMEM;
 	sp->ptr = ptr;
 	sp->id = count++;
 
-- 
2.13.7

Re: [PATCH v2 2/4] livepatch: return -ENOMEM on ptr_id() allocation failure

[Miroslav Benes]

On Mon, 4 Feb 2019, Petr Mladek wrote:

> From: Joe Lawrence <joe.lawrence@redhat.com>
> 
> Fixes the following smatch warning:
> 
>   lib/livepatch/test_klp_shadow_vars.c:47 ptr_id() warn: returning -1 instead of -ENOMEM is sloppy
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

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

Miroslav

[PATCH v2 3/4] livepatch: Proper error handling in the shadow variables selftest

[Petr Mladek]

Add proper error handling when allocating or getting shadow variables
in the selftest. It prevents an invalid pointer access in some situations.
It shows the good programming practice in the others.

The error codes are just the best guess and specific for this particular
test. In general, klp_shadow_alloc() returns NULL also when the given
shadow variable has already been allocated. In addition, both
klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from
other reasons when the constructor fails.

Note, that the error code is not really important even in the real life.
The use of shadow variables should be transparent for the original
livepatched code.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/livepatch/test_klp_shadow_vars.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index f5441c193166..fe5c413efe96 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -154,22 +154,37 @@ static int test_klp_shadow_vars_init(void)
 	 * Allocate a few shadow variables with different <obj> and <id>.
 	 */
 	sv1 = shadow_alloc(obj, id, size, gfp_flags, shadow_ctor, &var1);
+	if (!sv1)
+		return -ENOMEM;
+
 	sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2);
+	if (!sv2)
+		return -ENOMEM;
+
 	sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3);
+	if (!sv3)
+		return -ENOMEM;
 
 	/*
 	 * Verify we can find our new shadow variables and that they point
 	 * to expected data.
 	 */
 	ret = shadow_get(obj, id);
+	if (!ret)
+		return -EINVAL;
 	if (ret == sv1 && *sv1 == &var1)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv1), ptr_id(*sv1));
+
 	ret = shadow_get(obj + 1, id);
+	if (!ret)
+		return -EINVAL;
 	if (ret == sv2 && *sv2 == &var2)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv2), ptr_id(*sv2));
 	ret = shadow_get(obj, id + 1);
+	if (!ret)
+		return -EINVAL;
 	if (ret == sv3 && *sv3 == &var3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
@@ -179,7 +194,12 @@ static int test_klp_shadow_vars_init(void)
 	 * The second invocation should return the same shadow var.
 	 */
 	sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
+	if (!sv4)
+		return -ENOMEM;
+
 	ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
+	if (!ret)
+		return -EINVAL;
 	if (ret == sv4 && *sv4 == &var4)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv4), ptr_id(*sv4));
@@ -207,6 +227,8 @@ static int test_klp_shadow_vars_init(void)
 	 * We should still find an <id+1> variable.
 	 */
 	ret = shadow_get(obj, id + 1);
+	if (!ret)
+		return -EINVAL;
 	if (ret == sv3 && *sv3 == &var3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
-- 
2.13.7

Re: [PATCH v2 3/4] livepatch: Proper error handling in the shadow variables selftest

[Miroslav Benes]

On Mon, 4 Feb 2019, Petr Mladek wrote:

> Add proper error handling when allocating or getting shadow variables
> in the selftest. It prevents an invalid pointer access in some situations.
> It shows the good programming practice in the others.
> 
> The error codes are just the best guess and specific for this particular
> test. In general, klp_shadow_alloc() returns NULL also when the given
> shadow variable has already been allocated. In addition, both
> klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from
> other reasons when the constructor fails.
> 
> Note, that the error code is not really important even in the real life.
> The use of shadow variables should be transparent for the original
> livepatched code.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

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

Miroslav

Re: [PATCH v2 3/4] livepatch: Proper error handling in the shadow variables selftest

[Joe Lawrence]

On 2/4/19 8:56 AM, Petr Mladek wrote:
> Add proper error handling when allocating or getting shadow variables
> in the selftest. It prevents an invalid pointer access in some situations.
> It shows the good programming practice in the others.
> 
> The error codes are just the best guess and specific for this particular
> test. In general, klp_shadow_alloc() returns NULL also when the given
> shadow variable has already been allocated. In addition, both
> klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from
> other reasons when the constructor fails.
> 
> Note, that the error code is not really important even in the real life.
> The use of shadow variables should be transparent for the original
> livepatched code.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>   lib/livepatch/test_klp_shadow_vars.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
> index f5441c193166..fe5c413efe96 100644
> --- a/lib/livepatch/test_klp_shadow_vars.c
> +++ b/lib/livepatch/test_klp_shadow_vars.c
> @@ -154,22 +154,37 @@ static int test_klp_shadow_vars_init(void)
>   	 * Allocate a few shadow variables with different <obj> and <id>.
>   	 */
>   	sv1 = shadow_alloc(obj, id, size, gfp_flags, shadow_ctor, &var1);
> +	if (!sv1)
> +		return -ENOMEM;
> +
>   	sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2);
> +	if (!sv2)
> +		return -ENOMEM;
> +
>   	sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3);
> +	if (!sv3)
> +		return -ENOMEM;
>   
>   	/*
>   	 * Verify we can find our new shadow variables and that they point
>   	 * to expected data.
>   	 */
>   	ret = shadow_get(obj, id);
> +	if (!ret)
> +		return -EINVAL;
>   	if (ret == sv1 && *sv1 == &var1)
>   		pr_info("  got expected PTR%d -> PTR%d result\n",
>   			ptr_id(sv1), ptr_id(*sv1));
> +
>   	ret = shadow_get(obj + 1, id);
> +	if (!ret)
> +		return -EINVAL;
>   	if (ret == sv2 && *sv2 == &var2)
>   		pr_info("  got expected PTR%d -> PTR%d result\n",
>   			ptr_id(sv2), ptr_id(*sv2));
>   	ret = shadow_get(obj, id + 1);
> +	if (!ret)
> +		return -EINVAL;
>   	if (ret == sv3 && *sv3 == &var3)
>   		pr_info("  got expected PTR%d -> PTR%d result\n",
>   			ptr_id(sv3), ptr_id(*sv3));
> @@ -179,7 +194,12 @@ static int test_klp_shadow_vars_init(void)
>   	 * The second invocation should return the same shadow var.
>   	 */
>   	sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
> +	if (!sv4)
> +		return -ENOMEM;
> +
>   	ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
> +	if (!ret)
> +		return -EINVAL;
>   	if (ret == sv4 && *sv4 == &var4)
>   		pr_info("  got expected PTR%d -> PTR%d result\n",
>   			ptr_id(sv4), ptr_id(*sv4));
> @@ -207,6 +227,8 @@ static int test_klp_shadow_vars_init(void)
>   	 * We should still find an <id+1> variable.
>   	 */
>   	ret = shadow_get(obj, id + 1);
> +	if (!ret)
> +		return -EINVAL;
>   	if (ret == sv3 && *sv3 == &var3)
>   		pr_info("  got expected PTR%d -> PTR%d result\n",
>   			ptr_id(sv3), ptr_id(*sv3));
> 

Fixes look good to me,

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

[PATCH v2 4/4] livepatch: Module coming and going callbacks can proceed with all listed patches

[Petr Mladek]

Livepatches can no longer get enabled and disabled repeatedly.
The list klp_patches contains only enabled patches and eventually
the patch in transition.

The module coming and going callbacks do no longer need to check
for these state. They have to proceed with all listed patches.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 kernel/livepatch/core.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b716a6289204..684766d306ad 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1141,21 +1141,14 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
 
-			/*
-			 * Only unpatch the module if the patch is enabled or
-			 * is in transition.
-			 */
-			if (patch->enabled || patch == klp_transition_patch) {
-
-				if (patch != klp_transition_patch)
-					klp_pre_unpatch_callback(obj);
+			if (patch != klp_transition_patch)
+				klp_pre_unpatch_callback(obj);
 
-				pr_notice("reverting patch '%s' on unloading module '%s'\n",
-					  patch->mod->name, obj->mod->name);
-				klp_unpatch_object(obj);
+			pr_notice("reverting patch '%s' on unloading module '%s'\n",
+				  patch->mod->name, obj->mod->name);
+			klp_unpatch_object(obj);
 
-				klp_post_unpatch_callback(obj);
-			}
+			klp_post_unpatch_callback(obj);
 
 			klp_free_object_loaded(obj);
 			break;
@@ -1194,13 +1187,6 @@ int klp_module_coming(struct module *mod)
 				goto err;
 			}
 
-			/*
-			 * Only patch the module if the patch is enabled or is
-			 * in transition.
-			 */
-			if (!patch->enabled && patch != klp_transition_patch)
-				break;
-
 			pr_notice("applying patch '%s' to loading module '%s'\n",
 				  patch->mod->name, obj->mod->name);
 
-- 
2.13.7

Re: [PATCH v2 0/4] livepatch: Followup changes for the atomic replace patchset

[Petr Mladek]

On Mon 2019-02-04 14:56:49, Petr Mladek wrote:
> This patchset implements ideas that were mentioned and postponed during
> the review of the atomic replace patchset.
> 
> The patches apply on top of livepatching.git, branch
> origin/for-5.1/atomic-replace.
> 
> 
> Changes against v1:
> 
>   + Added Joe's patch that fixed ptr_id() error code [Joe]
>   + Did proper error handling in the shadow variable sefttest [All]
>   + Removed the controversial patch that was removing patch->enabled flag [All].
>   + Fixed few typo's [Joe]
>   + Added available Acks.
> 
> 
> Joe Lawrence (1):
>   livepatch: return -ENOMEM on ptr_id() allocation failure
> 
> Petr Mladek (3):
>   livepatch: Introduce klp_for_each_patch macro
>   livepatch: Proper error handling in the shadow variables selftest
>   livepatch: Module coming and going callbacks can proceed with all
>     listed patches

I have applied all 4 paches into for-5.1/atomic-replace branch.

Best Regards,
Petr

Re: [PATCH v2 0/4] livepatch: Followup changes for the atomic replace patchset

[Josh Poimboeuf]

On Mon, Feb 04, 2019 at 02:56:49PM +0100, Petr Mladek wrote:
> This patchset implements ideas that were mentioned and postponed during
> the review of the atomic replace patchset.
> 
> The patches apply on top of livepatching.git, branch
> origin/for-5.1/atomic-replace.
> 
> 
> Changes against v1:
> 
>   + Added Joe's patch that fixed ptr_id() error code [Joe]
>   + Did proper error handling in the shadow variable sefttest [All]
>   + Removed the controversial patch that was removing patch->enabled flag [All].
>   + Fixed few typo's [Joe]
>   + Added available Acks.

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh