From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1D563C433E0 for ; Sat, 16 Jan 2021 05:13:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DC81E23A50 for ; Sat, 16 Jan 2021 05:13:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725833AbhAPFNl (ORCPT ); Sat, 16 Jan 2021 00:13:41 -0500 Received: from mail.kernel.org ([198.145.29.99]:59192 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725797AbhAPFNl (ORCPT ); Sat, 16 Jan 2021 00:13:41 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id 3939C23A34; Sat, 16 Jan 2021 05:12:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1610773980; bh=XjHYfZhoEozBWkWnRtYvEXm1JhtV+IohCAnqoPyxpp0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Nedo86rhUMEYWWZTGQLmJe+9Hi+obI37NvHWROzDwLsdH64Paj54PUbFZaKLsxk4N h2U/cL6LvMOeMYLrrJUhaWGqoNxmP3NLyOz35MgdKKVM3sfTfqEGshLqlZQ1Q2hIoL hyoDx2+rGRpj5NAxKDHhUikbakYx2Leu7e0YdHtG5Gt5pucDeiLVf93unP6IdjXy4n dkBmTVGip6cYCokNzAwsCws+BsRLxvnilziyXVewHK4igUeUoEAMOjQ+bil+Gooh/Z tRZFz2o2lQ8KAm3Qidxx7grGqHWubB0ML+G6q+gILQX2a1x4wGa0bGd6rUagDhkcwx V2ceAUNEZ4DLQ== Date: Sat, 16 Jan 2021 07:12:54 +0200 From: Jarkko Sakkinen To: Borislav Petkov Cc: linux-sgx@vger.kernel.org, dave.hansen@intel.com, kai.huang@intel.com, haitao.huang@intel.com, seanjc@google.com, stable@vger.kernel.org, Haitao Huang , Thomas Gleixner , Ingo Molnar , x86@kernel.org, "H. Peter Anvin" , Jethro Beekman Subject: Re: [PATCH v4] x86/sgx: Fix the call order of synchronize_srcu() in sgx_release() Message-ID: References: <20210115014638.15037-1-jarkko@kernel.org> <20210115071809.GA9138@zn.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210115071809.GA9138@zn.tnic> Precedence: bulk List-ID: X-Mailing-List: linux-sgx@vger.kernel.org On Fri, Jan 15, 2021 at 08:18:09AM +0100, Borislav Petkov wrote: > On Fri, Jan 15, 2021 at 03:46:38AM +0200, jarkko@kernel.org wrote: > > From: Jarkko Sakkinen > > > > The most trivial example of a race condition can be demonstrated with this > > example where mm_list contains just one entry: > > > > CPU A CPU B > > sgx_release() > > sgx_mmu_notifier_release() > > list_del_rcu() > > sgx_encl_release() > > synchronize_srcu() > > cleanup_srcu_struct() > > > > To fix this, call synchronize_srcu() before checking whether mm_list is > > empty in sgx_release(). > > To fix what? > > Lemme explain one more time: a commit message for a race condition > fix needs to explain in *detail* what the race condition is. And that > explanation needs to be understandable months and years from now. > > You have the function call order above, now you have to explain what can > happen. Just how you did here: > > https://lkml.kernel.org/r/X/zoarV7gd/LNo4A@kernel.org OK, I could recall the race that from but that must be partly because I've been proactively working on it, i.e. getting your point. So let's say I add this after the sequence: "The sequence demonstrates a scenario where CPU B starts a new grace period, which goes unnoticed by CPU A in sgx_release(), because it did not remove the final entry from the enclave's mm list." Would this be sufficient or not? > > Cc: stable@vger.kernel.org > > Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer") > > Suggested-by: Sean Christopherson > > Suggested-by: Haitao Huang > > Signed-off-by: Jarkko Sakkinen > > --- > > v4: > > - Rewrite the commit message. > > - Just change the call order. *_expedited() is out of scope for this > > bug fix. > > v3: Fine-tuned tags, and added missing change log for v2. > > v2: Switch to synchronize_srcu_expedited(). > > arch/x86/kernel/cpu/sgx/driver.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c > > index f2eac41bb4ff..53056345f5f8 100644 > > --- a/arch/x86/kernel/cpu/sgx/driver.c > > +++ b/arch/x86/kernel/cpu/sgx/driver.c > > @@ -65,11 +65,16 @@ static int sgx_release(struct inode *inode, struct file *file) > > > > spin_unlock(&encl->mm_lock); > > > > + /* > > + * The call is need even if the list empty, because sgx_encl_mmu_notifier_release() > > "The call is need"? > > > $ git grep sgx_encl_mmu_notifier_release > $ > > WTF? > > Please be more careful. Note taken. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko