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