| Message ID | 1270017003-7523-1-git-send-email-sjayaraman@suse.de |
|---|---|
| State | New |
| Headers | show |
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>
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 */ >
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 */ >> > > >
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 */ > >> > > > > > > > >
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 >
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 */ >> > > >
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 */
(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(-)