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

login
register
mail settings
Submitter Suresh Jayaraman
Date March 30, 2010, 8:47 a.m.
Message ID <1269938863-13779-1-git-send-email-sjayaraman@suse.de>
Download mbox | patch
Permalink /patch/48945/
State New
Headers show

Comments

Suresh Jayaraman - March 30, 2010, 8:47 a.m.
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 <sjayaraman@suse.de>
---
 fs/cifs/cifssmb.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
Jeff Layton - March 30, 2010, 11:38 a.m.
On Tue, 30 Mar 2010 14:17:43 +0530
Suresh Jayaraman <sjayaraman@suse.de> 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 <sjayaraman@suse.de>
> ---
>  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.
Suresh Jayaraman - March 30, 2010, 12:33 p.m.
On 03/30/2010 05:08 PM, Jeff Layton wrote:
> On Tue, 30 Mar 2010 14:17:43 +0530
> Suresh Jayaraman <sjayaraman@suse.de> 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 <sjayaraman@suse.de>
>> ---
>>  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.

Apologies, first of this should have been a RFC patch given that there
is not much clarity and there were assumptions.

Yes, the spec says upto 65536, but some of the discussions at MS CIFS
discussion group point out that it could be for larger writes -
http://archives.neohapsis.com/archives/microsoft/various/cifs/2002-q4/0004.html

However the discussion is not conclusive on CAP_LARGE_WRITE_X.

> 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?

Comparing traces between OS/2 server and Samba the various fields that
differ are (Server's response):

Field           OS/2      Samba

file rw length: 4096     16384
Count low:      4096     16384
Remaining:     65535         0
Count High         1         0
Reserved:       FFFF      0000

The MS-CIFS spec says Reserved must be o (0x00000000). Also note that
the file is a regular file (not a pipe or device file).

OTOH, thinking about the case where CountHigh value is set to 1 (atleast)

               *nbytes = le16_to_cpu(pSMBr->CountHigh);
               *nbytes = (*nbytes) << 16;

the number of bytes written should be >= 65536 (which might mean large
writes are in progress)?

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

Yes, absolutely.


Thanks,
Steve French - March 30, 2010, 2:07 p.m.
The general idea to sanity check the bytes written field makes sense
(writes greater than 64K are allowed - and Samba server even supports
writes greater than 128K when CIFS_UNIX_LARGE_WRITE_CAP is returned in
posix caps on the posix qfsinfo).  Isn't the other obvious check to
add - simply based on the frame length or the original write length
rather than the CAP_LARGE_WRITE?

On Tue, Mar 30, 2010 at 6:38 AM, Jeff Layton <jlayton@samba.org> wrote:
> On Tue, 30 Mar 2010 14:17:43 +0530
> Suresh Jayaraman <sjayaraman@suse.de> 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 <sjayaraman@suse.de>
>> ---
>>  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 <jlayton@samba.org>
>

Patch

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);
 	}