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;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Jeff Layton" <jlayton@kernel.org>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 0/2] nfsd: close potential race between open and delegation
In-reply-to: <c10b61cd59a940dd93f6977300ab0d3c6742320f.camel@kernel.org>
References: <20220714200434.161818-1-jlayton@kernel.org>,
 <165784314214.25184.13511971308364755291@noble.neil.brown.name>,
 <c10b61cd59a940dd93f6977300ab0d3c6742320f.camel@kernel.org>
Date:   Mon, 18 Jul 2022 13:21:42 +1000
Message-id: <165811450205.25184.16800980627192339653@noble.neil.brown.name>
Precedence: bulk

On Fri, 15 Jul 2022, Jeff Layton wrote:
> On Fri, 2022-07-15 at 09:59 +1000, NeilBrown wrote:
> 
> > however...
> > 2/ Do we really need to lock the parent?  If a rename or unlink happens
> >    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.
> > 
> >    You don't *need* to lock the directory to open an existing file and
> >    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 locking
> >    the directory at all to get a delegation
> > 
> 
> 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.

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.

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.

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.
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.

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.

Would validate or verify be better?  Even though they are longer..

Thanks,
NeilBrown