[PATCH] NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID

[PATCH] NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID

[Chuck Lever]

The error values that TEST_STATEID is allowed to return does not
include 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 the NFS4ERR_STALE_STATEID during nograce recovery testing.
My client had updated its boot verifier, so the server instance hadn't
changed, but the client instance had.  Thus the server allowed the
SEQUENCE operation, but returned NFS4ERR_STALE_STATEID on the
TEST_STATEID operation.

After a client's lease expires, TEST_STATEID should report
NFS4ERR_EXPIRED for state IDs that the client tries to recover.  I
don't see a way to make that happen, though.

Finally, RFC 5661, section 18.48.3 has this:

 o  Special stateids are always considered invalid (they result in the
    error code NFS4ERR_BAD_STATEID).

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

Bruce, would you consider taking something like this?

 fs/nfsd/nfs4state.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 9235cfa..ae1fab3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3365,12 +3365,13 @@ __be32 nfs4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	struct nfs4_ol_stateid *ols;
 	__be32 status;
 
+	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
+		return nfserr_bad_stateid;
 	if (STALE_STATEID(stateid))
-		return nfserr_stale_stateid;
-
+		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;

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

[J. Bruce Fields]

On Thu, May 24, 2012 at 03:29:51PM -0400, Chuck Lever wrote:
> The error values that TEST_STATEID is allowed to return does not
> include 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 the NFS4ERR_STALE_STATEID during nograce recovery testing.
> My client had updated its boot verifier, so the server instance hadn't
> changed, but the client instance had.  Thus the server allowed the
> SEQUENCE operation, but returned NFS4ERR_STALE_STATEID on the
> TEST_STATEID operation.
> 
> After a client's lease expires, TEST_STATEID should report
> NFS4ERR_EXPIRED for state IDs that the client tries to recover.  I
> don't see a way to make that happen, though.

After the client's lease expires, the SEQUENCE operation will fail.

