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: <20220714200434.161818-1-jlayton@kernel.org>
References: <20220714200434.161818-1-jlayton@kernel.org>
Date:   Fri, 15 Jul 2022 09:59:02 +1000
Message-id: <165784314214.25184.13511971308364755291@noble.neil.brown.name>
Precedence: bulk

On Fri, 15 Jul 2022, Jeff Layton wrote:
> This is a respin of the patchset that I sent earlier today. I hit a
> deadlock with that one because of the ambiguous locking.
> 
> This series is based on top of Neil's set entitled:
> 
>     [PATCH 0/8] NFSD: clean up locking.
> 
> His patchset makes the locking in the nfsd4_open codepath much more
> consistent, and this becomes a lot simpler to fix. Without that set
> however, the state of the parent's i_rwsem is unclear after nfsd_lookup
> is called, and I don't see a way to determine it reliably.

I haven't examined these patch very closely, but a few initial thoughts
are:

1/ Before my series, you can unambiguously tell if i_rwsem is held by
   checking fhp->fh_locked.  In fact, just call "fh_lock()", and you can
   then be sure the fh is locked, whether or not it was locked before
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

3/ When you vet the name you only do a lookup_one_len(), while
   nfsd_lookup_dentry() also calls nfsd_cross_mnt() as it is possible
   for a file to be mounted on.
   That means that if I did bind mount one file over another and export
   over NFSD, the file will never offer a delegation.
   This is a minor point, but I think it would be best to be as correct
   and consistent as possible.

Thanks for working on this!

NeilBrown

> 
> Jeff Layton (2):
>   nfsd: drop fh argument from alloc_init_deleg
>   nfsd: vet the opened dentry after setting a delegation
> 
>  fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 45 insertions(+), 9 deletions(-)
> 
> -- 
> 2.36.1
> 
>