Received: by 2002:a05:6a11:4021:0:0:0:0 with SMTP id ky33csp190924pxb; Wed, 22 Sep 2021 19:52:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy9oA0XQaMHRan1nagseMgDm7TfuhIXehf1WaCoj8+SSKANS8VCRO+FFa+LJX91XLma4Ekf X-Received: by 2002:a92:c26c:: with SMTP id h12mr1834296ild.241.1632365548611; Wed, 22 Sep 2021 19:52:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1632365548; cv=none; d=google.com; s=arc-20160816; b=OFsAkJlZR/WSU9U8AdXEHJ938hicfamx/y9W9vS65oVW2Uq0/C/1EuOPpPyp0Dfq3v L90AV/TI6R1QBkGKUKpIUcPKq5UKfB9Q4HK+HaAEIjdaA7lzdnUcBljvdC/Akb+f0yxK ojebORrDfoS14lrWOTvpJe4iVu/jOr3bVpii/0fhNCGTzOzRwAy5TuFScAxMQ45YzU4l tWaOmpJn05xdg0d1dUMJdjYfcH1aTdP62naxVP53qQ4o24zeXTt0ybADkYi9us2Joyg2 ooV5AcHd4/wFYzcs0Exh2BuHhLPJn/I1GahjZl272hxPfNgvQ0+o929KJpGhiIRwUbWS OpmA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:date:cc:to:from:subject :message-id:dkim-signature:dkim-signature; bh=m1z6kaeKcFg9q/fTSiPLIInC9dpxETbY8DVKplWj3p4=; b=Dqs5RHqnVXy71DXc1Eaey0ITMFeyITACrW8KPdbnticXZWLxlZLtlZaBrJOj78VCUf HU+aIMiB4b5O97iSSJx9E1kkUJjx12P6Kp1E4ghXfLVY9XTP+NIMR/6exslYuvxaDNUT KNPX1HVBUJyY49uDmTrKhHwFCsGjYegGqpnfdWZYphBVsCPukAs7Bzwur8ZXXFeeeiD4 faccK0rQ4UCOqvplCXcLwmmaLsQXv+wfR5IOJaxItL/Zt+lvs4MYTRbrMUjCZSV/cNWX ZOgDOo5bGToFd+MiVjbH5HIjM1OXMh2oSPSMjH0nmoe0x9aPhAdh1QHLPTOeTZkl86SF GY9w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@themaw.net header.s=fm1 header.b="IZzug/0p"; dkim=pass header.i=@messagingengine.com header.s=fm3 header.b=HnU5f7h1; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id k26si4349891ioh.55.2021.09.22.19.52.12; Wed, 22 Sep 2021 19:52:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@themaw.net header.s=fm1 header.b="IZzug/0p"; dkim=pass header.i=@messagingengine.com header.s=fm3 header.b=HnU5f7h1; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238974AbhIWCwM (ORCPT + 99 others); Wed, 22 Sep 2021 22:52:12 -0400 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:36495 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238954AbhIWCwM (ORCPT ); Wed, 22 Sep 2021 22:52:12 -0400 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id E2EEE5C0110; Wed, 22 Sep 2021 22:50:40 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Wed, 22 Sep 2021 22:50:40 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=themaw.net; h= message-id:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version:content-transfer-encoding; s=fm1; bh= m1z6kaeKcFg9q/fTSiPLIInC9dpxETbY8DVKplWj3p4=; b=IZzug/0pmClEodTc rJF2kwNUnd5vbC0LQN2/z4IwHM82ocUf2Y897hpxi9+nO0fnztid5mUiUjLTsEHj 1X6G4WScKKBTcslXVwelC0mZN25oc8Hz6cvlMSxz+xSWvhhWjFJ7grL9q7Pd9UXl 6HkC7ix8rlIM3vcFyJ3N9Lfkwu+t/KFFivOeg99pwszVT1qn1eUUnCC8iA5H2GEe J0fBUCFB2TYjE2UhqR8n+rjZh6rDaDMTPlnkZgYAINSWe23upn0yUtQDBGgvFwwN WpdaO8dRPUPUusUzUufDLuT50yG7BFrLBpAtE4kgvvfXMaAmQv+SFKLB0a1HTNl4 5hnZtQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=m1z6kaeKcFg9q/fTSiPLIInC9dpxETbY8DVKplWj3 p4=; b=HnU5f7h1klNV9h0b9kzRvNqMwSDRF2RCUXMd4vw4igi2A1siaPEDlBeas yym2d5Dp1ACTZGMW8Xvqji3w239Y2k+Q6CZ6Z7kSBYVLgVaX0FjFJE4RrTEXb7dM rWFlrxjaAIU+juhwQRX4lKK0Ne+wCIdUB0UIrmbfDTC3vzApzrH1R6EMeSyqP8lE nFwW46zwOSpcQL4tEA+QfOiKikhXEk34VdRa9IcNv0D56AcXv35RZdQ+YEo99l+u Cxdsq01804zUBalBmtTJ+0uuTawVQHfDjLnpwQhWv6qp7fsLGuBABZ7dn4XFp7vS /KSs8dzw1s5QY6Ng8xka9ZwdmS+9w== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudeikedgiedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepkffuhffvffgjfhgtfggggfesthekredttderjeenucfhrhhomhepkfgrnhcu mfgvnhhtuceorhgrvhgvnhesthhhvghmrgifrdhnvghtqeenucggtffrrghtthgvrhhnpe fgleelkeetheelgeehueejueduhfeufffgleehgfevtdehhffhhffhtddugfefheenucev lhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehrrghvvghnse hthhgvmhgrfidrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 22 Sep 2021 22:50:37 -0400 (EDT) Message-ID: <077362887b4ceeb01c27fbf36fa35adae02967c9.camel@themaw.net> Subject: Re: [PATCH] kernfs: fix the race in the creation of negative dentry From: Ian Kent To: Hou Tao , Greg Kroah-Hartman , Tejun Heo , Miklos Szeredi Cc: viro@ZenIV.linux.org.uk, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Date: Thu, 23 Sep 2021 10:50:33 +0800 In-Reply-To: References: <20210911021342.3280687-1-houtao1@huawei.com> <7b92b158200567f0bba26a038191156890921f13.camel@themaw.net> <6c8088411523e52fc89b8dd07710c3825366ce64.camel@themaw.net> <747aee3255e7a07168557f29ad962e34e9cb964b.camel@themaw.net> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2021-09-23 at 09:52 +0800, Hou Tao wrote: > Hi, > > On 9/15/2021 10:09 AM, Ian Kent wrote: > > On Wed, 2021-09-15 at 09:35 +0800, Ian Kent wrote: > > > Sorry for the late reply. > > I think something like this is needed (not even compile tested): > > > > kernfs: dont create a negative dentry if node exists > > > > From: Ian Kent > > > > In kernfs_iop_lookup() a negative dentry is created if associated > > kernfs > > node is incative which makes it visible to lookups in the VFS path > > walk. > > > > But inactive kernfs nodes are meant to be invisible to the VFS and > > creating a negative for these can have unexpetced side effects. > > > > Signed-off-by: Ian Kent > > --- > >  fs/kernfs/dir.c |    9 ++++++++- > >  1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > index ba581429bf7b..a957c944cf3a 100644 > > --- a/fs/kernfs/dir.c > > +++ b/fs/kernfs/dir.c > > @@ -1111,7 +1111,14 @@ static struct dentry > > *kernfs_iop_lookup(struct inode *dir, > >   > >         kn = kernfs_find_ns(parent, dentry->d_name.name, ns); > >         /* attach dentry and inode */ > > -       if (kn && kernfs_active(kn)) { > > +       if (kn) { > > +               /* Inactive nodes are invisible to the VFS so don't > > +                * create a negative. > > +                */ > > +               if (!kernfs_active(kn)) { > > +                       up_read(&kernfs_rwsem); > > +                       return NULL; > > +               } > >                 inode = kernfs_get_inode(dir->i_sb, kn); > >                 if (!inode) > >                         inode = ERR_PTR(-ENOMEM); > > > > > > Essentially, the definition a kernfs negative dentry, for the > > cases it is meant to cover, is one that has no kernfs node, so > > one that does have a node should not be created as a negative. > > > > Once activated a subsequent ->lookup() will then create a > > positive dentry for the node so that no invalidation is > > necessary. > I'm fine with the fix which is much simpler. Great, although I was hoping you would check it worked as expected. Did you check? If not could you please do that check? > > This distinction is important because we absolutely do not want > > negative dentries created that aren't necessary. We don't want to > > leave any opportunities for negative dentries to accumulate if > > we don't have to. > >     > > I am still thinking about the race you have described. > > > > Given my above comments that race might have (maybe probably) > > been present in the original code before the rwsem change but > > didn't trigger because of the serial nature of the mutex. > I don't think there is such race before the enabling of negative > dentry, > but maybe I misunderstanding something. No, I think you're probably right, it's the introduction of using negative dentries to prevent the expensive dentry alloc/free cycle of frequent lookups of non-existent paths that's exposed the race. > > So it may be wise (perhaps necessary) to at least move the > > activation under the rwsem (as you have done) which covers most > > of the change your proposing and the remaining hunk shouldn't > > do any harm I think but again I need a little more time on that. > After above fix, doing sibling tree operation and activation > atomically > will reduce the unnecessary lookup, but I don't think it is necessary > for the fix of race. Sorry, I don't understand what your saying. Are you saying you did check my suggested patch alone and it resolved the problem. And that you also think the small additional dentry churn is ok too. If so I agree, and I'll forward the patch to Greg, ;) Ian > > Regards, > Tao > > I'm now a little concerned about the invalidation that should > > occur on deactivation so I want to have a look at that too but > > it's separate to this proposal. > > Greg, Tejun, Hou, any further thoughts on this would be most > > welcome. > > > > Ian > > > > > . >