| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH2r5mvTtsDVaYWYswo+CgFRRZJ-+hWTca-3V9vZP7wh0ezfyw@mail.gmail.com> Date: Tue, 2 Aug 2011 11:03:28 -0500 From: Steve French <smfrench@...il.com> To: Jeff Layton <jlayton@...hat.com> Cc: Jan Seiffert <kaffeemonster@...glemail.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, sfrench@...ibm.com, linux-cifs@...r.kernel.org Subject: Re: [3.0.0+][Regression][Bisected] CIFS: getdents() broken for large dirs your patch does look right. On Tue, Aug 2, 2011 at 11:00 AM, Jeff Layton <jlayton@...hat.com> wrote: > On Tue, 2 Aug 2011 06:44:55 -0400 > Jeff Layton <jlayton@...hat.com> wrote: > >> On Tue, 2 Aug 2011 02:30:35 +0200 >> Jan Seiffert <kaffeemonster@...glemail.com> wrote: >> >> > Please CC, as not subscibed. >> > >> > Looks like something broke CIFS. >> > Kernel 3.0.0 works, master after the CIFS merge does not ATM. >> > >> > Example cifs mount is: >> > $ mount >> > ... >> > //server/portage on /usr/portage type cifs (rw,mand) >> > >> > Server is: >> > $ smbd --version >> > Version 3.4.12 >> > >> > Actual Output is: >> > $ ls -l /usr/portage/ | wc -l >> > ls: reading directory /usr/portage/: input/output error >> > 1 >> > $ ls -l /usr/portage/distfiles | wc -l >> > 105 >> > >> > Expected Output: >> > $ ls -l /usr/portage/ | wc -l >> > 170 >> > $ ls -l /usr/portage/distfiles/ | wc -l >> > 47470 >> > >> > /usr/portage contains a lot of directories, /usr/portage/distfiles >> > contains lots of files. >> > >> > A bisect in fs/cifs gives: >> > $ git bisect bad >> > c4d3396b261473ded6f370edd1e79ba34e089d7e is the first bad commit >> > commit c4d3396b261473ded6f370edd1e79ba34e089d7e >> > Author: Jeff Layton <jlayton@...hat.com> >> > Date: Tue Jul 26 12:20:18 2011 -0400 >> > >> > cifs: advertise the right receive buffer size to the server >> > >> > Currently, we mirror the same size back to the server that it sends us. >> > That makes little sense. Instead we should be sending the server the >> > maximum buffer size that we can handle -- CIFSMaxBufSize minus the >> > 4 byte RFC1001 header. >> > >> > Signed-off-by: Jeff Layton <jlayton@...hat.com> >> > Signed-off-by: Steve French <sfrench@...ibm.com> >> > >> > :040000 040000 7a5015e0b47ca27538c4387a3b38470c86a42bae >> > c802cfda7d618f1073e45ce98efe65ce03cd0e79 M fs >> > >> > $ git bisect log >> > git bisect start '--' 'fs/cifs/' >> > # bad: [5f66d2b58ca879e70740c82422354144845d6dd3] Merge >> > git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6 >> > git bisect bad 5f66d2b58ca879e70740c82422354144845d6dd3 >> > # good: [02f8c6aee8df3cdc935e9bdd4f2d020306035dbe] Linux 3.0 >> > git bisect good 02f8c6aee8df3cdc935e9bdd4f2d020306035dbe >> > # good: [3ca30d40a91fb9b9871e61d5dea2c1a895906a15] CIFS: Fix oops >> > while mounting with prefixpath >> > git bisect good 3ca30d40a91fb9b9871e61d5dea2c1a895906a15 >> > # bad: [1f1cff0be05f59d5939edf28ff5ca0c6fd0a8e1c] cifs: trivial: goto >> > out here is unnecessary >> > git bisect bad 1f1cff0be05f59d5939edf28ff5ca0c6fd0a8e1c >> > # good: [e010a5ef95b8b6a12b74b548578f7dcf93564347] [CIFS] Redundant >> > null check after dereference >> > git bisect good e010a5ef95b8b6a12b74b548578f7dcf93564347 >> > # good: [1d87c28e680ce4ecb8c260d8ce070b8339d52abb] Merge >> > git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6 >> > git bisect good 1d87c28e680ce4ecb8c260d8ce070b8339d52abb >> > # bad: [c4d3396b261473ded6f370edd1e79ba34e089d7e] cifs: advertise the >> > right receive buffer size to the server >> > git bisect bad c4d3396b261473ded6f370edd1e79ba34e089d7e >> > >> > A git revert of c4d3396b261473ded6f370edd1e79ba34e089d7e seems to resolve it. >> > >> > If more info is needed, please let me know. >> >> Thanks for the bug report. According to the spec, I think I was a >> *little* off in the calculation but not by much. CIFSMaxBufSize doesn't >> include the size of the header, so the value we're sending is too small >> by 0x58 bytes. But, if anything though that should have led to the >> server sending smaller frames than we can handle, which should not >> cause this sort of problem. >> >> I tried to reproduce this on my test setup, but couldn't... >> >> Some questions... >> >> 1) did anything pop up in dmesg when this error occurred? >> >> 2) are you setting the CIFSMaxBufSize module parm to anything? >> >> 3) would it be possible to get debugging output? Instructions on how to >> do that are here: >> >> http://wiki.samba.org/index.php/LinuxCIFS_troubleshooting#Enabling_Debugging >> >> Thanks, > > Nevermind... I was able to reproduce it, and the following patch seems > to fix it for me. Jan, can you test this as well? If so, I'll > "officially" send it to Steve and we'll get this in ASAP. > > Long term, it would be better clean up the way CIFSMaxBufSize is > handled to get rid of this source of confusion... > > -----------------[snip]------------------- > > cifs: fix MaxBufferSize advertisement in session setup > > Commit c4d3396b2 failed to account for the fact that CIFSMaxBufSize does > not include the size of the header. This means that we're advertising a > size that's 0x58 bytes smaller than the client can handle. > > It's also problematic in the case of a large FIND_FIRST request. That > request will specify its own MaxDataCount value, which can turn out to > be larger than the originally provided MaxBufferSize. This causes the > server to break up the response into multiple pieces when it shouldn't > and throws off the checks in check2ndT2. > > Fix this by adding the amount of the header to the maxBufSize that we > advertise to the server. > > Reported-by: Jan Seiffert <kaffeemonster@...glemail.com> > Signed-off-by: Jeff Layton <jlayton@...hat.com> > --- > fs/cifs/sess.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c > index 243d587..c7d80e2 100644 > --- a/fs/cifs/sess.c > +++ b/fs/cifs/sess.c > @@ -124,8 +124,9 @@ static __u32 cifs_ssetup_hdr(struct cifs_ses *ses, SESSION_SETUP_ANDX *pSMB) > /* that we use in next few lines */ > /* Note that header is initialized to zero in header_assemble */ > pSMB->req.AndXCommand = 0xFF; > - pSMB->req.MaxBufferSize = cpu_to_le16(min_t(u32, CIFSMaxBufSize - 4, > - USHRT_MAX)); > + pSMB->req.MaxBufferSize = cpu_to_le16(min_t(u32, > + CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4, > + USHRT_MAX)); > pSMB->req.MaxMpxCount = cpu_to_le16(ses->server->maxReq); > pSMB->req.VcNumber = get_next_vcnum(ses); > > -- > 1.7.6 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@...r.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists