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: <c10b61cd59a940dd93f6977300ab0d3c6742320f.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:   Fri, 15 Jul 2022 07:32:41 -0400
In-Reply-To: <165784314214.25184.13511971308364755291@noble.neil.brown.name>
References: <20220714200434.161818-1-jlayton@kernel.org>
         <165784314214.25184.13511971308364755291@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 Fri, 2022-07-15 at 09:59 +1000, NeilBrown wrote:
> 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.
> >=20
> > This series is based on top of Neil's set entitled:
> >=20
> >     [PATCH 0/8] NFSD: clean up locking.
> >=20
> > 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.
>=20
> I haven't examined these patch very closely, but a few initial thoughts
> are:
>=20
> 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

Thanks, good to know. I wasn't sure how reliable that bool is. I guess
though that once you have a svc_fh, then you can more or less assume
that you have exclusive access to it for the life of the RPC being
processed.

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

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

Agreed, but that will take a bit more work. nfsd_lookup_dentry takes
several parameters that we don't currently have access to in
nfs4_set_delegation (e.g. the rqstp). Those will need to be plumbed
through several functions.

> Thanks for working on this!

...and thank you for the locking cleanup! Getting rid of fh_lock/_unlock
is a really nice cleanup that makes it a lot more clear how this should
work.
--=20
Jeff Layton <jlayton@kernel.org>