Patchwork cifs: Fix a kernel BUG with remote OS/2 server

login
register
mail settings
Submitter Suresh Jayaraman
Date March 30, 2010, 6:16 p.m.
Message ID <1269972967-6453-1-git-send-email-sjayaraman@suse.de>
Download mbox | patch
Permalink /patch/49005/
State New
Headers show

Comments

Suresh Jayaraman - March 30, 2010, 6:16 p.m.
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 <jlayton@samba.org>
Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/cifs/cifssmb.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
Jeff Layton - March 30, 2010, 6:35 p.m.
On Tue, 30 Mar 2010 23:46:07 +0530
Suresh Jayaraman <sjayaraman@suse.de> 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 <jlayton@samba.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  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.

Patch

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 */