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

[PATCH 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. I hope that I did not miss
anything.

Well, I did not add __used attribute to avoid non-static warnings
in modules for the selftest. The work on the sample modules somehow
stalled.

BTW: Does it make sense to maintain the sample modules any longer?
We could point people to the modules used by the selftest instead.


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


Petr Mladek (4):
  livepatch: Introduce klp_for_each_patch macro
  livepatch: Handle failing allocation of shadow variables in the
    selftest
  livepatch: Module coming and going callbacks can proceed all listed
    patches
  livepatch: Remove the redundant enabled flag in struct klp_patch

 include/linux/livepatch.h            |  2 --
 kernel/livepatch/core.c              | 57 ++++++++++++++++--------------------
 kernel/livepatch/core.h              |  6 ++++
 kernel/livepatch/transition.c        |  9 +++---
 kernel/livepatch/transition.h        |  1 +
 lib/livepatch/test_klp_shadow_vars.c |  8 ++---
 6 files changed, 40 insertions(+), 43 deletions(-)

-- 
2.13.7

[PATCH 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>
---
 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 2/4] livepatch: Handle failing allocation of shadow variables in the selftest

[Petr Mladek]

Do not dereference pointers to the shadow variables when either
klp_shadow_alloc() or klp_shadow_get() fail.

There is no need to check the other locations explicitly. The test
would fail if any allocation fails. And the existing messages, printed
during the test, provide enough information to debug eventual problems.

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

diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index 02f892f941dc..55e6820430dc 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
 	 * to expected data.
 	 */
 	ret = shadow_get(obj, id);
-	if (ret == sv1 && *sv1 == &var1)
+	if (ret && 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 == sv2 && *sv2 == &var2)
+	if (ret && 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 == sv3 && *sv3 == &var3)
+	if (ret && ret == sv3 && *sv3 == &var3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
 
@@ -180,7 +180,7 @@ static int test_klp_shadow_vars_init(void)
 	 */
 	sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
 	ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
-	if (ret == sv4 && *sv4 == &var4)
+	if (ret && ret == sv4 && *sv4 == &var4)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv4), ptr_id(*sv4));
 
-- 
2.13.7

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

[Petr Mladek]

Livepatches can not 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 not longer need to
check for these state. They have to proceed all listed patches.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.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

[PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Petr Mladek]

Livepatches can not longer get enabled and disabled repeatedly.
The list klp_patches contains only enabled patches and eventually
the patch in transition. As a result, the enabled flag in
struct klp_patch provides redundant information and can get
removed.

The flag is replaced by helper function klp_patch_enabled().
It simplifies the code. Also it helps to understand the semantic,
especially for the patch in transition.

