From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751364AbdIMNxX (ORCPT ); Wed, 13 Sep 2017 09:53:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35930 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750999AbdIMNxU (ORCPT ); Wed, 13 Sep 2017 09:53:20 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4024B5F7B2 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=joe.lawrence@redhat.com Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks To: Miroslav Benes , Josh Poimboeuf Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu , Jiri Kosina , Petr Mladek , Chris J Arges References: <1504191233-2642-1-git-send-email-joe.lawrence@redhat.com> <1504191233-2642-2-git-send-email-joe.lawrence@redhat.com> <20170912220544.zdgh65o4aqmd5ni4@redhat.com> <20170912222245.bzvwzef27ul77xqm@treble> From: Joe Lawrence Organization: Red Hat Message-ID: <6ed9c3ac-ac8f-8678-a884-754cdd07b447@redhat.com> Date: Wed, 13 Sep 2017 09:53:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 13 Sep 2017 13:53:20 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/13/2017 03:29 AM, Miroslav Benes wrote: > On Tue, 12 Sep 2017, Josh Poimboeuf wrote: > >> On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote: >>> On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote: >>> I've re-read this a few times, and I think I might have been originally >>> off-base with what I thought you were concerned about. But I think I >>> grok it now: the problem you pointed out arises because >>> klp_module_coming() iterates like so: >>> >>> for each klp_patch >>> for each kobj in klp_patch >>> >>> which means that we may have made pre-patch callbacks and patched a >>> given kobj for an earlier klp_patch that now fails for a later >>> klp_patch. > > Yes, that's the scenario. > >>> What should be the defined behavior in this case? I would expect that >>> we need to unpatch all similar kobjs across klp_patches which have >>> already been successfully patched. In turn, their post-unpatch >>> callbacks should be invoked. >>> >>> If that's true, maybe this would make a better follow-on patch. > > Yes, you'd need to loop back, unpatch everything and call post-unpatch > callbacks too. Probably too much for this patch set, so we can deal with > the problem later. Completely untested/compiled, but something like (sans callbacks)? --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -894,6 +894,29 @@ 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); + + /* + * Run back through the patch list and unpatch any klp_object + * that matches the one we errored on above. + */ + list_for_each_entry(patch, &klp_patches, list) { + + if (!patch->enabled || patch == klp_transition_patch) + continue; + + klp_for_each_object(patch, obj) { + + if (!obj->patched || + !klp_is_module(obj) || + strcmp(obj->name, mod->name)) + continue; + + klp_unpatch_object(obj); + /* post-unpatch callback would go here */ + + break; + } + } + mod->klp_alive = false; klp_free_object_loaded(obj); mutex_unlock(&klp_mutex); >> The rabbit hole seems to be getting deeper, is it really worth it? I'd >> rather we just make the pre-patch handler return void and be done with >> it, as Joe originally proposed. >> >> So far, allowing the pre-patch handler to halt patching is a purely >> theoretical feature, nobody even knows if we need it yet, and whether >> it's worth the pain. So I'd vote to just simplify this mess and let >> whoever wants the feature try to implement it :-) "Rabbit hole" is an apt description :) the question is whether this is the last hurdle, or just one of many more coming our way. I'd like to think the former, but I'm the guy down in the rabbit hole, so my perspective is tainted. Ripping this out of the code would be relatively easy. Re-doing the tests/documentation/comments will be a bit of work to reword everything. > Unfortunately, the problem is there even without Joe's callbacks. If it > was only a problem of callbacks, I'd go along with you. I see two options. > > 1. we'll fix this for klp_patch_object(). Then callbacks' problem would be > simple to solve, because the infrastructure would be already there. If the bugfix is like the above, then it's not too bad a diversion. I can run some tests this afternoon to try and tackle this. > 2. we'll remove any error handling from klp_coming_module and we'll allow > target modules to load even with a patching failure. This doesn't seem to > be the right approach... I agree. -- Joe