[patch] svcrdma: endian bug in send_write_chunks()

Sparse complains because arg_ch->rs_length is declared as network
endian but we're treating it as CPU endian.

Signed-off-by: Dan Carpenter <redacted>

diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
index 249a835..30fda86 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c

@@ -409,7 +409,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt,
 		u64 rs_offset;
 
 		arg_ch = &arg_ary->wc_array[chunk_no].wc_target;
-		write_len = min(xfer_len, arg_ch->rs_length);
+		write_len = min(xfer_len, ntohl(arg_ch->rs_length));
 
 		/* Prepare the response chunk given the length actually
 		 * written */

@@ -481,7 +481,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt,
 	     chunk_no++) {
 		u64 rs_offset;
 		ch = &arg_ary->wc_array[chunk_no].wc_target;
-		write_len = min(xfer_len, ch->rs_length);
+		write_len = min(xfer_len, ntohl(ch->rs_length));
 
 		/* Prepare the reply chunk given the length actually
 		 * written */
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jan 12, 2012 at 09:47:22AM +0300, Dan Carpenter wrote:

Sparse complains because arg_ch->rs_length is declared as network
endian but we're treating it as CPU endian.

This looks like it would actually change behavior on a little endian
architecture, so how did this work before?

From some quick grepping, I see assignments both of the form

	...rs_length = ntohl(...)

and

	...rs_length = htonl(...)

but only see one declaration for a field named rs_length.

So my best guess would be that the code is ugly but working as is, and
needs cleanup by someone who knows how this field was intended to be
used.

?

--b.

On Thu, 2012-01-12 at 11:21 -0500, J. Bruce Fields wrote: 

On Thu, Jan 12, 2012 at 09:47:22AM +0300, Dan Carpenter wrote:

quoted

Sparse complains because arg_ch->rs_length is declared as network
endian but we're treating it as CPU endian.

This looks like it would actually change behavior on a little endian
architecture, so how did this work before?

quoted

From some quick grepping, I see assignments both of the form

	...rs_length = ntohl(...)

and

	...rs_length = htonl(...)

but only see one declaration for a field named rs_length.

So my best guess would be that the code is ugly but working as is, and
needs cleanup by someone who knows how this field was intended to be
used.

It looks to me as if rs_handle and rs_offset are being similarly abused.
Basically, we need a serious clean up in svc_rdma_marshall.c to separate
out those variables that are in XDR-encoded form and those that are not.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

On 1/12/12 1:15 PM, Trond Myklebust wrote:

On Thu, 2012-01-12 at 11:21 -0500, J. Bruce Fields wrote:

quoted

On Thu, Jan 12, 2012 at 09:47:22AM +0300, Dan Carpenter wrote:

quoted

Sparse complains because arg_ch->rs_length is declared as network
endian but we're treating it as CPU endian.

This looks like it would actually change behavior on a little endian
architecture, so how did this work before?

quoted

From some quick grepping, I see assignments both of the form

	...rs_length = ntohl(...)

and

	...rs_length = htonl(...)

but only see one declaration for a field named rs_length.

So my best guess would be that the code is ugly but working as is, and
needs cleanup by someone who knows how this field was intended to be
used.

It looks to me as if rs_handle and rs_offset are being similarly abused.
Basically, we need a serious clean up in svc_rdma_marshall.c to separate
out those variables that are in XDR-encoded form and those that are not.

The abuse is taking place because the marshal/unmarshall is being done 
in-place and it seemed wasteful at the time to add a chunk of memory to 
preserve the aesthetic. A union would 'work', but you still wouldn't 
'know' whether the data was NBO or not by where it was -- which seems like 
the intent of the __beXX in the first place.

Tom