Re: [PATCH] NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID

["J. Bruce Fields" <bfields@fieldses.org>]

Thanks, applying for 3.6.

--b.

On Tue, May 29, 2012 at 01:56:37PM -0400, Chuck Lever wrote:
> According to RFC 5661, the TEST_STATEID operation is not allowed to
> return NFS4ERR_STALE_STATEID.  In addition, RFC 5661 says:
> 
> 15.1.16.5.  NFS4ERR_STALE_STATEID (Error Code 10023)
> 
>    A stateid generated by an earlier server instance was used.  This
>    error is moot in NFSv4.1 because all operations that take a stateid
>    MUST be preceded by the SEQUENCE operation, and the earlier server
>    instance is detected by the session infrastructure that supports
>    SEQUENCE.
> 
> I triggered NFS4ERR_STALE_STATEID while testing the Linux client's
> NOGRACE recovery.  Bruce suggested an additional test that could be
> useful to client developers.
> 
> Lastly, RFC 5661, section 18.48.3 has this:
> 
>  o  Special stateids are always considered invalid (they result in the
>     error code NFS4ERR_BAD_STATEID).
> 
> An explicit check is made for those state IDs to avoid printk noise.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfsd/nfs4state.c |   22 ++++++++++++++++------
>  fs/nfsd/state.h     |    1 -
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9235cfa..c6d44b4 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -38,6 +38,7 @@
>  #include <linux/namei.h>
>  #include <linux/swap.h>
>  #include <linux/pagemap.h>
> +#include <linux/ratelimit.h>
>  #include <linux/sunrpc/svcauth_gss.h>
>  #include <linux/sunrpc/clnt.h>
>  #include "xdr4.h"
> @@ -3359,18 +3360,26 @@ static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref,
>  	return nfserr_old_stateid;
>  }
>  
> -__be32 nfs4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> +static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  {
>  	struct nfs4_stid *s;
>  	struct nfs4_ol_stateid *ols;
>  	__be32 status;
>  
> -	if (STALE_STATEID(stateid))
> -		return nfserr_stale_stateid;
> -
> +	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
> +		return nfserr_bad_stateid;
> +	/* Client debugging aid. */
> +	if (!same_clid(&stateid->si_opaque.so_clid, &cl->cl_clientid)) {
> +		char addr_str[INET6_ADDRSTRLEN];
> +		rpc_ntop((struct sockaddr *)&cl->cl_addr, addr_str,
> +				 sizeof(addr_str));
> +		pr_warn_ratelimited("NFSD: client %s testing state ID "
> +					"with incorrect client ID\n", addr_str);
> +		return nfserr_bad_stateid;
> +	}
>  	s = find_stateid(cl, stateid);
>  	if (!s)
> -		 return nfserr_stale_stateid;
> +		return nfserr_bad_stateid;
>  	status = check_stateid_generation(stateid, &s->sc_stateid, 1);
>  	if (status)
>  		return status;
> @@ -3489,7 +3498,8 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	nfs4_lock_state();
>  	list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list)
> -		stateid->ts_id_status = nfs4_validate_stateid(cl, &stateid->ts_id_stateid);
> +		stateid->ts_id_status =
> +			nfsd4_validate_stateid(cl, &stateid->ts_id_stateid);
>  	nfs4_unlock_state();
>  
>  	return nfs_ok;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 89ab137..b6bd031 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -476,7 +476,6 @@ extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
>  extern int nfs4_client_to_reclaim(const char *name);
>  extern int nfs4_has_reclaimed_state(const char *name, bool use_exchange_id);
>  extern void release_session_client(struct nfsd4_session *);
> -extern __be32 nfs4_validate_stateid(struct nfs4_client *, stateid_t *);
>  extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
>  
>  /* nfs4recover operations */
>