Patchwork cifs: Fix a kernel BUG with remote OS/2 server (try #3)

login
register
mail settings
Submitter Suresh Jayaraman
Date March 31, 2010, 6:30 a.m.
Message ID <1270017003-7523-1-git-send-email-sjayaraman@suse.de>
Download mbox | patch
Permalink /patch/49081/
State New
Headers show

Comments

Suresh Jayaraman - March 31, 2010, 6:30 a.m.
(Please consider for -stable once reviewed and accepted).

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
results in '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.

The workaround is to mask off high 16 bits if the number of bytes written as
returned by the server is greater than the bytes requested by the client as
suggested by Jeff Layton.

Cc: Jeff Layton <jlayton@samba.org>
Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/cifs/cifssmb.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
Jeff Layton - March 31, 2010, 9:52 a.m.
On Wed, 31 Mar 2010 12:00:03 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> (Please consider for -stable once reviewed and accepted).
> 
> 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
> results in '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.
> 
> The workaround is to mask off high 16 bits if the number of bytes written as
> returned by the server is greater than the bytes requested by the client as
> suggested by Jeff Layton.
> 
> Cc: Jeff Layton <jlayton@samba.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/cifs/cifssmb.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 7cc7f83..7d8ada8 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1517,6 +1517,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>  		*nbytes = le16_to_cpu(pSMBr->CountHigh);
>  		*nbytes = (*nbytes) << 16;
>  		*nbytes += le16_to_cpu(pSMBr->Count);
> +
> +		/*
> +		 * Mask off high 16 bits when bytes written as returned by the
> +		 * server is greater than bytes requested by the client. Some
> +		 * OS/2 servers are known to set incorrect CountHigh values.
> +		 */
> +		if (*nbytes > count)
> +			*nbytes &= 0xFFFF;
>  	}
>  
>  	cifs_buf_release(pSMB);
> @@ -1605,6 +1613,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>  		*nbytes = le16_to_cpu(pSMBr->CountHigh);
>  		*nbytes = (*nbytes) << 16;
>  		*nbytes += le16_to_cpu(pSMBr->Count);
> +
> +		/*
> +		 * Mask off high 16 bits when bytes written as returned by the
> +		 * server is greater than bytes requested by the client. OS/2
> +		 * servers are known to set incorrect CountHigh values.
> +		 */
> +		if (*nbytes > count)
> +			*nbytes &= 0xFFFF;
>  	}
>  
>  /*	cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
> 

Looks good to me.

Reviewed-by: Jeff Layton <jlayton@samba.org>
Steve French - March 31, 2010, 1:22 p.m.
I also like the change to this patch slightly more.  This and previous
patch merged.  Will send upstream request after a few days.

On Wed, Mar 31, 2010 at 12:30 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> (Please consider for -stable once reviewed and accepted).
>
> 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
> results in '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.
>
> The workaround is to mask off high 16 bits if the number of bytes written as
> returned by the server is greater than the bytes requested by the client as
> suggested by Jeff Layton.
>
> Cc: Jeff Layton <jlayton@samba.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/cifs/cifssmb.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 7cc7f83..7d8ada8 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1517,6 +1517,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>                *nbytes = le16_to_cpu(pSMBr->CountHigh);
>                *nbytes = (*nbytes) << 16;
>                *nbytes += le16_to_cpu(pSMBr->Count);
> +
> +               /*
> +                * Mask off high 16 bits when bytes written as returned by the
> +                * server is greater than bytes requested by the client. Some
> +                * OS/2 servers are known to set incorrect CountHigh values.
> +                */
> +               if (*nbytes > count)
> +                       *nbytes &= 0xFFFF;
>        }
>
>        cifs_buf_release(pSMB);
> @@ -1605,6 +1613,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>                *nbytes = le16_to_cpu(pSMBr->CountHigh);
>                *nbytes = (*nbytes) << 16;
>                *nbytes += le16_to_cpu(pSMBr->Count);
> +
> +               /*
> +                * Mask off high 16 bits when bytes written as returned by the
> +                * server is greater than bytes requested by the client. OS/2
> +                * servers are known to set incorrect CountHigh values.
> +                */
> +               if (*nbytes > count)
> +                       *nbytes &= 0xFFFF;
>        }
>
>  /*     cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
>
Suresh Jayaraman - March 31, 2010, 5:38 p.m.
On 03/31/2010 06:52 PM, Steve French wrote:
> I also like the change to this patch slightly more.  This and previous
> patch merged.  Will send upstream request after a few days.

Thanks, but just out of curiosity - Is there a reference that explains
how CountHigh could/could not be used by the server and the client?

> On Wed, Mar 31, 2010 at 12:30 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
>> (Please consider for -stable once reviewed and accepted).
>>
>> 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
>> results in '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.
>>
>> The workaround is to mask off high 16 bits if the number of bytes written as
>> returned by the server is greater than the bytes requested by the client as
>> suggested by Jeff Layton.
>>
>> Cc: Jeff Layton <jlayton@samba.org>
>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>> ---
>> �fs/cifs/cifssmb.c | � 16 ++++++++++++++++
>> �1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 7cc7f83..7d8ada8 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -1517,6 +1517,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
>> � � � � � � � �*nbytes = (*nbytes) << 16;
>> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
>> +
>> + � � � � � � � /*
>> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
>> + � � � � � � � �* server is greater than bytes requested by the client. Some
>> + � � � � � � � �* OS/2 servers are known to set incorrect CountHigh values.
>> + � � � � � � � �*/
>> + � � � � � � � if (*nbytes > count)
>> + � � � � � � � � � � � *nbytes &= 0xFFFF;
>> � � � �}
>>
>> � � � �cifs_buf_release(pSMB);
>> @@ -1605,6 +1613,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
>> � � � � � � � �*nbytes = (*nbytes) << 16;
>> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
>> +
>> + � � � � � � � /*
>> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
>> + � � � � � � � �* server is greater than bytes requested by the client. OS/2
>> + � � � � � � � �* servers are known to set incorrect CountHigh values.
>> + � � � � � � � �*/
>> + � � � � � � � if (*nbytes > count)
>> + � � � � � � � � � � � *nbytes &= 0xFFFF;
>> � � � �}
>>
>> �/* � � cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
>>
> 
> 
>
Jeff Layton - March 31, 2010, 5:53 p.m.
On Wed, 31 Mar 2010 23:08:56 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 03/31/2010 06:52 PM, Steve French wrote:
> > I also like the change to this patch slightly more.  This and previous
> > patch merged.  Will send upstream request after a few days.
> 
> Thanks, but just out of curiosity - Is there a reference that explains
> how CountHigh could/could not be used by the server and the client?
> 

+1

While the fix you have here seems like it'll prevent most problems, it
sure would be nice to have a flag or something to key off of that
states that the CountHigh field should be trusted.

> > On Wed, Mar 31, 2010 at 12:30 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >> (Please consider for -stable once reviewed and accepted).
> >>
> >> 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
> >> results in '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.
> >>
> >> The workaround is to mask off high 16 bits if the number of bytes written as
> >> returned by the server is greater than the bytes requested by the client as
> >> suggested by Jeff Layton.
> >>
> >> Cc: Jeff Layton <jlayton@samba.org>
> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> >> ---
> >> �fs/cifs/cifssmb.c | � 16 ++++++++++++++++
> >> �1 files changed, 16 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >> index 7cc7f83..7d8ada8 100644
> >> --- a/fs/cifs/cifssmb.c
> >> +++ b/fs/cifs/cifssmb.c
> >> @@ -1517,6 +1517,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
> >> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
> >> � � � � � � � �*nbytes = (*nbytes) << 16;
> >> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
> >> +
> >> + � � � � � � � /*
> >> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
> >> + � � � � � � � �* server is greater than bytes requested by the client. Some
> >> + � � � � � � � �* OS/2 servers are known to set incorrect CountHigh values.
> >> + � � � � � � � �*/
> >> + � � � � � � � if (*nbytes > count)
> >> + � � � � � � � � � � � *nbytes &= 0xFFFF;
> >> � � � �}
> >>
> >> � � � �cifs_buf_release(pSMB);
> >> @@ -1605,6 +1613,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
> >> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
> >> � � � � � � � �*nbytes = (*nbytes) << 16;
> >> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
> >> +
> >> + � � � � � � � /*
> >> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
> >> + � � � � � � � �* server is greater than bytes requested by the client. OS/2
> >> + � � � � � � � �* servers are known to set incorrect CountHigh values.
> >> + � � � � � � � �*/
> >> + � � � � � � � if (*nbytes > count)
> >> + � � � � � � � � � � � *nbytes &= 0xFFFF;
> >> � � � �}
> >>
> >> �/* � � cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
> >>
> > 
> > 
> > 
> 
>
Steve French - March 31, 2010, 6:13 p.m.
Not that I know of - but should be in Chris Hertel's smb document.

On Wed, Mar 31, 2010 at 11:38 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> On 03/31/2010 06:52 PM, Steve French wrote:
>> I also like the change to this patch slightly more.  This and previous
>> patch merged.  Will send upstream request after a few days.
>
> Thanks, but just out of curiosity - Is there a reference that explains
> how CountHigh could/could not be used by the server and the client?
>
>> On Wed, Mar 31, 2010 at 12:30 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>> (Please consider for -stable once reviewed and accepted).
>>>
>>> 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
>>> results in '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.
>>>
>>> The workaround is to mask off high 16 bits if the number of bytes written as
>>> returned by the server is greater than the bytes requested by the client as
>>> suggested by Jeff Layton.
>>>
>>> Cc: Jeff Layton <jlayton@samba.org>
>>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>>> ---
>>> �fs/cifs/cifssmb.c | � 16 ++++++++++++++++
>>> �1 files changed, 16 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>>> index 7cc7f83..7d8ada8 100644
>>> --- a/fs/cifs/cifssmb.c
>>> +++ b/fs/cifs/cifssmb.c
>>> @@ -1517,6 +1517,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>>> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
>>> � � � � � � � �*nbytes = (*nbytes) << 16;
>>> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
>>> +
>>> + � � � � � � � /*
>>> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
>>> + � � � � � � � �* server is greater than bytes requested by the client. Some
>>> + � � � � � � � �* OS/2 servers are known to set incorrect CountHigh values.
>>> + � � � � � � � �*/
>>> + � � � � � � � if (*nbytes > count)
>>> + � � � � � � � � � � � *nbytes &= 0xFFFF;
>>> � � � �}
>>>
>>> � � � �cifs_buf_release(pSMB);
>>> @@ -1605,6 +1613,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>>> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
>>> � � � � � � � �*nbytes = (*nbytes) << 16;
>>> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
>>> +
>>> + � � � � � � � /*
>>> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
>>> + � � � � � � � �* server is greater than bytes requested by the client. OS/2
>>> + � � � � � � � �* servers are known to set incorrect CountHigh values.
>>> + � � � � � � � �*/
>>> + � � � � � � � if (*nbytes > count)
>>> + � � � � � � � � � � � *nbytes &= 0xFFFF;
>>> � � � �}
>>>
>>> �/* � � cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
>>>
>>
>>
>>
>
>
> --
> Suresh Jayaraman
>
Suresh Jayaraman - April 1, 2010, 4:03 a.m.
On 03/31/2010 06:52 PM, Steve French wrote:
> I also like the change to this patch slightly more.  This and previous
> patch merged.  Will send upstream request after a few days.
> 
> On Wed, Mar 31, 2010 at 12:30 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
>> (Please consider for -stable once reviewed and accepted).
>>
>> 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
>> results in '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.
>>
>> The workaround is to mask off high 16 bits if the number of bytes written as
>> returned by the server is greater than the bytes requested by the client as
>> suggested by Jeff Layton.
>>
>> Cc: Jeff Layton <jlayton@samba.org>
>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>> ---

I just got this tested too by the reporter and it fixed the kernel BUG.
We might as well add

Reported-and-tested-by: James Moe <jimoe@sohnen-moe.com>


Thanks,
>> �fs/cifs/cifssmb.c | � 16 ++++++++++++++++
>> �1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 7cc7f83..7d8ada8 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -1517,6 +1517,14 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
>> � � � � � � � �*nbytes = (*nbytes) << 16;
>> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
>> +
>> + � � � � � � � /*
>> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
>> + � � � � � � � �* server is greater than bytes requested by the client. Some
>> + � � � � � � � �* OS/2 servers are known to set incorrect CountHigh values.
>> + � � � � � � � �*/
>> + � � � � � � � if (*nbytes > count)
>> + � � � � � � � � � � � *nbytes &= 0xFFFF;
>> � � � �}
>>
>> � � � �cifs_buf_release(pSMB);
>> @@ -1605,6 +1613,14 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>> � � � � � � � �*nbytes = le16_to_cpu(pSMBr->CountHigh);
>> � � � � � � � �*nbytes = (*nbytes) << 16;
>> � � � � � � � �*nbytes += le16_to_cpu(pSMBr->Count);
>> +
>> + � � � � � � � /*
>> + � � � � � � � �* Mask off high 16 bits when bytes written as returned by the
>> + � � � � � � � �* server is greater than bytes requested by the client. OS/2
>> + � � � � � � � �* servers are known to set incorrect CountHigh values.
>> + � � � � � � � �*/
>> + � � � � � � � if (*nbytes > count)
>> + � � � � � � � � � � � *nbytes &= 0xFFFF;
>> � � � �}
>>
>> �/* � � cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */
>>
> 
> 
>

Patch

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 7cc7f83..7d8ada8 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1517,6 +1517,14 @@  CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
 		*nbytes = le16_to_cpu(pSMBr->CountHigh);
 		*nbytes = (*nbytes) << 16;
 		*nbytes += le16_to_cpu(pSMBr->Count);
+
+		/*
+		 * Mask off high 16 bits when bytes written as returned by the
+		 * server is greater than bytes requested by the client. Some
+		 * OS/2 servers are known to set incorrect CountHigh values.
+		 */
+		if (*nbytes > count)
+			*nbytes &= 0xFFFF;
 	}
 
 	cifs_buf_release(pSMB);
@@ -1605,6 +1613,14 @@  CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
 		*nbytes = le16_to_cpu(pSMBr->CountHigh);
 		*nbytes = (*nbytes) << 16;
 		*nbytes += le16_to_cpu(pSMBr->Count);
+
+		/*
+		 * Mask off high 16 bits when bytes written as returned by the
+		 * server is greater than bytes requested by the client. OS/2
+		 * servers are known to set incorrect CountHigh values.
+		 */
+		if (*nbytes > count)
+			*nbytes &= 0xFFFF;
 	}
 
 /*	cifs_small_buf_release(pSMB); */ /* Freed earlier now in SendReceive2 */