Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757929AbaLJPaT (ORCPT ); Wed, 10 Dec 2014 10:30:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39381 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757642AbaLJPaR (ORCPT ); Wed, 10 Dec 2014 10:30:17 -0500 Date: Wed, 10 Dec 2014 09:25:06 -0600 From: Josh Poimboeuf To: Petr Mladek Cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Steven Rostedt , Miroslav Benes , Masami Hiramatsu , Christoph Hellwig , Greg KH , Andy Lutomirski , live-patching@vger.kernel.org, x86@kernel.org, kpatch@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/6] livepatch v5: avoid race when checking for state of the patch Message-ID: <20141210152506.GA32670@treble.redhat.com> References: <1418148307-21434-1-git-send-email-pmladek@suse.cz> <1418148307-21434-2-git-send-email-pmladek@suse.cz> <20141209183249.GA25239@treble.redhat.com> <20141210101147.GB2454@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20141210101147.GB2454@pathway.suse.cz> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 10, 2014 at 11:11:47AM +0100, Petr Mladek wrote: > On Tue 2014-12-09 12:32:49, Josh Poimboeuf wrote: > > On Tue, Dec 09, 2014 at 07:05:02PM +0100, Petr Mladek wrote: > > > klp_patch_enable() and klp_patch_disable() should check the current state > > > of the patch under the klp_lock. Otherwise, it might detect that the operation > > > is valid but the situation might change before it takes the lock. > > > > Hi Petr, > > > > Thanks for the patches. > > > > I don't think this patch is necessary. klp_is_enabled() doesn't check > > the state of the patch. It checks the initialization state of the core > > module (klp_root_kobj), which can only be set in klp_init(). It's not > > protected by the lock, so I don't see the point of this patch. > > Ah, I have misread the name and expected that it checked whether > the patch was enabled or disabled. The original code is OK then. > > Well, Jiri Kosina pointed out that the check did not make much sense. > klp_is_enabled() could not be called if the livepatch module is not > loaded. And the later check for klp_patch_is_registered() is enough > to check whether the klp_enable_patch()/klp_disable_patch() calls > are allowed or not. But livepatch isn't a module, it's part of the kernel. Even if the init function returns an error, that doesn't prevent any of the other exported functions from getting called. > > So, I suggest to remove the checks at all. > > Best Regards, > Petr > > > > > > > > Signed-off-by: Petr Mladek > > > --- > > > kernel/livepatch/core.c | 16 ++++++++++------ > > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > > index d6d0f50e81f8..b848069e44cc 100644 > > > --- a/kernel/livepatch/core.c > > > +++ b/kernel/livepatch/core.c > > > @@ -425,11 +425,13 @@ int klp_disable_patch(struct klp_patch *patch) > > > { > > > int ret; > > > > > > - if (!klp_is_enabled()) > > > - return -ENODEV; > > > - > > > mutex_lock(&klp_mutex); > > > > > > + if (!klp_is_enabled()) { > > > + ret = -ENODEV; > > > + goto err; > > > + } > > > + > > > if (!klp_patch_is_registered(patch)) { > > > ret = -EINVAL; > > > goto err; > > > @@ -489,11 +491,13 @@ int klp_enable_patch(struct klp_patch *patch) > > > { > > > int ret; > > > > > > - if (!klp_is_enabled()) > > > - return -ENODEV; > > > - > > > mutex_lock(&klp_mutex); > > > > > > + if (!klp_is_enabled()) { > > > + ret = -ENODEV; > > > + goto err; > > > + } > > > + > > > if (!klp_patch_is_registered(patch)) { > > > ret = -EINVAL; > > > goto err; > > > -- > > > 1.8.5.2 > > > > > > > -- > > Josh -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/