From patchwork Tue Feb 21 21:21:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Josh Poimboeuf X-Patchwork-Id: 730788 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vSYNl6fdxz9s1h for ; Wed, 22 Feb 2017 08:22:15 +1100 (AEDT) Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3vSYNl5nmgzDqHs for ; Wed, 22 Feb 2017 08:22:15 +1100 (AEDT) X-Original-To: linuxppc-dev@lists.ozlabs.org Delivered-To: linuxppc-dev@lists.ozlabs.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3vSYMc2PrlzDqBv for ; Wed, 22 Feb 2017 08:21:16 +1100 (AEDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0376E3D944; Tue, 21 Feb 2017 21:21:15 +0000 (UTC) Received: from treble (ovpn-121-112.rdu2.redhat.com [10.10.121.112] (may be forged)) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id v1LLLBnl031122; Tue, 21 Feb 2017 16:21:12 -0500 Date: Tue, 21 Feb 2017 15:21:10 -0600 From: Josh Poimboeuf To: Miroslav Benes Subject: Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model Message-ID: <20170221212110.y2pstgeluwrmxjie@treble> References: <20170216203156.igvctdmyt6b6zcgg@treble> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 21 Feb 2017 21:21:15 +0000 (UTC) X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Petr Mladek , x86@kernel.org, Jessica Yu , Vojtech Pavlik , linux-s390@vger.kernel.org, Peter Zijlstra , Jiri Kosina , Heiko Carstens , linux-kernel@vger.kernel.org, Kamalesh Babulal , Andy Lutomirski , live-patching@vger.kernel.org, Jiri Slaby , linuxppc-dev@lists.ozlabs.org, Ingo Molnar , Chris J Arges Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Fri, Feb 17, 2017 at 09:51:29AM +0100, Miroslav Benes wrote: > On Thu, 16 Feb 2017, Josh Poimboeuf wrote: > > What do you think about the following? I tried to put the logic in > > klp_complete_transition(), so the module_put()'s would be in one place. > > But it was too messy, so I put it in klp_cancel_transition() instead. > > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > > index e96346e..bd1c1fd 100644 > > --- a/kernel/livepatch/transition.c > > +++ b/kernel/livepatch/transition.c > > @@ -121,8 +121,28 @@ static void klp_complete_transition(void) > > */ > > void klp_cancel_transition(void) > > { > > + bool immediate_func = false; > > + > > klp_target_state = !klp_target_state; > > klp_complete_transition(); > > + > > + if (klp_target_state == KLP_PATCHED) > > + return; > > This is not needed, I think. We call klp_cancel_transition() in > __klp_enable_patch() only. klp_target_state is set to KLP_PATCHED there > (through klp_init_transition()) and negated here. We know it must be > KLP_UNPATCHED. Yeah, I was trying to hedge against the possibility of future code calling this function in the disable path. But that probably won't happen and it would probably be cleaner to just add a warning if klp_target_state isn't KLP_PATCHED. > Moreover, due to klp_complete_transition() klp_target_state is always > KLP_UNDEFINED after it. > > > + > > + /* > > + * In the enable error path, even immediate patches can be safely > > + * removed because the transition hasn't been started yet. > > + * > > + * klp_complete_transition() doesn't have a module_put() for immediate > > + * patches, so do it here. > > + */ > > + klp_for_each_object(klp_transition_patch, obj) > > + klp_for_each_func(obj, func) > > + if (func->immediate) > > + immediate_func = true; > > + > > + if (klp_transition_patch->immediate || immediate_func) > > + module_put(klp_transition_patch->mod); > > Almost correct. The only problem is that klp_transition_patch is NULL at > this point. klp_complete_transition() does that and it should stay there > in my opinion to keep it simple. > > So we could either move all this to __klp_enable_patch(), where patch > variable is defined, or we could store klp_transition_patch to a local > variable here in klp_cancel_transition() before klp_complete_transition() > is called. That should be safe. I like the latter more, because it keeps > the code in klp_cancel_transition() where it belongs. Good points. Here's another try: diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index e96346e..a23c63c 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -121,8 +121,31 @@ static void klp_complete_transition(void) */ void klp_cancel_transition(void) { - klp_target_state = !klp_target_state; + struct klp_patch *patch = klp_transition_patch; + struct klp_object *obj; + struct klp_func *func; + bool immediate_func = false; + + if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED)) + return; + + klp_target_state = KLP_UNPATCHED; klp_complete_transition(); + + /* + * In the enable error path, even immediate patches can be safely + * removed because the transition hasn't been started yet. + * + * klp_complete_transition() doesn't have a module_put() for immediate + * patches, so do it here. + */ + klp_for_each_object(patch, obj) + klp_for_each_func(obj, func) + if (func->immediate) + immediate_func = true; + + if (patch->immediate || immediate_func) + module_put(patch->mod); } /*