Message ID | 1269938863-13779-1-git-send-email-sjayaraman@suse.de |
---|---|
State | New |
Headers | show |
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.
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,
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> >
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); }
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(-)