[PATCH] nfsd: Detect unhashed stids in nfsd4_verify_open_stid()

[PATCH] nfsd: Detect unhashed stids in nfsd4_verify_open_stid()

[Trond Myklebust]

The state of the stid is guaranteed by 2 locks:
- The nfs4_client 'cl_lock' spinlock
- The nfs4_ol_stateid 'st_mutex' mutex

so it is quite possible for the stid to be unhashed after lookup,
but before calling nfsd4_lock_ol_stateid(). So we do need to check
for a zero value for 'sc_type' in nfsd4_verify_open_stid().

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8d53d29161f5..b5bda115f6a9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3591,6 +3591,7 @@ nfsd4_verify_open_stid(struct nfs4_stid *s)
 	switch (s->sc_type) {
 	default:
 		break;
+	case 0:
 	case NFS4_CLOSED_STID:
 	case NFS4_CLOSED_DELEG_STID:
 		ret = nfserr_bad_stateid;
-- 
2.14.3

Re: [PATCH] nfsd: Detect unhashed stids in nfsd4_verify_open_stid()

[Chuck Lever]

> On Jan 12, 2018, at 5:42 PM, Trond Myklebust =
<trond.myklebust@primarydata.com> wrote:
>=20
> The state of the stid is guaranteed by 2 locks:
> - The nfs4_client 'cl_lock' spinlock
> - The nfs4_ol_stateid 'st_mutex' mutex
>=20
> so it is quite possible for the stid to be unhashed after lookup,
> but before calling nfsd4_lock_ol_stateid(). So we do need to check
> for a zero value for 'sc_type' in nfsd4_verify_open_stid().
>=20
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>

Three successful passes of the git regression suite on NFSv4.1
Three successful passes of xfstests on NFSv4.1

Tested-by: Chuck Lever <chuck.lever@oracle.com>


> ---
> fs/nfsd/nfs4state.c | 1 +
> 1 file changed, 1 insertion(+)
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8d53d29161f5..b5bda115f6a9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3591,6 +3591,7 @@ nfsd4_verify_open_stid(struct nfs4_stid *s)
> 	switch (s->sc_type) {
> 	default:
> 		break;
> +	case 0:
> 	case NFS4_CLOSED_STID:
> 	case NFS4_CLOSED_DELEG_STID:
> 		ret =3D nfserr_bad_stateid;
> --=20
> 2.14.3
>=20

--
Chuck Lever

Re: [PATCH] nfsd: Detect unhashed stids in nfsd4_verify_open_stid()

[J. Bruce Fields]

On Fri, Jan 12, 2018 at 09:15:46PM -0500, Chuck Lever wrote:
> 
> 
> > On Jan 12, 2018, at 5:42 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> > 
> > The state of the stid is guaranteed by 2 locks:
> > - The nfs4_client 'cl_lock' spinlock
> > - The nfs4_ol_stateid 'st_mutex' mutex
> > 
> > so it is quite possible for the stid to be unhashed after lookup,
> > but before calling nfsd4_lock_ol_stateid(). So we do need to check
> > for a zero value for 'sc_type' in nfsd4_verify_open_stid().
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> Three successful passes of the git regression suite on NFSv4.1
> Three successful passes of xfstests on NFSv4.1
> 
> Tested-by: Chuck Lever <chuck.lever@oracle.com>

Thanks!  Applying.

Then I think this makes a couple of "sc_type = NFS4_CLOSED_STID"'s
superfluous.

--b.

commit 17693d95b3d3
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Jan 17 16:25:59 2018 -0500

    nfsd4: don't set lock stateid's sc_type to CLOSED
    
    There's no point I can see to
    
            stp->st_stid.sc_type = NFS4_CLOSED_STID;
    
    given release_lock_stateid immediately sets sc_type to 0.
    
    That set of sc_type to 0 should be enough to prevent it being used where
    we don't want it to be; NFS4_CLOSED_STID should only be needed for
    actual open stateid's that are actually closed.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5a75135f5f53..150521c9671b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5183,7 +5183,6 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
 			    lockowner(stp->st_stateowner)))
 		goto out;
 
-	stp->st_stid.sc_type = NFS4_CLOSED_STID;
 	release_lock_stateid(stp);
 	ret = nfs_ok;
 
@@ -6079,10 +6078,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		 * If this is a new, never-before-used stateid, and we are
 		 * returning an error, then just go ahead and release it.
 		 */
-		if (status && new) {
-			lock_stp->st_stid.sc_type = NFS4_CLOSED_STID;
+		if (status && new)
 			release_lock_stateid(lock_stp);
-		}
 
 		mutex_unlock(&lock_stp->st_mutex);
 

Re: [PATCH] nfsd: Detect unhashed stids in nfsd4_verify_open_stid()

[J. Bruce Fields]

On Wed, Jan 17, 2018 at 04:30:57PM -0500, bfields wrote:
> On Fri, Jan 12, 2018 at 09:15:46PM -0500, Chuck Lever wrote:
> > 
> > 
> > > On Jan 12, 2018, at 5:42 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> > > 
> > > The state of the stid is guaranteed by 2 locks:
> > > - The nfs4_client 'cl_lock' spinlock
> > > - The nfs4_ol_stateid 'st_mutex' mutex
> > > 
> > > so it is quite possible for the stid to be unhashed after lookup,
> > > but before calling nfsd4_lock_ol_stateid(). So we do need to check
> > > for a zero value for 'sc_type' in nfsd4_verify_open_stid().
> > > 
> > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > Three successful passes of the git regression suite on NFSv4.1
> > Three successful passes of xfstests on NFSv4.1
> > 
> > Tested-by: Chuck Lever <chuck.lever@oracle.com>
> 
> Thanks!  Applying.

Though the changelog doesn't make sense to me.  What does it mean for
the "state of the stid" to be "gauranteed by 2 locks"?

The following paragraph suggests that once we acquire st_mutex, sc_type
can no longer change, but that doesn't look right--e.g.
nfsd4_release_lockowner calls unhash-lock_stateid->nfs4_unhash_stid with
only the cl_lock held.

--b.

> 
> Then I think this makes a couple of "sc_type = NFS4_CLOSED_STID"'s
> superfluous.
> 
> --b.
> 
> commit 17693d95b3d3
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Jan 17 16:25:59 2018 -0500
> 
>     nfsd4: don't set lock stateid's sc_type to CLOSED
>     
>     There's no point I can see to
>     
>             stp->st_stid.sc_type = NFS4_CLOSED_STID;
>     
>     given release_lock_stateid immediately sets sc_type to 0.
>     
>     That set of sc_type to 0 should be enough to prevent it being used where
>     we don't want it to be; NFS4_CLOSED_STID should only be needed for
>     actual open stateid's that are actually closed.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 5a75135f5f53..150521c9671b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5183,7 +5183,6 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
>  			    lockowner(stp->st_stateowner)))
>  		goto out;
>  
> -	stp->st_stid.sc_type = NFS4_CLOSED_STID;
>  	release_lock_stateid(stp);
>  	ret = nfs_ok;
>  
> @@ -6079,10 +6078,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		 * If this is a new, never-before-used stateid, and we are
>  		 * returning an error, then just go ahead and release it.
>  		 */
> -		if (status && new) {
> -			lock_stp->st_stid.sc_type = NFS4_CLOSED_STID;
> +		if (status && new)
>  			release_lock_stateid(lock_stp);
> -		}
>  
>  		mutex_unlock(&lock_stp->st_mutex);
>