Re: [PATCH 0/2 v4] nfs: return nfs4 compound header status on op header decoding error

[Benny Halevy <bhalevy@panasas.com>]

On Jul. 16, 2008, 0:57 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> On Thu, 2008-07-03 at 20:49 +0300, Benny Halevy wrote:
>> Trond,  following our conversation during the Connectathon
>> I reduced this patch as much as possible and restricted the
>> use of the compound header status to cases where op_hdr
>> decoding hit an error.
>>
>> This is needed for nfs41 for graceful fallback when trying
>> to mount a 4.0 server with 4.1.  In this case the server
>> returns no ops and the hdr status is set to
>> NFS4ERR_MINOR_VERS_MISMATCH.
>>
>> Please consider these patches:
>>
>> [PATCH 1/2] nfs: return nfs4 compound header status on op header decoding error
> 
> No, this patch doesn't look right either. If we overrun the end of the
> reply buffer, then xdr_inline_decode() will return a NULL pointer, so
> you should never hit the your (opnum != expected) case.

In patch 1, this is supposed to be handled like this:

decode_compound_hdr:
+	xdr->status = hdr->status;

decode_op_hdr:
+	p = xdr_inline_decode(xdr, 8);
+	if (unlikely(!p)) {
...
+		goto err;
+	}

+err:
+	if (xdr->status != NFS_OK)
+		return nfs4_stat_to_errno(xdr->status);

> 
> So given that your concern is primarily the case where nops==0, why
> don't you just add that particular case to decode_compound_hdr?
> 
> IOW: something like
> 
> 
> @@static int decode_compound_hdr(
> 	p += XDR_QUADLEN(hdr->taglen);
> 	READ32(hdr->nops);
> +	if (hdr->nops < 1)
> +		return nfs4_stat_to_errno(hdr->status);
> 	return 0;
> }
> 

This certainly provides a shortcut for the nops==0 case.
However, it doesn't solve the OP_ILLEGAL case all other
cases where xdr_inline_decode failed or opnum != expected,
we can easily handle the OP_ILLEGAL case explicitly in
decode_op_hdr.  Are you ok with the approach of
carrying the hdr.status in xdr->status for decode_op_hdr
use, or do you rather prefer to leave things as they are
for the invalid cases and explicitly handle only the
nops==0 and OP_ILLEGAL cases?

Benny