(Which I believe to be a valid, if unforgiving, server implementation.
If we were to implement "courtesy locks" in this case, I believe we'd
remember the clientid for longer, permit the SEQUENCE, and fail
individual stateid's with EXPIRED as appropriate?)

> Finally, RFC 5661, section 18.48.3 has this:
> 
>  o  Special stateids are always considered invalid (they result in the
>     error code NFS4ERR_BAD_STATEID).

Thanks for the explanation!

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> Bruce, would you consider taking something like this?

Sure; nits:

>  fs/nfsd/nfs4state.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9235cfa..ae1fab3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3365,12 +3365,13 @@ __be32 nfs4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  	struct nfs4_ol_stateid *ols;
>  	__be32 status;
>  
> +	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
> +		return nfserr_bad_stateid;

Or inval?  This is just a buggy client.

>  	if (STALE_STATEID(stateid))
> -		return nfserr_stale_stateid;
> -
> +		return nfserr_bad_stateid;

Again, this is just a buggy client, since we shouldn't have gotten past
the SEQUENCE in this case unless the client's sending a stateid that's
actually someone else's.

If you think it's worth checking for those buggy client cases, we could
instaed check that stateid->si_opaque.so_clid and cl->clientid agree.
That'd cover the special-stateid checks too.

>  	s = find_stateid(cl, stateid);
>  	if (!s)
> -		 return nfserr_stale_stateid;
> +		 return nfserr_bad_stateid;

So this must be the case you actually hit.  Agreed with this change.


--b.

>  	status = check_stateid_generation(stateid, &s->sc_stateid, 1);
>  	if (status)
>  		return status;
> 

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

[Chuck Lever]

On May 25, 2012, at 9:46 AM, J. Bruce Fields wrote:

> On Thu, May 24, 2012 at 03:29:51PM -0400, Chuck Lever wrote:
>> The error values that TEST_STATEID is allowed to return does not
>> include 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 the NFS4ERR_STALE_STATEID during nograce recovery testing.
>> My client had updated its boot verifier, so the server instance hadn't
>> changed, but the client instance had.  Thus the server allowed the
>> SEQUENCE operation, but returned NFS4ERR_STALE_STATEID on the
>> TEST_STATEID operation.
>> 
>> After a client's lease expires, TEST_STATEID should report
>> NFS4ERR_EXPIRED for state IDs that the client tries to recover.  I
>> don't see a way to make that happen, though.
> 
> After the client's lease expires, the SEQUENCE operation will fail.
> 
> (Which I believe to be a valid, if unforgiving, server implementation.
> If we were to implement "courtesy locks" in this case, I believe we'd
> remember the clientid for longer, permit the SEQUENCE, and fail
> individual stateid's with EXPIRED as appropriate?)
> 
>> Finally, RFC 5661, section 18.48.3 has this:
>> 
>> o  Special stateids are always considered invalid (they result in the
>>    error code NFS4ERR_BAD_STATEID).
> 
> Thanks for the explanation!
> 
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> Bruce, would you consider taking something like this?
> 
> Sure; nits:
> 
>> fs/nfsd/nfs4state.c |    7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 9235cfa..ae1fab3 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -3365,12 +3365,13 @@ __be32 nfs4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>> 	struct nfs4_ol_stateid *ols;
>> 	__be32 status;
>> 
>> +	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
>> +		return nfserr_bad_stateid;
> 
> Or inval?  This is just a buggy client.

RFC 5661 says specifically that TEST_STATEID should return NFS4ERR_BAD_STATEID in this case.

> 
>> 	if (STALE_STATEID(stateid))
>> -		return nfserr_stale_stateid;
>> -
>> +		return nfserr_bad_stateid;
> 
> Again, this is just a buggy client, since we shouldn't have gotten past
> the SEQUENCE in this case unless the client's sending a stateid that's
> actually someone else's.

In my nograce test, the server does get past the SEQUENCE.  So maybe there's a bug there too?

> If you think it's worth checking for those buggy client cases, we could
> instaed check that stateid->si_opaque.so_clid and cl->clientid agree.
> That'd cover the special-stateid checks too.

That's interesting.  I can redrive this patch with this change.

Yes, I think checking for buggy clients is a good idea.

> 
>> 	s = find_stateid(cl, stateid);
>> 	if (!s)
>> -		 return nfserr_stale_stateid;
>> +		 return nfserr_bad_stateid;
> 
> So this must be the case you actually hit.  Agreed with this change.
> 
> 
> --b.
> 
>> 	status = check_stateid_generation(stateid, &s->sc_stateid, 1);
>> 	if (status)
>> 		return status;
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

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

[Chuck Lever]

Hi-

On May 25, 2012, at 7:30 PM, Chuck Lever wrote:

> On May 25, 2012, at 9:46 AM, J. Bruce Fields wrote:
> 
>> If you think it's worth checking for those buggy client cases, we could
>> instaed check that stateid->si_opaque.so_clid and cl->clientid agree.
>> That'd cover the special-stateid checks too.
> 
> That's interesting.  I can redrive this patch with this change.

Doesn't a positive find_stateid(cl, stateid) result guarantee that so_clid and clientid are the same?

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

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

[J. Bruce Fields]

On Sun, May 27, 2012 at 04:30:24PM -0400, Chuck Lever wrote:
> Hi-
> 
> On May 25, 2012, at 7:30 PM, Chuck Lever wrote:
> 
> > On May 25, 2012, at 9:46 AM, J. Bruce Fields wrote:
> > 
> >> If you think it's worth checking for those buggy client cases, we could
> >> instaed check that stateid->si_opaque.so_clid and cl->clientid agree.
> >> That'd cover the special-stateid checks too.
> > 
> > That's interesting.  I can redrive this patch with this change.
> 
> Doesn't a positive find_stateid(cl, stateid) result guarantee that so_clid and clientid are the same?

No, it only looks at the si_opaque.so_id, and assumes this stateid
belongs to the given client.  The passed-in client was actually
extracted from the sequence at the start of the compound:

	nfsd4_sequence(...)
	{
		...
		session = find_in_sessionid_hashtbl(&seq->sessionid);
		...
		cstate->session = session;
	}

	nfsd4_free_stateid(...)
	{
		struct nfs4_client *cl = cstate->session->se_client;
		...
			... nfs4_validate_stateid(cl, &stateid->ts_id_stateid);
	}

So in theory a confused client sent a free_stateid referencing a stateid
that belonged to a client other than the one that the session belonged
to, and then we'd see this inconsistency.

--b.

[PATCH] NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID

[Chuck Lever]

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 */

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

[J. Bruce Fields]

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 */
>