[PATCH 0/5] nfs41: return correct errors on callback replays

[PATCH 0/5] nfs41: return correct errors on callback replays

[andros]

The first three patches clean up callback processing
0001-nfs41-fix-wrong-error-on-callback-decode-hdr-overflo.patch
0002-nfs41-directly-encode-back-channel-error.patch
0003-nfs41-remove-uneeded-checks-in-callback-processing.patch

These next two implement correct error returns for v4.1 callback replays.
Since our back channel has a ca_maxrequestsize_cached = 0, a replay with
cachethis set to true results in a NFS4ERR_TOO_BIG_TO_CACHE error.
This code is set up to do a real DRC.

A replay with cachethis set to false returns a NFS4ERR_RETRY_UNCACHED_REP
error.

0004-nfs41-prepare-for-back-channel-drc.patch
0005-nfs41-back-channel-drc-minimal-implementation.patch

TODO: The callback code currently returns NFS4ERR_RESOURCE on all xdr
overflows. This is correct for v4.0, incorrect for v4.1.

Testing:

Modified nfsv4.1 pynfs server tested cb_recall replays with the cb_sequence
cachethis set to False and to True.

Connectathon tests pass.

-->Andy

[PATCH 1/5] nfs41: fix wrong error on callback decode hdr overflow

[andros]

From: Andy Adamson <andros@netapp.com>

