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

On Tue, 30 Mar 2010 14:17:43 +0530
Suresh Jayaraman <[email protected]> wrote:

> 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.
> 
> Looking up the CIFS spec, the purpose of 'CountHigh' field in WriteAndX
> response is not clear. But since 'nbytes' can be greater than 64k only
> in case of large writes (default writes can be upto 56k), I assume 'CountHigh'
> is used only for larger writes. The below patch workarounds the case when
> servers set 'CountHigh' incorrectly for normal writes.
> 
> Signed-off-by: Suresh Jayaraman <[email protected]>
> ---
>  fs/cifs/cifssmb.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 7cc7f83..ceced62 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1514,8 +1514,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>  		cFYI(1, ("Send error in write = %d", rc));
>  		*nbytes = 0;
>  	} else {
> -		*nbytes = le16_to_cpu(pSMBr->CountHigh);
> -		*nbytes = (*nbytes) << 16;
> +		/*
> +		 * Some legacy servers (read OS/2) might incorrectly set
> +		 * CountHigh for normal writes resulting in wrong 'nbytes' value
> +		 */
> +		if (tcon->ses->capabilities & CAP_LARGE_WRITE_X) {
> +			*nbytes = le16_to_cpu(pSMBr->CountHigh);
> +			*nbytes = (*nbytes) << 16;
> +		}
>  		*nbytes += le16_to_cpu(pSMBr->Count);
>  	}
>  
> @@ -1602,8 +1608,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>  		rc = -EIO;
>  	} else {
>  		WRITE_RSP *pSMBr = (WRITE_RSP *)iov[0].iov_base;
> -		*nbytes = le16_to_cpu(pSMBr->CountHigh);
> -		*nbytes = (*nbytes) << 16;
> +		/*
> +		 * Some legacy servers (read OS/2) might incorrectly set
> +		 * CountHigh for normal writes resulting in wrong 'nbytes' value
> +		 */
> +		if (tcon->ses->capabilities & CAP_LARGE_WRITE_X) {
> +			*nbytes = le16_to_cpu(pSMBr->CountHigh);
> +			*nbytes = (*nbytes) << 16;
> +		}
>  		*nbytes += le16_to_cpu(pSMBr->Count);
>  	}
>  

MS-SMB says that CAP_LARGE_WRITE_X allows you to write up to 65536
bytes. But, writing that many bytes doesn't require more than the 16
bit count field. The MS-SMB/CIFS specs say that the CountHigh field is
"Reserved" and make no mention of it.

I have a vague recollection of someone (Jeremy?) mentioning using a
portion of the reserved field to handle large writes, but I don't
recall how they indicated its presence. I'm not sure that
CAP_LARGE_WRITE_X is the right bit to key off of here. Might there be
servers that set that bit, but still put junk the the "reserved"
section?

Either way, I think we need some clarification on when it's safe to
trust the CountHigh field.

-- 
Jeff Layton <[email protected]>