Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751553AbdIMH3s (ORCPT ); Wed, 13 Sep 2017 03:29:48 -0400 Received: from mx2.suse.de ([195.135.220.15]:50947 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750949AbdIMH3o (ORCPT ); Wed, 13 Sep 2017 03:29:44 -0400 Date: Wed, 13 Sep 2017 09:29:41 +0200 (CEST) From: Miroslav Benes To: Josh Poimboeuf cc: Joe Lawrence , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu , Jiri Kosina , Petr Mladek , Chris J Arges Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks In-Reply-To: <20170912222245.bzvwzef27ul77xqm@treble> Message-ID: 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> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4255 Lines: 106 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: > > > On 09/12/2017 04:53 AM, Miroslav Benes wrote: > > > >> @@ -871,13 +882,27 @@ int klp_module_coming(struct module *mod) > > > >> pr_notice("applying patch '%s' to loading module '%s'\n", > > > >> patch->mod->name, obj->mod->name); > > > >> > > > >> + ret = klp_pre_patch_callback(obj); > > > >> + if (ret) { > > > >> + pr_warn("pre-patch callback failed for object '%s'\n", > > > >> + obj->name); > > > >> + goto err; > > > >> + } > > > > > > > > There is a problem here. We cycle through all enabled patches (or > > > > klp_transition_patch) and call klp_pre_patch_callback() everytime an > > > > enabled patch contains a patch for a coming module. Now, it can easily > > > > happen that klp_pre_patch_callback() fails. And not the first one from the > > > > first relevant patch, but the next one. In that case we need to call > > > > klp_post_unpatch_callback() for all already processed relevant patches in > > > > the error path. > > > > > > Good test case, if I understand you correctly: > > > > > > - Load target modules mod1 and mod2 > > > - Load a livepatch that targets mod1 and mod2 > > > - pre-patch succeeds for mod1 > > > - pre-patch fails for mod2 > > > > > > and then we should: > > > > > > - NOT run post-patch or pre/post-unpatch handlers for mod2 > > > - NOT run post-patch or pre-unpatch handlers for mod1 > > > - do run post-unpatch handler for mod1 > > > - Refuse to load the livepatch > > > > > > Does that sound right? > > > > Erm, probably not... > > > > > > Unfortunately, we need to do the same for klp_patch_object() below, > > > > because there is the same problem and we missed it. > > > > > > > >> + > > > >> ret = klp_patch_object(obj); > > > >> if (ret) { > > > >> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > > >> patch->mod->name, obj->mod->name, ret); > > > >> + > > > >> + if (patch != klp_transition_patch) > > > >> + klp_post_unpatch_callback(obj); > > > >> + > > > >> goto err; > > > > > > > > Here. > > > > > > > > Could you do it as a part of the patch set (or send it separately), > > > > please? > > > > 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. > 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 :-) 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. 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... Miroslav