decode_op_hdr returns NFS4ERR_RESOURCE on decode buffer overflow which is
correct for v4.0. Will fix the return for v4.1 along with all the other
NFS4ERR_RESOURCE overflow errors in a later patch.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_xdr.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 8e1a251..e24487d 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -605,17 +605,15 @@ static __be32 process_op(uint32_t minorversion, int nop,
 		struct xdr_stream *xdr_out, void *resp)
 {
 	struct callback_op *op = &callback_ops[0];
-	unsigned int op_nr = OP_CB_ILLEGAL;
+	unsigned int op_nr;
 	__be32 status;
 	long maxlen;
 	__be32 res;
 
 	dprintk("%s: start\n", __func__);
 	status = decode_op_hdr(xdr_in, &op_nr);
-	if (unlikely(status)) {
-		status = htonl(NFS4ERR_OP_ILLEGAL);
+	if (unlikely(status))
 		goto out;
-	}
 
 	dprintk("%s: minorversion=%d nop=%d op_nr=%u\n",
 		__func__, minorversion, nop, op_nr);
-- 
1.6.5.2

[PATCH 2/5] nfs41: directly encode back channel error

[andros]

From: Andy Adamson <andros@netapp.com>

Skip all other processing when error is encountered.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_xdr.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index e24487d..3637e9a 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -613,7 +613,7 @@ static __be32 process_op(uint32_t minorversion, int nop,
 	dprintk("%s: start\n", __func__);
 	status = decode_op_hdr(xdr_in, &op_nr);
 	if (unlikely(status))
-		goto out;
+		goto encode_hdr;
 
 	dprintk("%s: minorversion=%d nop=%d op_nr=%u\n",
 		__func__, minorversion, nop, op_nr);
@@ -622,16 +622,19 @@ static __be32 process_op(uint32_t minorversion, int nop,
 				preprocess_nfs4_op(op_nr, &op);
 	if (status == htonl(NFS4ERR_OP_ILLEGAL))
 		op_nr = OP_CB_ILLEGAL;
-out:
+	if (status)
+		goto encode_hdr;
+
 	maxlen = xdr_out->end - xdr_out->p;
 	if (maxlen > 0 && maxlen < PAGE_SIZE) {
-		if (likely(status == 0 && op->decode_args != NULL))
+		if (likely(op->decode_args != NULL))
 			status = op->decode_args(rqstp, xdr_in, argp);
 		if (likely(status == 0 && op->process_op != NULL))
 			status = op->process_op(argp, resp);
 	} else
 		status = htonl(NFS4ERR_RESOURCE);
 
+encode_hdr:
 	res = encode_op_hdr(xdr_out, op_nr, status);
 	if (status == 0)
 		status = res;
-- 
1.6.5.2

[PATCH 3/5] nfs41: remove uneeded checks in callback processing

[andros]

From: Andy Adamson <andros@netapp.com>

All callback operations have arguments to decode and require processing.
The preprocess_nfs4X_op functions catch unsupported or illegal ops so
decode_args and process_op pointers are always non NULL.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_xdr.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 3637e9a..c64a174 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -627,9 +627,8 @@ static __be32 process_op(uint32_t minorversion, int nop,
 
 	maxlen = xdr_out->end - xdr_out->p;
 	if (maxlen > 0 && maxlen < PAGE_SIZE) {
-		if (likely(op->decode_args != NULL))
-			status = op->decode_args(rqstp, xdr_in, argp);
-		if (likely(status == 0 && op->process_op != NULL))
+		status = op->decode_args(rqstp, xdr_in, argp);
+		if (likely(status == 0))
 			status = op->process_op(argp, resp);
 	} else
 		status = htonl(NFS4ERR_RESOURCE);
-- 
1.6.5.2

[PATCH 4/5] nfs41: prepare for back channel drc

[andros]

From: Andy Adamson <andros@netapp.com>

Make all cb_sequence arguments available to verify_seqid which will make
replay decisions.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_proc.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index defa9b4..7f92b6d 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -153,34 +153,34 @@ int nfs41_validate_delegation_stateid(struct nfs_delegation *delegation, const n
  * a single outstanding callback request at a time.
  */
 static int
-validate_seqid(struct nfs4_slot_table *tbl, u32 slotid, u32 seqid)
+validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 {
 	struct nfs4_slot *slot;
 
 	dprintk("%s enter. slotid %d seqid %d\n",
-		__func__, slotid, seqid);
+		__func__, args->csa_slotid, args->csa_sequenceid);
 
-	if (slotid > NFS41_BC_MAX_CALLBACKS)
+	if (args->csa_slotid > NFS41_BC_MAX_CALLBACKS)
 		return htonl(NFS4ERR_BADSLOT);
 
-	slot = tbl->slots + slotid;
+	slot = tbl->slots + args->csa_slotid;
 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
 
 	/* Normal */
-	if (likely(seqid == slot->seq_nr + 1)) {
+	if (likely(args->csa_sequenceid == slot->seq_nr + 1)) {
 		slot->seq_nr++;
 		return htonl(NFS4_OK);
 	}
 
 	/* Replay */
-	if (seqid == slot->seq_nr) {
+	if (args->csa_sequenceid == slot->seq_nr) {
 		dprintk("%s seqid %d is a replay - no DRC available\n",
-			__func__, seqid);
+			__func__, args->csa_sequenceid);
 		return htonl(NFS4_OK);
 	}
 
 	/* Wraparound */
-	if (seqid == 1 && (slot->seq_nr + 1) == 0) {
+	if (args->csa_sequenceid == 1 && (slot->seq_nr + 1) == 0) {
 		slot->seq_nr = 1;
 		return htonl(NFS4_OK);
 	}
@@ -241,8 +241,7 @@ unsigned nfs4_callback_sequence(struct cb_sequenceargs *args,
 	if (clp == NULL)
 		goto out;
 
-	status = validate_seqid(&clp->cl_session->bc_slot_table,
-				args->csa_slotid, args->csa_sequenceid);
+	status = validate_seqid(&clp->cl_session->bc_slot_table, args);
 	if (status)
 		goto out_putclient;
 
-- 
1.6.5.2

[PATCH 5/5] nfs41: back channel drc minimal implementation

[andros]

From: Andy Adamson <andros@netapp.com>

For now the back channel ca_maxresponsesize_cached is 0 and there is no
backchannel DRC. Return NFS4ERR_REP_TOO_BIG_TO_CACHE when a cb_sequence
cachethis is true.  When it is false, return NFS4ERR_RETRY_UNCACHED_REP as the
next operation error.

Remember the replay error accross compound operation processing.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_proc.c |   25 +++++++++++++++++--------
 fs/nfs/callback_xdr.c  |   19 +++++++++++++++----
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 7f92b6d..3cc2333 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -143,9 +143,8 @@ int nfs41_validate_delegation_stateid(struct nfs_delegation *delegation, const n
  * Return success if the sequenceID is one more than what we last saw on
  * this slot, accounting for wraparound.  Increments the slot's sequence.
  *
- * We don't yet implement a duplicate request cache, so at this time
- * we will log replays, and process them as if we had not seen them before,
- * but we don't bump the sequence in the slot.  Not too worried about it,
+ * We don't yet implement a duplicate request cache, instead we set the
+ * back channel ca_maxresponsesize_cached to zero. This is OK for now
  * since we only currently implement idempotent callbacks anyway.
  *
  * We have a single slot backchannel at this time, so we don't bother
@@ -174,9 +173,15 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 
 	/* Replay */
 	if (args->csa_sequenceid == slot->seq_nr) {
-		dprintk("%s seqid %d is a replay - no DRC available\n",
+		dprintk("%s seqid %d is a replay\n",
 			__func__, args->csa_sequenceid);
-		return htonl(NFS4_OK);
+		/* Signal process_op to set this error on next op */
+		if (args->csa_cachethis == 0)
+			return htonl(NFS4ERR_RETRY_UNCACHED_REP);
+
+		/* The ca_maxresponsesize_cached is 0 with no DRC */
+		else if (args->csa_cachethis == 1)
+			return htonl(NFS4ERR_REP_TOO_BIG_TO_CACHE);
 	}
 
 	/* Wraparound */
@@ -255,9 +260,13 @@ unsigned nfs4_callback_sequence(struct cb_sequenceargs *args,
 out_putclient:
 	nfs_put_client(clp);
 out:
-	dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
-	res->csr_status = status;
-	return res->csr_status;
+	if (status == htonl(NFS4ERR_RETRY_UNCACHED_REP))
+		res->csr_status = 0;
+	else
+		res->csr_status = status;
+	dprintk("%s: exit with status = %d res->csr_status %d\n", __func__,
+		ntohl(status), ntohl(res->csr_status));
+	return status;
 }
 
 unsigned nfs4_callback_recallany(struct cb_recallanyargs *args, void *dummy)
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index c64a174..76d082e 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -602,7 +602,7 @@ preprocess_nfs4_op(unsigned int op_nr, struct callback_op **op)
 static __be32 process_op(uint32_t minorversion, int nop,
 		struct svc_rqst *rqstp,
 		struct xdr_stream *xdr_in, void *argp,
-		struct xdr_stream *xdr_out, void *resp)
+		struct xdr_stream *xdr_out, void *resp, int* drc_status)
 {
 	struct callback_op *op = &callback_ops[0];
 	unsigned int op_nr;
@@ -625,6 +625,11 @@ static __be32 process_op(uint32_t minorversion, int nop,
 	if (status)
 		goto encode_hdr;
 
+	if (*drc_status) {
+		status = *drc_status;
+		goto encode_hdr;
+	}
+
 	maxlen = xdr_out->end - xdr_out->p;
 	if (maxlen > 0 && maxlen < PAGE_SIZE) {
 		status = op->decode_args(rqstp, xdr_in, argp);
@@ -633,6 +638,12 @@ static __be32 process_op(uint32_t minorversion, int nop,
 	} else
 		status = htonl(NFS4ERR_RESOURCE);
 
+	/* Only set by OP_CB_SEQUENCE processing */
+	if (status == htonl(NFS4ERR_RETRY_UNCACHED_REP)) {
+		*drc_status = status;
+		status = 0;
+	}
+
 encode_hdr:
 	res = encode_op_hdr(xdr_out, op_nr, status);
 	if (status == 0)
@@ -652,7 +663,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
 	struct cb_compound_hdr_res hdr_res = { NULL };
 	struct xdr_stream xdr_in, xdr_out;
 	__be32 *p;
-	__be32 status;
+	__be32 status, drc_status = 0;
 	unsigned int nops = 0;
 
 	dprintk("%s: start\n", __func__);
@@ -672,8 +683,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
 		return rpc_system_err;
 
 	while (status == 0 && nops != hdr_arg.nops) {
-		status = process_op(hdr_arg.minorversion, nops,
-				    rqstp, &xdr_in, argp, &xdr_out, resp);
+		status = process_op(hdr_arg.minorversion, nops, rqstp,
+				    &xdr_in, argp, &xdr_out, resp, &drc_status);
 		nops++;
 	}
 
-- 
1.6.5.2

Re: [PATCH 2/5] nfs41: directly encode back channel error

[Trond Myklebust]

On Wed, 2010-01-06 at 13:23 -0500, andros@netapp.com wrote: 
> From: Andy Adamson <andros@netapp.com>
> 
> Skip all other processing when error is encountered.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/callback_xdr.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index e24487d..3637e9a 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -613,7 +613,7 @@ static __be32 process_op(uint32_t minorversion, int nop,
>  	dprintk("%s: start\n", __func__);
>  	status = decode_op_hdr(xdr_in, &op_nr);
>  	if (unlikely(status))
> -		goto out;
> +		goto encode_hdr;
>  
>  	dprintk("%s: minorversion=%d nop=%d op_nr=%u\n",
>  		__func__, minorversion, nop, op_nr);
> @@ -622,16 +622,19 @@ static __be32 process_op(uint32_t minorversion, int nop,
>  				preprocess_nfs4_op(op_nr, &op);
>  	if (status == htonl(NFS4ERR_OP_ILLEGAL))
>  		op_nr = OP_CB_ILLEGAL;
> -out:
> +	if (status)
> +		goto encode_hdr;
> +
>  	maxlen = xdr_out->end - xdr_out->p;
>  	if (maxlen > 0 && maxlen < PAGE_SIZE) {
> -		if (likely(status == 0 && op->decode_args != NULL))
> +		if (likely(op->decode_args != NULL))
>  			status = op->decode_args(rqstp, xdr_in, argp);
>  		if (likely(status == 0 && op->process_op != NULL))
>  			status = op->process_op(argp, resp);
>  	} else
>  		status = htonl(NFS4ERR_RESOURCE);
>  
> +encode_hdr:
>  	res = encode_op_hdr(xdr_out, op_nr, status);
>  	if (status == 0)
>  		status = res;

Same problem. You have a random value for op_nr...

Re: [PATCH 1/5] nfs41: fix wrong error on callback decode hdr overflow

[Trond Myklebust]

On Wed, 2010-01-06 at 13:23 -0500, andros@netapp.com wrote: 
> From: Andy Adamson <andros@netapp.com>
> 
> decode_op_hdr returns NFS4ERR_RESOURCE on decode buffer overflow which is
> correct for v4.0. Will fix the return for v4.1 along with all the other
> NFS4ERR_RESOURCE overflow errors in a later patch.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/callback_xdr.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index 8e1a251..e24487d 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -605,17 +605,15 @@ static __be32 process_op(uint32_t minorversion, int nop,
>  		struct xdr_stream *xdr_out, void *resp)
>  {
>  	struct callback_op *op = &callback_ops[0];
> -	unsigned int op_nr = OP_CB_ILLEGAL;
> +	unsigned int op_nr;

This will cause us to return a random op number in the case where the
buffer overflows.

> 	__be32 status;
>  	long maxlen;
>  	__be32 res;
>  
>  	dprintk("%s: start\n", __func__);
>  	status = decode_op_hdr(xdr_in, &op_nr);
> -	if (unlikely(status)) {
> -		status = htonl(NFS4ERR_OP_ILLEGAL);
> +	if (unlikely(status))
>  		goto out;
> -	}
>  
>  	dprintk("%s: minorversion=%d nop=%d op_nr=%u\n",
>  		__func__, minorversion, nop, op_nr);

The correct thing to do would appear to be rather to set
NFS4ERR_RESOURCE in the CB_COMPOUND return value, and simply not to
return an op here at all.

Trond

Re: [PATCH 1/5] nfs41: fix wrong error on callback decode hdr overflow

[Andy Adamson]

On Jan 11, 2010, at 6:08 PM, Trond Myklebust wrote:

> On Wed, 2010-01-06 at 13:23 -0500, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> decode_op_hdr returns NFS4ERR_RESOURCE on decode buffer overflow  
>> which is
>> correct for v4.0. Will fix the return for v4.1 along with all the  
>> other
>> NFS4ERR_RESOURCE overflow errors in a later patch.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfs/callback_xdr.c |    6 ++----
>> 1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
>> index 8e1a251..e24487d 100644
>> --- a/fs/nfs/callback_xdr.c
>> +++ b/fs/nfs/callback_xdr.c
>> @@ -605,17 +605,15 @@ static __be32 process_op(uint32_t  
>> minorversion, int nop,
>> 		struct xdr_stream *xdr_out, void *resp)
>> {
>> 	struct callback_op *op = &callback_ops[0];
>> -	unsigned int op_nr = OP_CB_ILLEGAL;
>> +	unsigned int op_nr;
>
> This will cause us to return a random op number in the case where the
> buffer overflows.

yes - good catch.

>
>> 	__be32 status;
>> 	long maxlen;
>> 	__be32 res;
>>
>> 	dprintk("%s: start\n", __func__);
>> 	status = decode_op_hdr(xdr_in, &op_nr);
>> -	if (unlikely(status)) {
>> -		status = htonl(NFS4ERR_OP_ILLEGAL);
>> +	if (unlikely(status))
>> 		goto out;
>> -	}
>>
>> 	dprintk("%s: minorversion=%d nop=%d op_nr=%u\n",
>> 		__func__, minorversion, nop, op_nr);
>
> The correct thing to do would appear to be rather to set
> NFS4ERR_RESOURCE in the CB_COMPOUND return value, and simply not to
> return an op here at all.

Agreed.

-->Andy
>
> Trond