From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751780AbdITPRX (ORCPT ); Wed, 20 Sep 2017 11:17:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50524 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751507AbdITPRW (ORCPT ); Wed, 20 Sep 2017 11:17:22 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CD40C356D7 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=joe.lawrence@redhat.com Date: Wed, 20 Sep 2017 11:17:19 -0400 From: Joe Lawrence To: Miroslav Benes Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Petr Mladek , Chris J Arges Subject: Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails Message-ID: <20170920151719.5qnegujtazay5mop@redhat.com> References: <6ed9c3ac-ac8f-8678-a884-754cdd07b447@redhat.com> <1505338598-16041-1-git-send-email-joe.lawrence@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-08-08) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 20 Sep 2017 15:17:22 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > 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 > > Signed-off-by: Joe Lawrence > > --- > > 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