This patchset implements ideas that were mentioned and postponed during
the review of the atomic replace patchset.
The patches apply on top of livepatching.git, branch
origin/for-5.1/atomic-replace.
Changes against v1:
+ Added Joe's patch that fixed ptr_id() error code [Joe]
+ Did proper error handling in the shadow variable sefttest [All]
+ Removed the controversial patch that was removing patch->enabled flag [All].
+ Fixed few typo's [Joe]
+ Added available Acks.
Joe Lawrence (1):
livepatch: return -ENOMEM on ptr_id() allocation failure
Petr Mladek (3):
livepatch: Introduce klp_for_each_patch macro
livepatch: Proper error handling in the shadow variables selftest
livepatch: Module coming and going callbacks can proceed with all
listed patches
kernel/livepatch/core.c | 34 ++++++++++------------------------
kernel/livepatch/core.h | 6 ++++++
kernel/livepatch/transition.c | 2 +-
lib/livepatch/test_klp_shadow_vars.c | 24 +++++++++++++++++++++++-
4 files changed, 40 insertions(+), 26 deletions(-)
--
2.13.7
From: Joe Lawrence <[email protected]>
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]>
Signed-off-by: Petr Mladek <[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.13.7
Add proper error handling when allocating or getting shadow variables
in the selftest. It prevents an invalid pointer access in some situations.
It shows the good programming practice in the others.
The error codes are just the best guess and specific for this particular
test. In general, klp_shadow_alloc() returns NULL also when the given
shadow variable has already been allocated. In addition, both
klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from
other reasons when the constructor fails.
Note, that the error code is not really important even in the real life.
The use of shadow variables should be transparent for the original
livepatched code.
Signed-off-by: Petr Mladek <[email protected]>
---
lib/livepatch/test_klp_shadow_vars.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index f5441c193166..fe5c413efe96 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -154,22 +154,37 @@ static int test_klp_shadow_vars_init(void)
* Allocate a few shadow variables with different <obj> and <id>.
*/
sv1 = shadow_alloc(obj, id, size, gfp_flags, shadow_ctor, &var1);
+ if (!sv1)
+ return -ENOMEM;
+
sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2);
+ if (!sv2)
+ return -ENOMEM;
+
sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3);
+ if (!sv3)
+ return -ENOMEM;
/*
* Verify we can find our new shadow variables and that they point
* to expected data.
*/
ret = shadow_get(obj, id);
+ if (!ret)
+ return -EINVAL;
if (ret == sv1 && *sv1 == &var1)
pr_info(" got expected PTR%d -> PTR%d result\n",
ptr_id(sv1), ptr_id(*sv1));
+
ret = shadow_get(obj + 1, id);
+ if (!ret)
+ return -EINVAL;
if (ret == sv2 && *sv2 == &var2)
pr_info(" got expected PTR%d -> PTR%d result\n",
ptr_id(sv2), ptr_id(*sv2));
ret = shadow_get(obj, id + 1);
+ if (!ret)
+ return -EINVAL;
if (ret == sv3 && *sv3 == &var3)
pr_info(" got expected PTR%d -> PTR%d result\n",
ptr_id(sv3), ptr_id(*sv3));
@@ -179,7 +194,12 @@ static int test_klp_shadow_vars_init(void)
* The second invocation should return the same shadow var.
*/
sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
+ if (!sv4)
+ return -ENOMEM;
+
ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
+ if (!ret)
+ return -EINVAL;
if (ret == sv4 && *sv4 == &var4)
pr_info(" got expected PTR%d -> PTR%d result\n",
ptr_id(sv4), ptr_id(*sv4));
@@ -207,6 +227,8 @@ static int test_klp_shadow_vars_init(void)
* We should still find an <id+1> variable.
*/
ret = shadow_get(obj, id + 1);
+ if (!ret)
+ return -EINVAL;
if (ret == sv3 && *sv3 == &var3)
pr_info(" got expected PTR%d -> PTR%d result\n",
ptr_id(sv3), ptr_id(*sv3));
--
2.13.7
Livepatches can no longer get enabled and disabled repeatedly.
The list klp_patches contains only enabled patches and eventually
the patch in transition.
The module coming and going callbacks do no longer need to check
for these state. They have to proceed with all listed patches.
Suggested-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
Acked-by: Joe Lawrence <[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
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]>
Acked-by: Joe Lawrence <[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
On Mon, 4 Feb 2019, Petr Mladek wrote:
> From: Joe Lawrence <[email protected]>
>
> 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]>
> Signed-off-by: Petr Mladek <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
Miroslav
On Mon, 4 Feb 2019, Petr Mladek wrote:
> Add proper error handling when allocating or getting shadow variables
> in the selftest. It prevents an invalid pointer access in some situations.
> It shows the good programming practice in the others.
>
> The error codes are just the best guess and specific for this particular
> test. In general, klp_shadow_alloc() returns NULL also when the given
> shadow variable has already been allocated. In addition, both
> klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from
> other reasons when the constructor fails.
>
> Note, that the error code is not really important even in the real life.
> The use of shadow variables should be transparent for the original
> livepatched code.
>
> Signed-off-by: Petr Mladek <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
Miroslav
On 2/4/19 8:56 AM, Petr Mladek wrote:
> Add proper error handling when allocating or getting shadow variables
> in the selftest. It prevents an invalid pointer access in some situations.
> It shows the good programming practice in the others.
>
> The error codes are just the best guess and specific for this particular
> test. In general, klp_shadow_alloc() returns NULL also when the given
> shadow variable has already been allocated. In addition, both
> klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from
> other reasons when the constructor fails.
>
> Note, that the error code is not really important even in the real life.
> The use of shadow variables should be transparent for the original
> livepatched code.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> lib/livepatch/test_klp_shadow_vars.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
> index f5441c193166..fe5c413efe96 100644
> --- a/lib/livepatch/test_klp_shadow_vars.c
> +++ b/lib/livepatch/test_klp_shadow_vars.c
> @@ -154,22 +154,37 @@ static int test_klp_shadow_vars_init(void)
> * Allocate a few shadow variables with different <obj> and <id>.
> */
> sv1 = shadow_alloc(obj, id, size, gfp_flags, shadow_ctor, &var1);
> + if (!sv1)
> + return -ENOMEM;
> +
> sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2);
> + if (!sv2)
> + return -ENOMEM;
> +
> sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3);
> + if (!sv3)
> + return -ENOMEM;
>
> /*
> * Verify we can find our new shadow variables and that they point
> * to expected data.
> */
> ret = shadow_get(obj, id);
> + if (!ret)
> + return -EINVAL;
> if (ret == sv1 && *sv1 == &var1)
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv1), ptr_id(*sv1));
> +
> ret = shadow_get(obj + 1, id);
> + if (!ret)
> + return -EINVAL;
> if (ret == sv2 && *sv2 == &var2)
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv2), ptr_id(*sv2));
> ret = shadow_get(obj, id + 1);
> + if (!ret)
> + return -EINVAL;
> if (ret == sv3 && *sv3 == &var3)
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv3), ptr_id(*sv3));
> @@ -179,7 +194,12 @@ static int test_klp_shadow_vars_init(void)
> * The second invocation should return the same shadow var.
> */
> sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
> + if (!sv4)
> + return -ENOMEM;
> +
> ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
> + if (!ret)
> + return -EINVAL;
> if (ret == sv4 && *sv4 == &var4)
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv4), ptr_id(*sv4));
> @@ -207,6 +227,8 @@ static int test_klp_shadow_vars_init(void)
> * We should still find an <id+1> variable.
> */
> ret = shadow_get(obj, id + 1);
> + if (!ret)
> + return -EINVAL;
> if (ret == sv3 && *sv3 == &var3)
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv3), ptr_id(*sv3));
>
Fixes look good to me,
Acked-by: Joe Lawrence <[email protected]>
-- Joe
On Mon 2019-02-04 14:56:49, Petr Mladek wrote:
> This patchset implements ideas that were mentioned and postponed during
> the review of the atomic replace patchset.
>
> The patches apply on top of livepatching.git, branch
> origin/for-5.1/atomic-replace.
>
>
> Changes against v1:
>
> + Added Joe's patch that fixed ptr_id() error code [Joe]
> + Did proper error handling in the shadow variable sefttest [All]
> + Removed the controversial patch that was removing patch->enabled flag [All].
> + Fixed few typo's [Joe]
> + Added available Acks.
>
>
> Joe Lawrence (1):
> livepatch: return -ENOMEM on ptr_id() allocation failure
>
> Petr Mladek (3):
> livepatch: Introduce klp_for_each_patch macro
> livepatch: Proper error handling in the shadow variables selftest
> livepatch: Module coming and going callbacks can proceed with all
> listed patches
I have applied all 4 paches into for-5.1/atomic-replace branch.
Best Regards,
Petr
On Mon, Feb 04, 2019 at 02:56:49PM +0100, Petr Mladek wrote:
> This patchset implements ideas that were mentioned and postponed during
> the review of the atomic replace patchset.
>
> The patches apply on top of livepatching.git, branch
> origin/for-5.1/atomic-replace.
>
>
> Changes against v1:
>
> + Added Joe's patch that fixed ptr_id() error code [Joe]
> + Did proper error handling in the shadow variable sefttest [All]
> + Removed the controversial patch that was removing patch->enabled flag [All].
> + Fixed few typo's [Joe]
> + Added available Acks.
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh