Re: NFSv3/v2: Fix data corruption with NFS short reads.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3395 bytes --]

On Fri, 29 Mar 2013 19:57:06 -0000 malahal naineni <malahal@us.ibm.com> wrote:

> This bug seems to be present in v2.6.37 or lower versions. The code was
> re-organized in v2.6.38 that eliminated the bug. Current upstream code
> doesn't have this bug. This may be applicable to some longterm releases!
> 
> Here are the bug details:
> 
> 1. nfs_read_rpcsetup(), args.count and res.count are both set to the
>    actual number of bytes to be read. Let us assume that the request is
>    for 16K, so arg.count = res.count = 16K
> 2. nfs3_xdr_readres() conditionally sets res.count to to the actual
>    number of bytes read. This condition is true for the first response
>    as res.count was set to args.count before the first request. Let us
>    say the server returned only 4K bytes. res.count=4K
> 3. Another read request is sent for the remaining data. Note that
>    res.count is NOT updated. It is still set to the actual amount of
>    bytes we got in the first response.  The client will send a READ
>    request for the remaining 12K.

This is looks like a real bug, but I think the "NOT" above is the best thing
to fix.
i.e. when another read request is set, res.count *SHOULD*BE* updated.  That
makes it consistent with the original send, and consistency is good!

So this patch.

Index: linux-2.6.32-SLE11-SP1-LTSS/fs/nfs/read.c
===================================================================
--- linux-2.6.32-SLE11-SP1-LTSS.orig/fs/nfs/read.c	2013-03-20 16:24:31.426605189 +1100
+++ linux-2.6.32-SLE11-SP1-LTSS/fs/nfs/read.c	2013-04-11 11:19:57.670724540 +1000
@@ -368,6 +368,7 @@ static void nfs_readpage_retry(struct rp
 	argp->offset += resp->count;
 	argp->pgbase += resp->count;
 	argp->count -= resp->count;
+	resp->count = argp->count;
 	nfs4_restart_rpc(task, NFS_SERVER(data->inode)->nfs_client);
 	return;
 out:


This would old affect clients with servers which would sometimes return
partial reads, and I don't think the Linux NFS server does.  What server have
you seen this against?

NeilBrown



> 4. Assume that the server gave all 12K bytes. We still think we got ony
>    4K bytes due to conditional update in nfs3_xdr_readres(). The client
>    sends further requests, if not EOF! If this response includes EOF, it
>    truncates pages beyond 4K+4K causing data corruption beyond 8K. The
>    corrupted data is filled with zeros!
> 
> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> 
> ---
> fs/nfs/nfs2xdr.c | 3 +--
>  fs/nfs/nfs3xdr.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> index 5914a19..710ca56 100644
> --- a/fs/nfs/nfs2xdr.c
> +++ b/fs/nfs/nfs2xdr.c
> @@ -287,8 +287,7 @@ nfs_xdr_readres(struct rpc_rqst *req, __be32 *p, struct nfs_readres *res)
>  	}
>  
>  	dprintk("RPC:      readres OK count %u\n", count);
> -	if (count < res->count)
> -		res->count = count;
> +	res->count = count;
>  
>  	return count;
>  }
> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> index f6cc60f..152a5e4 100644
> --- a/fs/nfs/nfs3xdr.c
> +++ b/fs/nfs/nfs3xdr.c
> @@ -914,8 +914,7 @@ nfs3_xdr_readres(struct rpc_rqst *req, __be32 *p, struct nfs_readres *res)
>  		res->eof = 0;
>  	}
>  
> -	if (count < res->count)
> -		res->count = count;
> +	res->count = count;
>  
>  	return count;
>  }


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]