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: <2d6fb0c2f74a75308243d10cc0531a8ccfdfc703.camel@kernel.org>
Subject: Re: [RFC PATCH 3/3] nfsd: vet the opened dentry after setting a
 delegation
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Neil Brown <neilb@suse.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Thu, 14 Jul 2022 14:49:59 -0400
In-Reply-To: <87EC2FB8-EA8A-4322-8725-A6494B606317@oracle.com>
References: <20220714152819.128276-1-jlayton@kernel.org>
         <20220714152819.128276-4-jlayton@kernel.org>
         <F7B94422-F889-49DA-AA38-0D8AA1F9B682@oracle.com>
         <fc9e2f339e9a84912e9e4644292bec44b5e49137.camel@kernel.org>
         <87EC2FB8-EA8A-4322-8725-A6494B606317@oracle.com>
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 Thu, 2022-07-14 at 17:16 +0000, Chuck Lever III wrote:
>=20
> > On Jul 14, 2022, at 1:11 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >=20
> > On Thu, 2022-07-14 at 16:53 +0000, Chuck Lever III wrote:
> > >=20
> > > > On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@kernel.org> wrot=
e:
> > > >=20
> > > > Between opening a file and setting a delegation on it, someone coul=
d
> > > > rename or unlink the dentry. If this happens, we do not want to gra=
nt a
> > > > delegation on the open.
> > > >=20
> > > > Take the i_rwsem before setting the delegation. If we're granted a
> > > > lease, redo the lookup of the file being opened and validate that t=
he
> > > > resulting dentry matches the one in the open file description. We o=
nly
> > > > need to do this for the CLAIM_NULL open case however.
> > > >=20
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > > fs/nfsd/nfs4state.c | 55 ++++++++++++++++++++++++++++++++++++++++--=
---
> > > > 1 file changed, 50 insertions(+), 5 deletions(-)
> > > >=20
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index 347794028c98..e5c7f6690d2d 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -1172,6 +1172,7 @@ alloc_init_deleg(struct nfs4_client *clp, str=
uct nfs4_file *fp,
> > > >=20
> > > > void
> > > > nfs4_put_stid(struct nfs4_stid *s)
> > > > +	__releases(&clp->cl_lock)
> > > > {
> > > > 	struct nfs4_file *fp =3D s->sc_file;
> > > > 	struct nfs4_client *clp =3D s->sc_client;
> > >=20
> > > This hunk causes a bunch of new sparse warnings:
> > >=20
> > > /home/cel/src/linux/klimt/include/linux/list.h:137:19: warning: conte=
xt imbalance in 'put_clnt_odstate' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1174:1: warning: contex=
t imbalance in 'nfs4_put_stid' - different lock contexts for basic block
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1230:13: warning: conte=
xt imbalance in 'destroy_unhashed_deleg' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1528:17: warning: conte=
xt imbalance in 'release_lock_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1624:17: warning: conte=
xt imbalance in 'release_last_closed_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:2212:22: warning: conte=
xt imbalance in '__destroy_client' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4481:17: warning: conte=
xt imbalance in 'nfsd4_find_and_lock_existing_open' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4557:25: warning: conte=
xt imbalance in 'init_open_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4606:17: warning: conte=
xt imbalance in 'move_to_close_lru' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:4752:13: warning: conte=
xt imbalance in 'nfsd4_cb_recall_release' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5003:17: warning: conte=
xt imbalance in 'nfs4_check_deleg' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5392:9: warning: contex=
t imbalance in 'nfs4_set_delegation' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5467:9: warning: contex=
t imbalance in 'nfs4_open_delegation' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5619:17: warning: conte=
xt imbalance in 'nfsd4_process_open2' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5638:17: warning: conte=
xt imbalance in 'nfsd4_cleanup_open_state' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:5934:27: warning: conte=
xt imbalance in 'nfs4_laundromat' - different lock contexts for basic block
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6160:17: warning: conte=
xt imbalance in 'nfsd4_lookup_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6374:25: warning: conte=
xt imbalance in 'nfs4_preprocess_stateid_op' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6422:9: warning: contex=
t imbalance in 'nfsd4_free_lock_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6459:28: warning: conte=
xt imbalance in 'nfsd4_free_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6528:17: warning: conte=
xt imbalance in 'nfs4_preprocess_seqid_op' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6545:17: warning: conte=
xt imbalance in 'nfs4_preprocess_confirmed_seqid_op' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6588:9: warning: contex=
t imbalance in 'nfsd4_open_confirm' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6657:9: warning: contex=
t imbalance in 'nfsd4_open_downgrade' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6730:9: warning: contex=
t imbalance in 'nfsd4_close' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:6762:9: warning: contex=
t imbalance in 'nfsd4_delegreturn' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7034:17: warning: conte=
xt imbalance in 'init_lock_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7063:17: warning: conte=
xt imbalance in 'find_or_create_lock_stateid' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7362:17: warning: conte=
xt imbalance in 'nfsd4_lock' - unexpected unlock
> > > /home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:7535:9: warning: contex=
t imbalance in 'nfsd4_locku' - unexpected unlock
> > >=20
> > > Let's repair the "/home/cel/src/linux/klimt/fs/nfsd/nfs4state.c:1185:=
9:
> > > warning: context imbalance in 'nfs4_put_stid' - unexpected unlock" me=
ssage
> > > in a separate patch.
> > >=20
> >=20
> >=20
> > Yeah, I saw that too after I sent this. That hunk doesn't belong in
> > here. I'll drop it from my local copy.
>=20
> Well, I'm interested in trying to get rid of the existing sparse
> warnings too, since those tend to block our ability to see new
> warnings that arise.
>=20
> If you have suggestions or proposals, please send them!
>=20

We should definitely annotate these functions that have unbalanced
locking like this one. That's the usual way to silence these sorts of
warnings. I'm hoping Neil's patches will make it easier to address.

--=20
Jeff Layton <jlayton@kernel.org>