Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

Joe Lawrence
2017-09-20T15:17:23Z

On Wed, Sep 20, 2017 at 01:19:05PM +0200, Miroslav Benes wrote:
> On Wed, 13 Sep 2017, Joe Lawrence wrote:
>
> > Hi Miroslav,
>
> Hi,
>
> sorry for the late response. I'm also travelling now and we have SUSECon
> conference next week, so just a quick answer. It looks ok at first glance,
> but I need to take a proper look.

No problem, thanks for the update and safe travels.

> > I worked out the code that I posted earlier today and I think this could
> > address the multiple-patch module_coming() issue you pointed out.
> >
> > Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
> > callbacks" patchset, so it includes unpatching callbacks. I can easily
> > strip those out (and remove the additional debugging pr_'s) and make
> > this a stand-alone patch that would apply before the callback patchset.
>
> I think this would be better. Strip callbacks out and send this either
> separately (and base callbacks patch set on this), or make it 1/n of the
> series.

Agreed.

> > See the test case below.
> >
> > -- Joe
> >
> > Test X
> > ------
> >
> > Multiple livepatches targeting the same klp_objects may be loaded at
> > the same time. If a target module loads and any of the livepatch's
> > pre-patch callbacks fail, then the module is not allowed to load.
> > Furthermore, any livepatches that that did succeed will be reverted
> > (only the incoming module / klp_object) and their pre/post-unpatch
> > callbacks executed.
> >
> > - load livepatch
> > - load livepatch2
> > - load livepatch3
> > - setup livepatch3 pre-patch return of -ENODEV
> > - load target module (should fail)
> > - disable livepatch3
> > - disable livepatch2
> > - disable livepatch
> > - unload livepatch3
> > - unload livepatch2
> > - unload livepatch
> >
> >
> > Load three livepatches, each target a livepatch_callbacks_mod module and
> > vmlinux:
> >
> > % insmod samples/livepatch/livepatch-callbacks-demo.ko
> > [ 26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
> > [ 26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
> > [ 26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> > [ 26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
> > [ 27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
> > [ 27.744130] livepatch: 'livepatch_callbacks_demo': patching complete
> >
> > % insmod samples/livepatch/livepatch-callbacks-demo2.ko
> > [ 29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
> > [ 29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
> > [ 29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
> > [ 30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
> > [ 30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete
> >
> > % insmod samples/livepatch/livepatch-callbacks-demo3.ko
> > [ 32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
> > [ 32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
> > [ 32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
> > [ 33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
> > [ 33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete
> >
> > Setup the third livepatch to fail its pre-patch callback when the target
> > module is loaded:
> >
> > % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret
> >
> > Load the target module:
> >
> > % insmod samples/livepatch/livepatch-callbacks-mod.ko
> >
> > The first livepatch pre-patch callback succeeds, the klp_object is
> > patched, and its post-patch callback is executed:
> >
> > [ 38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
> > [ 38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> > [ 38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >
> > Likewise for the second livepatch:
> >
> > [ 38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
> > [ 38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> > [ 38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >
> > But the third livepatch fails its pre-patch callback:
> >
> > [ 38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
> > [ 38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
> >
> > We refuse to load the target module:
> >
> > [ 38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
> >
> > So we double back and unpatch (including pre-unpatch and post-unpatch
> > callbacks) the first livepatch, then the second:
> >
> > [ 38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> > [ 38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> > [ 38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >
> > Finally the module loader reports an error:
> >
> > [ 38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
> >
> > Clean it all up:
> >
> > % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
> > [ 41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
> > [ 41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
> > [ 42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
> > [ 42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
> >
> > % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
> > [ 47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
> > [ 47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
> > [ 48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
> > [ 48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> >
> > % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
> > [ 53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
> > [ 53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
> > [ 54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
> > [ 54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete
> >
> > % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
> > % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
> > % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> >
> >
> > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> >
> > >From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
> > From: Joe Lawrence <joe.lawrence@redhat.com>
> > Date: Wed, 13 Sep 2017 16:51:13 -0400
> > Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails
> >
> > 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 fail to patch for whatever
> > reason, klp_module_coming()'s error path should double back and unpatch
> > any previous kernel object that was patched for a previous patch.
> >
> > Reported-by: Miroslav Benes <mbenes@suse.cz>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> > kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index aca62c4b8616..7f5192618cc8 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
> > goto err;
> > }
> >
> > +pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> > +
> > ret = klp_patch_object(obj);
> > if (ret) {
> > pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > @@ -919,7 +921,33 @@ 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);
> > +
> > + /*
> > + * Run back through the patch list and unpatch any klp_object that
> > + * was patched before hitting an error above.
> > + */
> > +
> > + list_for_each_entry(patch, &klp_patches, list) {
>
> I think it would be safer to use
> list_for_each_entry_{continue,from}_reverse() iterator (probably
> _continue_reverse(), because the current patch failed). That would unpatch
> the objects in the correct order (see your test case above) and it is
> also an optimization because you'd process only those patches which were
> walked through during the first loop.

I had originally tested with list_for_each_entry_reverse(), but then
noticed that klp_module_going() iterates through the patches using
list_for_each_entry(). Strictly speaking, there is also the matter of
the klp_objects, but we don't have a klp_for_each_object_reverse() macro
that would complete the mirrored-symmetry.

For pre/post-(un)patch callbacks, they are supposed to be independent
from each other anyway, so theoretically their execution order shouldn't
matter.

That said, maybe we can compromise on list_for_each_entry_reverse() for
both klp_module_going() and the klp_module_coming() error path above?

> > +
> > + if (!patch->enabled || patch == klp_transition_patch)
> > + continue;
>
> Is the second part with klp_transition_patch correct? Yes, we need to skip
> disabled patches. No question about that. But klp_transition_patch seems
> odd. It is true, that (if I am not mistaken) klp_transition_patch is the
> last patch in patches list which is relevant (because we cannot
> enable/disable any random patch in the list). If that failed to patch,
> you wouldn't need to worry about it anyway, because you need to process
> previous patches only. Am I missing something? So I think it can stay,
> yes. But I'd like to understand it.

You might be correct here, I basically copy/pasted it from the code
above with the understanding that the klp_transition_patch was handled
by klp_complete_transition(). If it is an unneeded check, then I can
remove it.

Thanks,

-- Joe