Date:   Wed, 20 Sep 2017 11:17:19 -0400
From:   Joe Lawrence <joe.lawrence@...hat.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        Petr Mladek <pmladek@...e.com>,
        Chris J Arges <chris.j.arges@...onical.com>
Subject: Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming
 fails

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@...hat.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@...e.cz>
> > Signed-off-by: Joe Lawrence <joe.lawrence@...hat.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 

Powered by blists - more mailing lists