LinuxLists.cc - [PATCH 0/4] livepatch: Followup changes for the atomic replace patchset

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
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

2019-01-16 22:57:26

[permalink] [raw]

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

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 <[email protected]>
---
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

2019-01-16 22:58:06

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
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

2019-01-16 22:58:33

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
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

2019-01-21 12:12:31

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>

Acked-by: Miroslav Benes <[email protected]>

M

2019-01-21 12:16:16

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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

2019-01-21 17:25:53

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>

Acked-by: Miroslav Benes <[email protected]>

M

2019-01-21 22:36:18

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> 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 <[email protected]>

-- Joe

2019-01-21 22:41:53

[permalink] [raw]

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

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

2019-01-21 22:49:23

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> 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 <[email protected]>

-- Joe

2019-01-21 22:52:59

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> 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 <[email protected]>

-- Joe

2019-01-22 10:09:39

[permalink] [raw]

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

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

2019-01-23 18:29:24

[permalink] [raw]

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

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

2019-01-29 20:02:06

by Josh Poimboeuf

[permalink] [raw]

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

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

2019-01-30 08:46:30

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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

2019-01-30 08:56:45

[permalink] [raw]

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

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

2019-01-30 09:45:02

[permalink] [raw]

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

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

2019-01-31 08:42:15

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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

2019-02-01 16:03:29

[permalink] [raw]

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

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 <[email protected]>
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 <[email protected]>
---
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

2019-02-04 10:05:43