Re: [PATCH] cifs: Fix a kernel BUG with remote OS/2 server

On Tue, 30 Mar 2010 23:46:07 +0530
Suresh Jayaraman <[email protected]> wrote:

> Sorry about one more iteration -- fixed a few checkpatch warnings.
> 
> While chasing a bug report involving a OS/2 server, I noticed the server sets
> pSMBr->CountHigh to a incorrect value even in case of normal writes. This
> leads to 'nbytes' being computed wrongly and triggers a kernel BUG at
> mm/filemap.c.
> 
> void iov_iter_advance(struct iov_iter *i, size_t bytes)
> {
>         BUG_ON(i->count < bytes);    <--- BUG here
> 
> Why the server is setting 'CountHigh' is not clear but only does so after
> writing 64k bytes. Though this looks like the server bug, the client side
> crash may not be acceptable.
> 
> When the write returns successfully and the bytes written as returned by
> the server is greater than bytes requested by the client, reset it to original
> bytes requested.
> 
> 
> Cc: Jeff Layton <[email protected]>
> Signed-off-by: Suresh Jayaraman <[email protected]>
> ---
>  fs/cifs/cifssmb.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 7cc7f83..5872e58 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1517,6 +1517,16 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>  		*nbytes = le16_to_cpu(pSMBr->CountHigh);
>  		*nbytes = (*nbytes) << 16;
>  		*nbytes += le16_to_cpu(pSMBr->Count);
> +
> +		/*
> +		 * Workaround: Some legacy servers (read OS/2) might incorrectly
> +		 * set CountHigh for normal writes resulting in wrong 'nbytes'
> +		 * value. So when the write returns successfully and the bytes
> +		 * written as returned by the server is greater than bytes
> +		 * requested, reset it to original bytes requested.
> +		 */
> +		if (*nbytes > count)
> +			*nbytes = count;
>  	}
>  
>  	cifs_buf_release(pSMB);
> @@ -1605,6 +1615,16 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>  		*nbytes = le16_to_cpu(pSMBr->CountHigh);
>  		*nbytes = (*nbytes) << 16;
>  		*nbytes += le16_to_cpu(pSMBr->Count);
> +
> +		/* Workaround: Some legacy servers (read OS/2) might incorrectly
> +		 * set CountHigh for normal writes resulting in wrong 'nbytes'
> +		 * value. So when the write returns successfully and the bytes
> +		 * written as returned by the server is greater than bytes
> +		 * requested, reset it to original bytes requested.
> +		 */
> +
> +		if (*nbytes > count)
> +			*nbytes = count;
>  	}
>  
>  /*	cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
> 

So what happens if the server returned less than "count" bytes in the
->Count field and put junk in the CountHigh? You'll now be returning
that "count" bytes were written when they really weren't.

I don't think that's what you want. I think Steve was suggesting
that we don't use "CountHigh" when that would make it look like the
server wrote more than was requested. I think rather than setting
*nbytes to count, you should just mask off the upper 16 bits in *nbytes.
-- 
Jeff Layton <[email protected]>