* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-02 15:56 [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails Joe Lawrence
@ 2017-10-05 21:55 ` Josh Poimboeuf
2017-10-06 22:20 ` Josh Poimboeuf
2017-10-06 15:14 ` Josh Poimboeuf
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Josh Poimboeuf @ 2017-10-05 21:55 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, linux-kernel, Jessica Yu, Jiri Kosina,
Miroslav Benes, Petr Mladek
On Mon, Oct 02, 2017 at 11:56:48AM -0400, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
>
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
>
> - cleanup comment describing the new function
> - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> - added a suggested-by tag for Petr since he suggested both code and
> commentary :)
>
> kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..bf8c8fd72589 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
> }
> EXPORT_SYMBOL_GPL(klp_register_patch);
>
> +/*
> + * Remove parts of patches that touch a given kernel module. The list of
> + * patches processed might be limited. When limit is NULL, all patches
> + * will be handled.
> + */
> +static void klp_cleanup_module_patches_limited(struct module *mod,
> + struct klp_patch *limit)
One nit, I think the function name is too verbose. It already has a
'limit' argument, so I think putting 'limited' in the name is too much
detail. I would just call it klp_cleanup_module_patches().
Otherwise the patch looks great. Thanks for fixing it!
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
--
Josh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-05 21:55 ` Josh Poimboeuf
@ 2017-10-06 22:20 ` Josh Poimboeuf
0 siblings, 0 replies; 8+ messages in thread
From: Josh Poimboeuf @ 2017-10-06 22:20 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, linux-kernel, Jessica Yu, Jiri Kosina,
Miroslav Benes, Petr Mladek
On Thu, Oct 05, 2017 at 04:55:43PM -0500, Josh Poimboeuf wrote:
> On Mon, Oct 02, 2017 at 11:56:48AM -0400, Joe Lawrence wrote:
> > When an incoming module is considered for livepatching by
> > klp_module_coming(), it iterates over multiple patches and multiple
> > kernel objects in this order:
> >
> > list_for_each_entry(patch, &klp_patches, list) {
> > klp_for_each_object(patch, obj) {
> >
> > which means that if one of the kernel objects fails to patch,
> > klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> > objects that were already patched by a previous patch.
> >
> > Reported-by: Miroslav Benes <mbenes@suse.cz>
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> > v2:
> >
> > - cleanup comment describing the new function
> > - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> > - added a suggested-by tag for Petr since he suggested both code and
> > commentary :)
> >
> > kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 37 insertions(+), 23 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..bf8c8fd72589 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
> > }
> > EXPORT_SYMBOL_GPL(klp_register_patch);
> >
> > +/*
> > + * Remove parts of patches that touch a given kernel module. The list of
> > + * patches processed might be limited. When limit is NULL, all patches
> > + * will be handled.
> > + */
> > +static void klp_cleanup_module_patches_limited(struct module *mod,
> > + struct klp_patch *limit)
>
> One nit, I think the function name is too verbose. It already has a
> 'limit' argument, so I think putting 'limited' in the name is too much
> detail. I would just call it klp_cleanup_module_patches().
I rescind my nit. I forgot that the 'limited' suffix is already a
convention used throughout the livepatch code. So my ack is
unconditional now.
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
--
Josh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-02 15:56 [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails Joe Lawrence
2017-10-05 21:55 ` Josh Poimboeuf
@ 2017-10-06 15:14 ` Josh Poimboeuf
2017-10-09 14:49 ` Miroslav Benes
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Josh Poimboeuf @ 2017-10-06 15:14 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, linux-kernel, Jessica Yu, Jiri Kosina,
Miroslav Benes, Petr Mladek
On Mon, Oct 02, 2017 at 11:56:48AM -0400, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
>
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
>
> - cleanup comment describing the new function
> - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> - added a suggested-by tag for Petr since he suggested both code and
> commentary :)
Any more comments on this patch? This is holding up the callbacks patch
so it would be great if we could get this merged soon.
--
Josh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-02 15:56 [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails Joe Lawrence
2017-10-05 21:55 ` Josh Poimboeuf
2017-10-06 15:14 ` Josh Poimboeuf
@ 2017-10-09 14:49 ` Miroslav Benes
2017-10-09 16:26 ` Josh Poimboeuf
2017-10-10 12:31 ` Petr Mladek
2017-10-11 13:39 ` Jiri Kosina
4 siblings, 1 reply; 8+ messages in thread
From: Miroslav Benes @ 2017-10-09 14:49 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
Jiri Kosina, Petr Mladek
On Mon, 2 Oct 2017, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
>
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
>
> - cleanup comment describing the new function
> - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> - added a suggested-by tag for Petr since he suggested both code and
> commentary :)
>
> kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..bf8c8fd72589 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
> }
> EXPORT_SYMBOL_GPL(klp_register_patch);
>
> +/*
> + * Remove parts of patches that touch a given kernel module. The list of
> + * patches processed might be limited. When limit is NULL, all patches
> + * will be handled.
> + */
> +static void klp_cleanup_module_patches_limited(struct module *mod,
> + struct klp_patch *limit)
> +{
> + struct klp_patch *patch;
> + struct klp_object *obj;
> +
> + list_for_each_entry(patch, &klp_patches, list) {
> + if (patch == limit)
> + break;
> +
> + klp_for_each_object(patch, obj) {
> + 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) {
> + pr_notice("reverting patch '%s' on unloading module '%s'\n",
> + patch->mod->name, obj->mod->name);
> + klp_unpatch_object(obj);
> + }
> +
> + klp_free_object_loaded(obj);
> + break;
> + }
> + }
> +}
> +
> int klp_module_coming(struct module *mod)
> {
> int ret;
> @@ -894,7 +929,7 @@ int klp_module_coming(struct module *mod)
> pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> patch->mod->name, obj->mod->name, obj->mod->name);
> mod->klp_alive = false;
> - klp_free_object_loaded(obj);
> + klp_cleanup_module_patches_limited(mod, patch);
> mutex_unlock(&klp_mutex);
>
> return ret;
> @@ -902,9 +937,6 @@ int klp_module_coming(struct module *mod)
>
> void klp_module_going(struct module *mod)
> {
> - struct klp_patch *patch;
> - struct klp_object *obj;
> -
> if (WARN_ON(mod->state != MODULE_STATE_GOING &&
> mod->state != MODULE_STATE_COMING))
> return;
> @@ -917,25 +949,7 @@ void klp_module_going(struct module *mod)
> */
> mod->klp_alive = false;
>
> - list_for_each_entry(patch, &klp_patches, list) {
> - klp_for_each_object(patch, obj) {
> - 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) {
> - pr_notice("reverting patch '%s' on unloading module '%s'\n",
> - patch->mod->name, obj->mod->name);
> - klp_unpatch_object(obj);
> - }
> -
> - klp_free_object_loaded(obj);
> - break;
> - }
> - }
> + klp_cleanup_module_patches_limited(mod, NULL);
>
> mutex_unlock(&klp_mutex);
> }
Well, I don't know. I like the idea of reusing the code a lot, but it
feels odd not to use list_for_each_entry_{continue,from}_reverse()
iterator. And I'm not talking about _reverse there (more on that later).
That continue part gives us limited functionality for free. We cannot do
the same in klp_free_funcs_limited(), because klp_funcs form an array. It
is not a list.
On the other hand, the code would be slightly more complicated, because
only the inner part of the loop could be reused.
Now about _reverse. I don't know about that either. The module's code is
not used yet when klp_module_coming() is called (or in
klp_module_going()). So it is probable that the order does not matter at
all. But it would be the correct way to do it.
To sum it up, I'm able to live with the proposed approach if that's the
consensus, because I haven't managed to convince myself that my proposal
would be better.
Thanks,
Miroslav
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-09 14:49 ` Miroslav Benes
@ 2017-10-09 16:26 ` Josh Poimboeuf
0 siblings, 0 replies; 8+ messages in thread
From: Josh Poimboeuf @ 2017-10-09 16:26 UTC (permalink / raw)
To: Miroslav Benes
Cc: Joe Lawrence, live-patching, linux-kernel, Jessica Yu,
Jiri Kosina, Petr Mladek
On Mon, Oct 09, 2017 at 04:49:29PM +0200, Miroslav Benes wrote:
> On Mon, 2 Oct 2017, Joe Lawrence wrote:
>
> > When an incoming module is considered for livepatching by
> > klp_module_coming(), it iterates over multiple patches and multiple
> > kernel objects in this order:
> >
> > list_for_each_entry(patch, &klp_patches, list) {
> > klp_for_each_object(patch, obj) {
> >
> > which means that if one of the kernel objects fails to patch,
> > klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> > objects that were already patched by a previous patch.
> >
> > Reported-by: Miroslav Benes <mbenes@suse.cz>
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> > v2:
> >
> > - cleanup comment describing the new function
> > - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> > - added a suggested-by tag for Petr since he suggested both code and
> > commentary :)
> >
> > kernel/livepatch/core.c | 60 ++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 37 insertions(+), 23 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..bf8c8fd72589 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -830,6 +830,41 @@ int klp_register_patch(struct klp_patch *patch)
> > }
> > EXPORT_SYMBOL_GPL(klp_register_patch);
> >
> > +/*
> > + * Remove parts of patches that touch a given kernel module. The list of
> > + * patches processed might be limited. When limit is NULL, all patches
> > + * will be handled.
> > + */
> > +static void klp_cleanup_module_patches_limited(struct module *mod,
> > + struct klp_patch *limit)
> > +{
> > + struct klp_patch *patch;
> > + struct klp_object *obj;
> > +
> > + list_for_each_entry(patch, &klp_patches, list) {
> > + if (patch == limit)
> > + break;
> > +
> > + klp_for_each_object(patch, obj) {
> > + 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) {
> > + pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > + patch->mod->name, obj->mod->name);
> > + klp_unpatch_object(obj);
> > + }
> > +
> > + klp_free_object_loaded(obj);
> > + break;
> > + }
> > + }
> > +}
> > +
> > int klp_module_coming(struct module *mod)
> > {
> > int ret;
> > @@ -894,7 +929,7 @@ int klp_module_coming(struct module *mod)
> > pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> > patch->mod->name, obj->mod->name, obj->mod->name);
> > mod->klp_alive = false;
> > - klp_free_object_loaded(obj);
> > + klp_cleanup_module_patches_limited(mod, patch);
> > mutex_unlock(&klp_mutex);
> >
> > return ret;
> > @@ -902,9 +937,6 @@ int klp_module_coming(struct module *mod)
> >
> > void klp_module_going(struct module *mod)
> > {
> > - struct klp_patch *patch;
> > - struct klp_object *obj;
> > -
> > if (WARN_ON(mod->state != MODULE_STATE_GOING &&
> > mod->state != MODULE_STATE_COMING))
> > return;
> > @@ -917,25 +949,7 @@ void klp_module_going(struct module *mod)
> > */
> > mod->klp_alive = false;
> >
> > - list_for_each_entry(patch, &klp_patches, list) {
> > - klp_for_each_object(patch, obj) {
> > - 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) {
> > - pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > - patch->mod->name, obj->mod->name);
> > - klp_unpatch_object(obj);
> > - }
> > -
> > - klp_free_object_loaded(obj);
> > - break;
> > - }
> > - }
> > + klp_cleanup_module_patches_limited(mod, NULL);
> >
> > mutex_unlock(&klp_mutex);
> > }
>
> Well, I don't know. I like the idea of reusing the code a lot, but it
> feels odd not to use list_for_each_entry_{continue,from}_reverse()
> iterator. And I'm not talking about _reverse there (more on that later).
> That continue part gives us limited functionality for free. We cannot do
> the same in klp_free_funcs_limited(), because klp_funcs form an array. It
> is not a list.
>
> On the other hand, the code would be slightly more complicated, because
> only the inner part of the loop could be reused.
>
> Now about _reverse. I don't know about that either. The module's code is
> not used yet when klp_module_coming() is called (or in
> klp_module_going()). So it is probable that the order does not matter at
> all. But it would be the correct way to do it.
>
> To sum it up, I'm able to live with the proposed approach if that's the
> consensus, because I haven't managed to convince myself that my proposal
> would be better.
As you said, the order doesn't matter because the code isn't runnable.
And doing it this way allows us to share more code. So I like the patch
as it is.
--
Josh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-02 15:56 [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails Joe Lawrence
` (2 preceding siblings ...)
2017-10-09 14:49 ` Miroslav Benes
@ 2017-10-10 12:31 ` Petr Mladek
2017-10-11 13:39 ` Jiri Kosina
4 siblings, 0 replies; 8+ messages in thread
From: Petr Mladek @ 2017-10-10 12:31 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
Jiri Kosina, Miroslav Benes
On Mon 2017-10-02 11:56:48, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
>
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Looks good to me.
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails
2017-10-02 15:56 [PATCH v2] livepatch: unpatch all klp_objects if klp_module_coming fails Joe Lawrence
` (3 preceding siblings ...)
2017-10-10 12:31 ` Petr Mladek
@ 2017-10-11 13:39 ` Jiri Kosina
4 siblings, 0 replies; 8+ messages in thread
From: Jiri Kosina @ 2017-10-11 13:39 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
Miroslav Benes, Petr Mladek
On Mon, 2 Oct 2017, Joe Lawrence wrote:
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
>
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> v2:
>
> - cleanup comment describing the new function
> - s/klp_cleanup_module_objects_limited/klp_cleanup_module_patches_limited
> - added a suggested-by tag for Petr since he suggested both code and
> commentary :)
Appplied to for-4.14/upstream-fixes. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 8+ messages in thread