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

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 <[email protected]> wrote:
> 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]>
>



-- 
Thanks,

Steve