Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e8d4d73ce58e3ecac0de35b0641501673dadfa18.camel@kernel.org>
Subject: Re: [PATCH 0/2] nfsd: close potential race between open and
 delegation
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Date:   Mon, 18 Jul 2022 07:16:24 -0400
In-Reply-To: <165811450205.25184.16800980627192339653@noble.neil.brown.name>
References: <20220714200434.161818-1-jlayton@kernel.org>
        , <165784314214.25184.13511971308364755291@noble.neil.brown.name>
        , <c10b61cd59a940dd93f6977300ab0d3c6742320f.camel@kernel.org>
         <165811450205.25184.16800980627192339653@noble.neil.brown.name>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.3 (3.44.3-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-07-18 at 13:21 +1000, NeilBrown wrote:
> On Fri, 15 Jul 2022, Jeff Layton wrote:
> > On Fri, 2022-07-15 at 09:59 +1000, NeilBrown wrote:
> >=20
> > > however...
> > > 2/ Do we really need to lock the parent?  If a rename or unlink happe=
ns
> > >    after the lease was taken, the lease will be broken. So
> > >     take lease.
> > >     repeat lookup (locklessly)
> > >     Check if lease has been broken
> > >    Should provide all you need.
> > >=20
> > >    You don't *need* to lock the directory to open an existing file an=
d
> > >    with my pending parallel-updates patch set, you only need a shared
> > >    lock on the directory to create a file.  So I'd rather not be lock=
ing
> > >    the directory at all to get a delegation
> > >=20
> >=20
> > Yeah, we probably don't need to lock the dir. That said, after your
> > patch series we already hold the i_rwsem on the parent at this point so
> > lookup_one_len is fine in this instance.
>=20
> But the only reason we hold i_rwsem at this point is to prevent renames
> in the "opened existing file" case.  The "created new file" case holds
> it as well just be be consistent with the first case.
>=20
> If we "vet" the dentry, then we don't need the lock any more.  We can
> then simplify nfsd_lookup_dentry() to always assume the dir is not
> locked - so the "locked" arg can go, and nfsd_lookup() can lose the
> "lock" arg and always return with the directory unlocked.
>=20
> I'm tempted to add your patch to the front of my series.  The
> inconsistency in locking can be fix by unlocking the directory before we
> get even close to handing out a delegation - so the delegation never
> sees a locked directory.

Hmm, ok. I suppose we don't necessarily have to care whether the thing
is locked before calling into nfsd_lookup_dentry. I'll take another stab
at fixing this in the kernel w/o your series. That'll make Chuck happy
too.

> But right now I have a cold and don't trust myself to think clearly
> enough to create code worth posting.  Hopefully I'll be thinking more
> clearly later in the week.
>=20
> While I'm here ...  is "vet" a good word?  The meaning is appropriate,
> but I wonder if it would cause our friends for whom English isn't their
> first language to stumble.  There are about 5 uses in the kernel at
> present.
>=20
> Would validate or verify be better?  Even though they are longer..


Good point. I'm all for helping out non-native English speakers. I'll
plan to change it to something less esoteric.
--=20
Jeff Layton <jlayton@kernel.org>