[linux-cifs-client] [PATCH] cifs: Fix a kernel BUG with remote OS/2 server

[Suresh Jayaraman]

On 03/30/2010 05:08 PM, Jeff Layton wrote:
> On Tue, 30 Mar 2010 14:17:43 +0530
> Suresh Jayaraman <sjayaraman at 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 at 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,

-- 
Suresh Jayaraman