Alternative solution was to remove klp_target_state. But this
would be unfortunate. The three state variable helps to
catch bugs and regressions. Also it makes it easier to get
the state a lockless way in klp_update_patch_state().

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h     |  2 --
 kernel/livepatch/core.c       | 23 +++++++++++++++--------
 kernel/livepatch/transition.c |  7 +++----
 kernel/livepatch/transition.h |  1 +
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..fa68192e6bb2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -155,7 +155,6 @@ struct klp_object {
  * @kobj:	kobject for sysfs resources
  * @obj_list:	dynamic list of the object entries
  * @kobj_added: @kobj has been added and needs freeing
- * @enabled:	the patch is enabled (but operation may be incomplete)
  * @forced:	was involved in a forced transition
  * @free_work:	patch cleanup from workqueue-context
  * @finish:	for waiting till it is safe to remove the patch module
@@ -171,7 +170,6 @@ struct klp_patch {
 	struct kobject kobj;
 	struct list_head obj_list;
 	bool kobj_added;
-	bool enabled;
 	bool forced;
 	struct work_struct free_work;
 	struct completion finish;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 684766d306ad..8e644837e668 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -59,6 +59,17 @@ static bool klp_is_module(struct klp_object *obj)
 	return obj->name;
 }
 
+static bool klp_patch_enabled(struct klp_patch *patch)
+{
+	if (patch == klp_transition_patch) {
+		WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
+
+		return klp_target_state == KLP_PATCHED;
+	}
+
+	return !list_empty(&patch->list);
+}
+
 /* sets obj->mod if object is not vmlinux and module is found */
 static void klp_find_object_module(struct klp_object *obj)
 {
@@ -335,7 +346,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 	mutex_lock(&klp_mutex);
 
-	if (patch->enabled == enabled) {
+	if (klp_patch_enabled(patch) == enabled) {
 		/* already in requested state */
 		ret = -EINVAL;
 		goto out;
@@ -369,7 +380,7 @@ static ssize_t enabled_show(struct kobject *kobj,
 	struct klp_patch *patch;
 
 	patch = container_of(kobj, struct klp_patch, kobj);
-	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
+	return snprintf(buf, PAGE_SIZE-1, "%d\n", klp_patch_enabled(patch));
 }
 
 static ssize_t transition_show(struct kobject *kobj,
@@ -862,7 +873,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
 	INIT_LIST_HEAD(&patch->list);
 	INIT_LIST_HEAD(&patch->obj_list);
 	patch->kobj_added = false;
-	patch->enabled = false;
 	patch->forced = false;
 	INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
 	init_completion(&patch->finish);
@@ -919,7 +929,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
 
-	if (WARN_ON(!patch->enabled))
+	if (WARN_ON(!klp_patch_enabled(patch)))
 		return -EINVAL;
 
 	if (klp_transition_patch)
@@ -941,7 +951,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
 	smp_wmb();
 
 	klp_start_transition();
-	patch->enabled = false;
 	klp_try_complete_transition();
 
 	return 0;
@@ -955,7 +964,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (klp_transition_patch)
 		return -EBUSY;
 
-	if (WARN_ON(patch->enabled))
+	if (list_empty(&patch->list))
 		return -EINVAL;
 
 	if (!patch->kobj_added)
@@ -994,7 +1003,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	}
 
 	klp_start_transition();
-	patch->enabled = true;
 	klp_try_complete_transition();
 
 	return 0;
@@ -1093,7 +1101,6 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch)
 		if (old_patch == new_patch)
 			return;
 
-		old_patch->enabled = false;
 		klp_unpatch_objects(old_patch);
 		klp_free_patch_start(old_patch);
 		schedule_work(&old_patch->free_work);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index a3a6f32c6fd0..a40b58660640 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -31,7 +31,7 @@
 
 struct klp_patch *klp_transition_patch;
 
-static int klp_target_state = KLP_UNDEFINED;
+int klp_target_state = KLP_UNDEFINED;
 
 /*
  * This work can be performed periodically to finish patching or unpatching any
@@ -354,6 +354,7 @@ static bool klp_try_switch_task(struct task_struct *task)
 void klp_try_complete_transition(void)
 {
 	unsigned int cpu;
+	int target_state = klp_target_state;
 	struct task_struct *g, *task;
 	struct klp_patch *patch;
 	bool complete = true;
@@ -412,7 +413,7 @@ void klp_try_complete_transition(void)
 	 * klp_complete_transition() but it is called also
 	 * from klp_cancel_transition().
 	 */
-	if (!patch->enabled) {
+	if (target_state == KLP_UNPATCHED) {
 		klp_free_patch_start(patch);
 		schedule_work(&patch->free_work);
 	}
@@ -545,8 +546,6 @@ void klp_reverse_transition(void)
 		 klp_target_state == KLP_PATCHED ? "patching to unpatching" :
 						   "unpatching to patching");
 
-	klp_transition_patch->enabled = !klp_transition_patch->enabled;
-
 	klp_target_state = !klp_target_state;
 
 	/*
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index f9d0bc016067..b9f3e96d8c13 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -5,6 +5,7 @@
 #include <linux/livepatch.h>
 
 extern struct klp_patch *klp_transition_patch;
+extern int klp_target_state;
 
 void klp_init_transition(struct klp_patch *patch, int state);
 void klp_cancel_transition(void);
-- 
2.13.7

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

[Miroslav Benes]

On Wed, 16 Jan 2019, Petr Mladek wrote:

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

M

Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest

[Miroslav Benes]

Hi,

On Wed, 16 Jan 2019, Petr Mladek wrote:

> Do not dereference pointers to the shadow variables when either
> klp_shadow_alloc() or klp_shadow_get() fail.

I may misunderstand the patch, so bear with me, please. Is this because of 
a possible null pointer dereference? If yes, shouldn't this say rather 
"when both klp_shadow_alloc() and klp_shadow_get() fail"?
 
> There is no need to check the other locations explicitly. The test
> would fail if any allocation fails. And the existing messages, printed
> during the test, provide enough information to debug eventual problems.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/livepatch/test_klp_shadow_vars.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
> index 02f892f941dc..55e6820430dc 100644
> --- a/lib/livepatch/test_klp_shadow_vars.c
> +++ b/lib/livepatch/test_klp_shadow_vars.c
> @@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
>  	 * to expected data.
>  	 */
>  	ret = shadow_get(obj, id);
> -	if (ret == sv1 && *sv1 == &var1)
> +	if (ret && 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 == sv2 && *sv2 == &var2)
> +	if (ret && 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 == sv3 && *sv3 == &var3)
> +	if (ret && ret == sv3 && *sv3 == &var3)
>  		pr_info("  got expected PTR%d -> PTR%d result\n",
>  			ptr_id(sv3), ptr_id(*sv3));

There is one more similar site calling shadow_get(obj, id + 1) which is 
fixed.

Thanks,
Miroslav

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

[Miroslav Benes]

On Wed, 16 Jan 2019, Petr Mladek wrote:

> Livepatches can not 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 not longer need to
> check for these state. They have to proceed 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>

M

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

[Joe Lawrence]

On Wed, Jan 16, 2019 at 05:17:17PM +0100, Petr Mladek wrote:
> 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>
> ---
>  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
> 

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

-- Joe

Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest

[Joe Lawrence]

On Wed, Jan 16, 2019 at 05:17:18PM +0100, Petr Mladek wrote:
> Do not dereference pointers to the shadow variables when either
> klp_shadow_alloc() or klp_shadow_get() fail.
> 
> There is no need to check the other locations explicitly. The test
> would fail if any allocation fails. And the existing messages, printed
> during the test, provide enough information to debug eventual problems.
> 

I didn't run the test under those failing conditions, but at looking at
the code, I think it would simply skip the "expected <conditions> found"
and the test script would complain about not seeing that msg.

Would it be easier to just bite the bullet and verify sv[0-4] at their
allocation sites?  Then later uses (ie, the sv3 dereference that
Miroslav spotted at the bottom) or new code wouldn't fall through the
cracks.

-- Joe

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

[Joe Lawrence]

On Wed, Jan 16, 2019 at 05:17:19PM +0100, Petr Mladek wrote:
> Livepatches can not longer get enabled and disabled repeatedly.

nit: s/not longer/no longer/g

> The list klp_patches contains only enabled patches and eventually
> the patch in transition.
> 
> The module coming and going callbacks do not longer need to
> check for these state. They have to proceed all listed patches.

nit: suggestion to modify "proceed all" to "proceed with" or "execute
all".  Same suggestion for the subject line.  (I keep reading it as
"precede all" and I'm wondering if there is some kind of ordering
change.)
   
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.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
> 

Any simplication to the callback code is welcome!  Thanks for cleaning
this one up.

With a few commit msg nits,
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Joe Lawrence]

On Wed, Jan 16, 2019 at 05:17:20PM +0100, Petr Mladek wrote:
> Livepatches can not longer get enabled and disabled repeatedly.

nit: s/not longer/no longer/g

> The list klp_patches contains only enabled patches and eventually
> the patch in transition. As a result, the enabled flag in
> struct klp_patch provides redundant information and can get
> removed.
> 
> The flag is replaced by helper function klp_patch_enabled().
> It simplifies the code. Also it helps to understand the semantic,
> especially for the patch in transition.
> 
> Alternative solution was to remove klp_target_state. But this
> would be unfortunate. The three state variable helps to
> catch bugs and regressions. Also it makes it easier to get
> the state a lockless way in klp_update_patch_state().

smaller nit: s/get the state/get the state in/

> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/livepatch.h     |  2 --
>  kernel/livepatch/core.c       | 23 +++++++++++++++--------
>  kernel/livepatch/transition.c |  7 +++----
>  kernel/livepatch/transition.h |  1 +
>  4 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 53551f470722..fa68192e6bb2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -155,7 +155,6 @@ struct klp_object {
>   * @kobj:	kobject for sysfs resources
>   * @obj_list:	dynamic list of the object entries
>   * @kobj_added: @kobj has been added and needs freeing
> - * @enabled:	the patch is enabled (but operation may be incomplete)
>   * @forced:	was involved in a forced transition
>   * @free_work:	patch cleanup from workqueue-context
>   * @finish:	for waiting till it is safe to remove the patch module
> @@ -171,7 +170,6 @@ struct klp_patch {
>  	struct kobject kobj;
>  	struct list_head obj_list;
>  	bool kobj_added;
> -	bool enabled;
>  	bool forced;
>  	struct work_struct free_work;
>  	struct completion finish;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 684766d306ad..8e644837e668 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -59,6 +59,17 @@ static bool klp_is_module(struct klp_object *obj)
>  	return obj->name;
>  }
>  
> +static bool klp_patch_enabled(struct klp_patch *patch)
> +{
> +	if (patch == klp_transition_patch) {
> +		WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
> +
> +		return klp_target_state == KLP_PATCHED;
> +	}
> +
> +	return !list_empty(&patch->list);
> +}
> +
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {
> @@ -335,7 +346,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	mutex_lock(&klp_mutex);
>  
> -	if (patch->enabled == enabled) {
> +	if (klp_patch_enabled(patch) == enabled) {
>  		/* already in requested state */
>  		ret = -EINVAL;
>  		goto out;
> @@ -369,7 +380,7 @@ static ssize_t enabled_show(struct kobject *kobj,
>  	struct klp_patch *patch;
>  
>  	patch = container_of(kobj, struct klp_patch, kobj);
> -	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
> +	return snprintf(buf, PAGE_SIZE-1, "%d\n", klp_patch_enabled(patch));
>  }
>  
>  static ssize_t transition_show(struct kobject *kobj,
> @@ -862,7 +873,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
>  	INIT_LIST_HEAD(&patch->list);
>  	INIT_LIST_HEAD(&patch->obj_list);
>  	patch->kobj_added = false;
> -	patch->enabled = false;
>  	patch->forced = false;
>  	INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
>  	init_completion(&patch->finish);
> @@ -919,7 +929,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  {
>  	struct klp_object *obj;
>  
> -	if (WARN_ON(!patch->enabled))
> +	if (WARN_ON(!klp_patch_enabled(patch)))
>  		return -EINVAL;
>  
>  	if (klp_transition_patch)
> @@ -941,7 +951,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  	smp_wmb();
>  
>  	klp_start_transition();
> -	patch->enabled = false;
>  	klp_try_complete_transition();
>  
>  	return 0;
> @@ -955,7 +964,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	if (klp_transition_patch)
>  		return -EBUSY;
>  
> -	if (WARN_ON(patch->enabled))
> +	if (list_empty(&patch->list))
>  		return -EINVAL;
>  
>  	if (!patch->kobj_added)
> @@ -994,7 +1003,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	}
>  
>  	klp_start_transition();
> -	patch->enabled = true;
>  	klp_try_complete_transition();
>  
>  	return 0;
> @@ -1093,7 +1101,6 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch)
>  		if (old_patch == new_patch)
>  			return;
>  
> -		old_patch->enabled = false;
>  		klp_unpatch_objects(old_patch);
>  		klp_free_patch_start(old_patch);
>  		schedule_work(&old_patch->free_work);
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index a3a6f32c6fd0..a40b58660640 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -31,7 +31,7 @@
>  
>  struct klp_patch *klp_transition_patch;
>  
> -static int klp_target_state = KLP_UNDEFINED;
> +int klp_target_state = KLP_UNDEFINED;
>  
>  /*
>   * This work can be performed periodically to finish patching or unpatching any
> @@ -354,6 +354,7 @@ static bool klp_try_switch_task(struct task_struct *task)
>  void klp_try_complete_transition(void)
>  {
>  	unsigned int cpu;
> +	int target_state = klp_target_state;
>  	struct task_struct *g, *task;
>  	struct klp_patch *patch;
>  	bool complete = true;
> @@ -412,7 +413,7 @@ void klp_try_complete_transition(void)
>  	 * klp_complete_transition() but it is called also
>  	 * from klp_cancel_transition().
>  	 */
> -	if (!patch->enabled) {
> +	if (target_state == KLP_UNPATCHED) {
>  		klp_free_patch_start(patch);
>  		schedule_work(&patch->free_work);
>  	}
> @@ -545,8 +546,6 @@ void klp_reverse_transition(void)
>  		 klp_target_state == KLP_PATCHED ? "patching to unpatching" :
>  						   "unpatching to patching");
>  
> -	klp_transition_patch->enabled = !klp_transition_patch->enabled;
> -
>  	klp_target_state = !klp_target_state;
>  
>  	/*
> diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> index f9d0bc016067..b9f3e96d8c13 100644
> --- a/kernel/livepatch/transition.h
> +++ b/kernel/livepatch/transition.h
> @@ -5,6 +5,7 @@
>  #include <linux/livepatch.h>
>  
>  extern struct klp_patch *klp_transition_patch;
> +extern int klp_target_state;
>  
>  void klp_init_transition(struct klp_patch *patch, int state);
>  void klp_cancel_transition(void);
> -- 
> 2.13.7
> 

With commit msg nits,
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Miroslav Benes]

On Wed, 16 Jan 2019, Petr Mladek wrote:

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 684766d306ad..8e644837e668 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -59,6 +59,17 @@ static bool klp_is_module(struct klp_object *obj)
>  	return obj->name;
>  }
>  
> +static bool klp_patch_enabled(struct klp_patch *patch)
> +{
> +	if (patch == klp_transition_patch) {
> +		WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);

I think we'd have a race in the code then. enabled_show() does not take 
klp_mutex() when calling klp_patch_enabled().

A patch sysfs attributes are added quite early during its enablement. 
klp_init_transition() first sets klp_transition_patch, then 
klp_target_state. It means one can call enabled_show() with patch == 
klp_transition_patch and klp_target_state == KLP_UNDEFINED. No?

The similar applies the disablement. klp_complete_transition() first 
clears klp_target_state (sets it to KLP_UNDEFINED), then it clears 
klp_transition_patch.

We could add locking to enabled_show() or swap the assignments with some 
barriers on top.

Or we could remove WARN_ON_ONCE() and live with false results in 
enabled_show(). It does not matter much, I think. All the other call sites 
of klp_patch_enabled() should be fine.

> +		return klp_target_state == KLP_PATCHED;
> +	}
> +
> +	return !list_empty(&patch->list);
> +}

Shouldn't we also change list_del(&patch->list) in klp_free_patch_start() 
to list_del_init(&patch->list)?

[...]

> @@ -955,7 +964,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	if (klp_transition_patch)
>  		return -EBUSY;
>  
> -	if (WARN_ON(patch->enabled))
> +	if (list_empty(&patch->list))
>  		return -EINVAL;

I wanted to ask why there is list_empty() and not klp_patch_enabled(), so 
just to be sure... the patch was added to klp_patches list, so patch->list 
is not empty (should not be). We could achieve the same by calling 
!klp_patch_enabled() given its implementation, but it would look 
counter-intuitive here.

The rest looks fine.

However, I am not sure if the outcome is better than what we have. Yes, 
patch->enabled is not technically necessary and we can live with that (as 
the patch proves). On the other hand, it gives the reader clear guidance 
about the patch's state. klp_patch_enabled() is not a complete 
replacement. We have to call list_empty() in __klp_enable_patch() or check 
the original klp_target_state in klp_try_complete_transition().

I am not against the change, I am glad to see it is achievable, but I am 
not sure if the code is better with it. Joe acked it. What do the others 
think?

Thanks,
Miroslav

Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Joe Lawrence]

On 1/22/19 5:06 AM, Miroslav Benes wrote:
> On Wed, 16 Jan 2019, Petr Mladek wrote:
> 
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 684766d306ad..8e644837e668 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -59,6 +59,17 @@ static bool klp_is_module(struct klp_object *obj)
>>   	return obj->name;
>>   }
>>   
>> +static bool klp_patch_enabled(struct klp_patch *patch)
>> +{
>> +	if (patch == klp_transition_patch) {
>> +		WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
> 
> I think we'd have a race in the code then. enabled_show() does not take
> klp_mutex() when calling klp_patch_enabled().
 >
> A patch sysfs attributes are added quite early during its enablement.
> klp_init_transition() first sets klp_transition_patch, then
> klp_target_state. It means one can call enabled_show() with patch ==
> klp_transition_patch and klp_target_state == KLP_UNDEFINED. No?
> 
> The similar applies the disablement. klp_complete_transition() first
> clears klp_target_state (sets it to KLP_UNDEFINED), then it clears
> klp_transition_patch.
> 
> We could add locking to enabled_show() or swap the assignments with some
> barriers on top.
>

Taking the mutex as enabled_store() does would be simplest, no?

> Or we could remove WARN_ON_ONCE() and live with false results in
> enabled_show(). It does not matter much, I think. All the other call sites
> of klp_patch_enabled() should be fine.
> 

Hmm, the self-tests and the kpatch tool inspect the sysfs files, but as 
long as the false result is a stale value, I think they would be okay. 
Those tools poll sysfs and don't depend on a one-shot-read value to make 
an enabled/disabled determination.

>> +		return klp_target_state == KLP_PATCHED;
>> +	}
>> +
>> +	return !list_empty(&patch->list);
>> +}
> 
> Shouldn't we also change list_del(&patch->list) in klp_free_patch_start()
> to list_del_init(&patch->list)?
> 

Right, we should do that if klp_patch_enabled() is going to subsequently 
check that list.

>> @@ -955,7 +964,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
>>   	if (klp_transition_patch)
>>   		return -EBUSY;
>>   
>> -	if (WARN_ON(patch->enabled))
>> +	if (list_empty(&patch->list))
>>   		return -EINVAL;
> 
> I wanted to ask why there is list_empty() and not klp_patch_enabled(), so
> just to be sure... the patch was added to klp_patches list, so patch->list
> is not empty (should not be). We could achieve the same by calling
> !klp_patch_enabled() given its implementation, but it would look
> counter-intuitive here.
> 
> The rest looks fine.
> 
> However, I am not sure if the outcome is better than what we have. Yes,
> patch->enabled is not technically necessary and we can live with that (as
> the patch proves). On the other hand, it gives the reader clear guidance
> about the patch's state. klp_patch_enabled() is not a complete
> replacement. We have to call list_empty() in __klp_enable_patch() or check
> the original klp_target_state in klp_try_complete_transition().
> 
> I am not against the change, I am glad to see it is achievable, but I am
> not sure if the code is better with it. Joe acked it. What do the others
> think?

Let me qualify my ack -- I think minimizing the number of state 
variables like patch->enabled can help readability... on the other hand, 
deducing the same information from other properties like list-empty can 
be confusing, ie, klp_patch_enabled() is generally a lot clearer than 
list_empty(&patch->list)).

So I like this idea and would be interested to hear what folks think 
about the exception cases you pointed out.

-- Joe

Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Josh Poimboeuf]

On Wed, Jan 23, 2019 at 01:27:59PM -0500, Joe Lawrence wrote:
> > I wanted to ask why there is list_empty() and not klp_patch_enabled(), so
> > just to be sure... the patch was added to klp_patches list, so patch->list
> > is not empty (should not be). We could achieve the same by calling
> > !klp_patch_enabled() given its implementation, but it would look
> > counter-intuitive here.
> > 
> > The rest looks fine.
> > 
> > However, I am not sure if the outcome is better than what we have. Yes,
> > patch->enabled is not technically necessary and we can live with that (as
> > the patch proves). On the other hand, it gives the reader clear guidance
> > about the patch's state. klp_patch_enabled() is not a complete
> > replacement. We have to call list_empty() in __klp_enable_patch() or check
> > the original klp_target_state in klp_try_complete_transition().
> > 
> > I am not against the change, I am glad to see it is achievable, but I am
> > not sure if the code is better with it. Joe acked it. What do the others
> > think?
> 
> Let me qualify my ack -- I think minimizing the number of state variables
> like patch->enabled can help readability... on the other hand, deducing the
> same information from other properties like list-empty can be confusing, ie,
> klp_patch_enabled() is generally a lot clearer than
> list_empty(&patch->list)).
> 
> So I like this idea and would be interested to hear what folks think about
> the exception cases you pointed out.

I share Miroslav and Joe's ambivalence.  It's interesting to see that it
can be done, and normally I'd prefer to get rid of extraneous data
fields, but the patch doesn't reduce code, and it even makes the code
slightly more complex IMO, because klp_patch_enabled() doesn't always
work like you'd expect.

So while I suggested it to begin with, I'm going to go with a NACK :-)

-- 
Josh

Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest

[Petr Mladek]

On Mon 2019-01-21 13:14:38, Miroslav Benes wrote:
> Hi,
> 
> On Wed, 16 Jan 2019, Petr Mladek wrote:
> 
> > Do not dereference pointers to the shadow variables when either
> > klp_shadow_alloc() or klp_shadow_get() fail.
> 
> I may misunderstand the patch, so bear with me, please. Is this because of 
> a possible null pointer dereference? If yes, shouldn't this say rather 
> "when both klp_shadow_alloc() and klp_shadow_get() fail"?

Well, klp_shadow_get() could fail also from other reasons if there is
a bug in the implementation.

> > There is no need to check the other locations explicitly. The test
> > would fail if any allocation fails. And the existing messages, printed
> > during the test, provide enough information to debug eventual problems.

Heh, this is actually the reason why I did not add the check
for shadow_alloc(). Any error would be detected later
with klp_shadow_get() calls that should get tested anyway.

Hmm, when I think about it. A good practice is to handle
klp_shadow_allow() or klp_shadow_get() failures immediately.
After all, it is the sample code that people might follow.


> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  lib/livepatch/test_klp_shadow_vars.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
> > index 02f892f941dc..55e6820430dc 100644
> > --- a/lib/livepatch/test_klp_shadow_vars.c
> > +++ b/lib/livepatch/test_klp_shadow_vars.c
> > @@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
> >  	 * to expected data.
> >  	 */
> >  	ret = shadow_get(obj, id);
> > -	if (ret == sv1 && *sv1 == &var1)
> > +	if (ret && 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 == sv2 && *sv2 == &var2)
> > +	if (ret && 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 == sv3 && *sv3 == &var3)
> > +	if (ret && ret == sv3 && *sv3 == &var3)
> >  		pr_info("  got expected PTR%d -> PTR%d result\n",
> >  			ptr_id(sv3), ptr_id(*sv3));
> 
> There is one more similar site calling shadow_get(obj, id + 1) which is 
> fixed.

Heh, I think that I did not add the check there on purpose.
If we are here, shadow_get(obj, id + 1) must have already succeeded
above.

But it is a bad practice. We should always check the output.
I'll do so in v2.

Best Regards,
Petr

Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest

[Petr Mladek]

On Mon 2019-01-21 17:40:12, Joe Lawrence wrote:
> On Wed, Jan 16, 2019 at 05:17:18PM +0100, Petr Mladek wrote:
> > Do not dereference pointers to the shadow variables when either
> > klp_shadow_alloc() or klp_shadow_get() fail.
> > 
> > There is no need to check the other locations explicitly. The test
> > would fail if any allocation fails. And the existing messages, printed
> > during the test, provide enough information to debug eventual problems.
> > 
> 
> I didn't run the test under those failing conditions, but at looking at
> the code, I think it would simply skip the "expected <conditions> found"
> and the test script would complain about not seeing that msg.

Accessing an invalid pointer would crash the kernel.


> Would it be easier to just bite the bullet and verify sv[0-4] at their
> allocation sites?  Then later uses (ie, the sv3 dereference that
> Miroslav spotted at the bottom) or new code wouldn't fall through the
> cracks.

As I wrote in the replay to Miroslav. The best practice is to
handle errors everywhere. I am going to do so in v2. People
might use it as a sample...

Best Regards,
Petr

Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Petr Mladek]

On Tue 2019-01-29 14:00:49, Josh Poimboeuf wrote:
> On Wed, Jan 23, 2019 at 01:27:59PM -0500, Joe Lawrence wrote:
> > > I wanted to ask why there is list_empty() and not klp_patch_enabled(), so
> > > just to be sure... the patch was added to klp_patches list, so patch->list
> > > is not empty (should not be). We could achieve the same by calling
> > > !klp_patch_enabled() given its implementation, but it would look
> > > counter-intuitive here.
> > > 
> > > The rest looks fine.
> > > 
> > > However, I am not sure if the outcome is better than what we have. Yes,
> > > patch->enabled is not technically necessary and we can live with that (as
> > > the patch proves). On the other hand, it gives the reader clear guidance
> > > about the patch's state. klp_patch_enabled() is not a complete
> > > replacement. We have to call list_empty() in __klp_enable_patch() or check
> > > the original klp_target_state in klp_try_complete_transition().
> > > 
> > > I am not against the change, I am glad to see it is achievable, but I am
> > > not sure if the code is better with it. Joe acked it. What do the others
> > > think?
> > 
> > Let me qualify my ack -- I think minimizing the number of state variables
> > like patch->enabled can help readability... on the other hand, deducing the
> > same information from other properties like list-empty can be confusing, ie,
> > klp_patch_enabled() is generally a lot clearer than
> > list_empty(&patch->list)).
> > 
> > So I like this idea and would be interested to hear what folks think about
> > the exception cases you pointed out.
> 
> I share Miroslav and Joe's ambivalence.  It's interesting to see that it
> can be done, and normally I'd prefer to get rid of extraneous data
> fields, but the patch doesn't reduce code, and it even makes the code
> slightly more complex IMO, because klp_patch_enabled() doesn't always
> work like you'd expect.
> 
> So while I suggested it to begin with, I'm going to go with a NACK :-)

Fair enough. I took this patch as an experiment that might fail :-)

Best Regards,
Petr

Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest

[Miroslav Benes]

On Wed, 30 Jan 2019, Petr Mladek wrote:

> On Mon 2019-01-21 13:14:38, Miroslav Benes wrote:
> > Hi,
> > 
> > On Wed, 16 Jan 2019, Petr Mladek wrote:
> > 
> > > Do not dereference pointers to the shadow variables when either
> > > klp_shadow_alloc() or klp_shadow_get() fail.
> > 
> > I may misunderstand the patch, so bear with me, please. Is this because of 
> > a possible null pointer dereference? If yes, shouldn't this say rather 
> > "when both klp_shadow_alloc() and klp_shadow_get() fail"?
> 
> Well, klp_shadow_get() could fail also from other reasons if there is
> a bug in the implementation.

Yes, but I meant that if only klp_shadow_alloc() or klp_shadow_get() 
failed, it would be caught by ret == sv1 comparison and you would not need 
to add checking of ret at the beginning.
 
> > > There is no need to check the other locations explicitly. The test
> > > would fail if any allocation fails. And the existing messages, printed
> > > during the test, provide enough information to debug eventual problems.
> 
> Heh, this is actually the reason why I did not add the check
> for shadow_alloc(). Any error would be detected later
> with klp_shadow_get() calls that should get tested anyway.
> 
> Hmm, when I think about it. A good practice is to handle
> klp_shadow_allow() or klp_shadow_get() failures immediately.
> After all, it is the sample code that people might follow.

I think so. 

> > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > > ---
> > >  lib/livepatch/test_klp_shadow_vars.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
> > > index 02f892f941dc..55e6820430dc 100644
> > > --- a/lib/livepatch/test_klp_shadow_vars.c
> > > +++ b/lib/livepatch/test_klp_shadow_vars.c
> > > @@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
> > >  	 * to expected data.
> > >  	 */
> > >  	ret = shadow_get(obj, id);
> > > -	if (ret == sv1 && *sv1 == &var1)
> > > +	if (ret && 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 == sv2 && *sv2 == &var2)
> > > +	if (ret && 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 == sv3 && *sv3 == &var3)
> > > +	if (ret && ret == sv3 && *sv3 == &var3)
> > >  		pr_info("  got expected PTR%d -> PTR%d result\n",
> > >  			ptr_id(sv3), ptr_id(*sv3));
> > 
> > There is one more similar site calling shadow_get(obj, id + 1) which is 
> > fixed.
> 
> Heh, I think that I did not add the check there on purpose.
> If we are here, shadow_get(obj, id + 1) must have already succeeded
> above.

Yes, but if it failed, you would not notice. The message would not be 
printed and that's all. So it is possible to run into the same problematic 
error condition here.

> But it is a bad practice. We should always check the output.
> I'll do so in v2.

Agreed.

Miroslav

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

[Joe Lawrence]

On Wed, Jan 16, 2019 at 05:17:16PM +0100, Petr Mladek wrote:
> This patchset implements ideas that were mentioned and postponed during
> the review of the atomic replace patchset. I hope that I did not miss
> anything.
> 
> Well, I did not add __used attribute to avoid non-static warnings
> in modules for the selftest. The work on the sample modules somehow
> stalled.
> 
> BTW: Does it make sense to maintain the sample modules any longer?
> We could point people to the modules used by the selftest instead.
> 
> 
> The patches apply on top of livepatching.git, branch
> origin/for-5.1/atomic-replace.
> 
> 
> Petr Mladek (4):
>   livepatch: Introduce klp_for_each_patch macro
>   livepatch: Handle failing allocation of shadow variables in the
>     selftest
>   livepatch: Module coming and going callbacks can proceed all listed
>     patches
>   livepatch: Remove the redundant enabled flag in struct klp_patch
> 
>  include/linux/livepatch.h            |  2 --
>  kernel/livepatch/core.c              | 57 ++++++++++++++++--------------------
>  kernel/livepatch/core.h              |  6 ++++
>  kernel/livepatch/transition.c        |  9 +++---
>  kernel/livepatch/transition.h        |  1 +
>  lib/livepatch/test_klp_shadow_vars.c |  8 ++---
>  6 files changed, 40 insertions(+), 43 deletions(-)
> 
> -- 
> 2.13.7
> 

Hi Petr,

This change is trivial, but since folks are letting loose various static
code analysers on the livepatch samples and selftests, could you add
this to your patchset.  The shadow variable selftest is happy with this
change since it expects to see specific (non-negative) ptr_id values.

Thanks,

-- Joe

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

From e4c1e95a2145405115a984c6567740d12f20ccb7 Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@redhat.com>
Date: Fri, 1 Feb 2019 10:53:25 -0500
Subject: [PATCH] livepatch: return -ENOMEM on ptr_id() allocation failure

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

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

[Petr Mladek]

On Fri 2019-02-01 11:03:03, Joe Lawrence wrote:
> On Wed, Jan 16, 2019 at 05:17:16PM +0100, Petr Mladek wrote:
> > This patchset implements ideas that were mentioned and postponed during
> > the review of the atomic replace patchset. I hope that I did not miss
> > anything.
> > 
> > Well, I did not add __used attribute to avoid non-static warnings
> > in modules for the selftest. The work on the sample modules somehow
> > stalled.
> > 
> > BTW: Does it make sense to maintain the sample modules any longer?
> > We could point people to the modules used by the selftest instead.
> > 
> > 
> > The patches apply on top of livepatching.git, branch
> > origin/for-5.1/atomic-replace.
> > 
> > 
> > Petr Mladek (4):
> >   livepatch: Introduce klp_for_each_patch macro
> >   livepatch: Handle failing allocation of shadow variables in the
> >     selftest
> >   livepatch: Module coming and going callbacks can proceed all listed
> >     patches
> >   livepatch: Remove the redundant enabled flag in struct klp_patch
> > 
> >  include/linux/livepatch.h            |  2 --
> >  kernel/livepatch/core.c              | 57 ++++++++++++++++--------------------
> >  kernel/livepatch/core.h              |  6 ++++
> >  kernel/livepatch/transition.c        |  9 +++---
> >  kernel/livepatch/transition.h        |  1 +
> >  lib/livepatch/test_klp_shadow_vars.c |  8 ++---
> >  6 files changed, 40 insertions(+), 43 deletions(-)
> > 
> > -- 
> > 2.13.7
> > 
> 
> Hi Petr,
> 
> This change is trivial, but since folks are letting loose various static
> code analysers on the livepatch samples and selftests, could you add
> this to your patchset.  The shadow variable selftest is happy with this
> change since it expects to see specific (non-negative) ptr_id values.

Sure. I am going to queue the patch into v2.

Best Regards,
Petr