From owner-svn-src-all@FreeBSD.ORG Wed Aug 25 10:58:24 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 57C0C1065695; Wed, 25 Aug 2010 10:58:24 +0000 (UTC) (envelope-from prvs=1846a26b27=brian@Awfulhak.org) Received: from idcmail-mo2no.shaw.ca (idcmail-mo2no.shaw.ca [64.59.134.9]) by mx1.freebsd.org (Postfix) with ESMTP id E49A08FC08; Wed, 25 Aug 2010 10:58:23 +0000 (UTC) Received: from pd5ml1no-ssvc.prod.shaw.ca ([10.0.153.166]) by pd5mo1no-svcs.prod.shaw.ca with ESMTP; 25 Aug 2010 04:43:23 -0600 X-Cloudmark-SP-Filtered: true X-Cloudmark-SP-Result: v=1.1 cv=37qdrPIVUooonMxFi2BWZ8DhoCRe+hJcgJuumZcJ4K8= c=1 sm=1 a=ac2em_AgDcUA:10 a=VphdPIyG4kEA:10 a=MJPcHhXccCG8eBs0us8XwA==:17 a=6I5d2MoRAAAA:8 a=pGLkceISAAAA:8 a=MMwg4So0AAAA:8 a=Cf0GPqMsS1ijzaY0mdoA:9 a=EWXa41gkmEwbzuL6hUsA:7 a=34lhgKc9DzPs-LWaQYvXyiSyWBUA:4 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 a=MSl-tDqOz04A:10 a=WJ3hkfHDukgA:10 a=_j7Oz4fwN7ICFR0y:21 a=XY3BRwbVpKqtCihq:21 a=KSqKyckMdjsNzCdPUo0A:9 a=NJPTa544hkXawsaBHB8Pu1Jc7egA:4 a=HpAAvcLHHh0Zw7uRqdWCyQ==:117 Received: from unknown (HELO store.lan.Awfulhak.org) ([70.79.162.198]) by pd5ml1no-dmz.prod.shaw.ca with ESMTP; 25 Aug 2010 04:43:22 -0600 Received: from store.lan.Awfulhak.org (localhost.localdomain [127.0.0.1]) by localhost (Email Security Appliance) with SMTP id 96164C433AC_C74F363B; Wed, 25 Aug 2010 10:41:39 +0000 (GMT) Received: from gw.Awfulhak.org (gw.lan.Awfulhak.org [172.16.0.1]) by store.lan.Awfulhak.org (Sophos Email Appliance) with ESMTP id EA7FAC4612C_C74F35EF; Wed, 25 Aug 2010 10:41:34 +0000 (GMT) Received: from dev.lan.Awfulhak.org (brian@dev.lan.Awfulhak.org [172.16.0.5]) by gw.Awfulhak.org (8.14.4/8.14.4) with ESMTP id o7PAfYsj003778; Wed, 25 Aug 2010 03:41:34 -0700 (PDT) (envelope-from brian@Awfulhak.org) Date: Wed, 25 Aug 2010 03:41:27 -0700 From: Brian Somers To: Brian Somers Message-ID: <20100825034127.174d03ab@dev.lan.Awfulhak.org> In-Reply-To: <20100825024651.288b67b5@dev.lan.Awfulhak.org> References: <201008230533.o7N5XVxa028293@svn.freebsd.org> <20100823102858.GD2396@deviant.kiev.zoral.com.ua> <20100825024651.288b67b5@dev.lan.Awfulhak.org> X-Mailer: Claws Mail 3.7.6 (GTK+ 2.20.1; i386-portbld-freebsd9.0) Mime-Version: 1.0 Content-Type: multipart/signed; micalg=PGP-SHA1; boundary="Sig_/yekzVUPQiBL_wEZc.Zr+aq5"; protocol="application/pgp-signature" Cc: Kostik Belousov , svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r211684 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Aug 2010 10:58:24 -0000 --Sig_/yekzVUPQiBL_wEZc.Zr+aq5 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Wed, 25 Aug 2010 02:46:51 -0700 Brian Somers wrote: > On Mon, 23 Aug 2010 13:28:58 +0300 Kostik Belousov = wrote: > > On Mon, Aug 23, 2010 at 05:33:31AM +0000, Brian Somers wrote: > > > Author: brian > > > Date: Mon Aug 23 05:33:31 2010 > > > New Revision: 211684 > > > URL: http://svn.freebsd.org/changeset/base/211684 > > >=20 > > > Log: > > > uio_resid isn't updated by VOP_READDIR for nfs filesystems. Use > > > the uio_offset adjustment instead to calculate a correct *len. > > Isn't this should be fixed in nfs instead ? Please note that the moral > > equivalent of the code is also present in compat/linux/linux_cwd.c: > > linux_getcwd_scandir(). I did not inspected other callers of > > VOP_READDIR. > >=20 > > > =20 > > > Without this change, we run off the end of the directory data > > > we're reading and panic horribly for nfs filesystems. > > > =20 > > > MFC after: 1 week > > >=20 > > > Modified: > > > head/sys/kern/vfs_default.c > > >=20 > > > Modified: head/sys/kern/vfs_default.c > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > > --- head/sys/kern/vfs_default.c Mon Aug 23 05:33:20 2010 (r211683) > > > +++ head/sys/kern/vfs_default.c Mon Aug 23 05:33:31 2010 (r211684) > > > @@ -281,10 +281,9 @@ get_next_dirent(struct vnode *vp, struct > > > if (error) > > > return (error); > > > =20 > > > - *off =3D uio.uio_offset; > > > - > > > *cpos =3D dirbuf; > > > - *len =3D (dirbuflen - uio.uio_resid); > > > + *len =3D uio.uio_offset - *off; > > > + *off =3D uio.uio_offset; > > > } > > > =20 > > > dp =3D (struct dirent *)(*cpos); >=20 > I'm looking into why uio_resid isn't being updated - it's a bit awkward > as this is happening on a production box running 8.1 (just upgraded > from 7), so it may take a few days. Hmm, I've just seen a crash here with the new code.... (kgdb) p *len $2 =3D -35080 (kgdb) p uio.uio_offset $3 =3D 512 (kgdb) p uio.uio_resid $4 =3D 8192 So it looks like my fix is wrong (so much for real-world tests!). Note, the uio values above have survived on the stack from the first call to get_next_dirent() where *len was zero and we read the content of the directory. The directory file is 512 bytes and the crash happens when we run off the end of the 512 bytes read. (kgdb) p *(struct dirent *)(dirbuf + 512) $9 =3D {d_fileno =3D 680038892, d_reclen =3D 40, d_type =3D 12 '\f', d_naml= en =3D 34 '"',=20 d_name =3D "?\206\210(\000\000\000\000\025\000\000 ?\206\210(\004\222\210= ((\000\f\"?\206\210(\000\000\000\000\025\000\000 ?\206\210(\034\222\210((\0= 00\f\"?\206\210(\000\000\000\000\025\000\000 ?\206\210(4\222\210((\000\f\"?= \206\210(\000\000\000\000\025\000\000 \000\207\210(L\222\210((\000\f\"\000\= 207\210(\000\000\000\000\025\000\000 \020\207\210(d\222\210((\000\f\"\020\2= 07\210(\000\000\000\000\025\000\000 \207\210(|\222\210((\000\f\" \207\210(= \000\000\000\000\025\000\000 0\207\210(\224\222\210((\000\f\"0\207\210(\000= \000\000\000\025\000\000 @\207\210(?\222\210((\000\f\"@\207\210(\000\000\00= 0\000"...} (kgdb) p *(struct dirent *)(dirbuf + 552) $10 =3D {d_fileno =3D 536870933, d_reclen =3D 34528, d_type =3D 136 '\210',= d_namlen =3D 40 '(',=20 d_name =3D "\034\222\210((\000\f\"?\206\210(\000\000\000\000\025\000\000 = ?\206\210(4\222\210((\000\f\"?\206\210(\000\000\000\000\025\000\000 \000\20= 7\210(L\222\210((\000\f\"\000\207\210(\000\000\000\000\025\000\000 \020\207= \210(d\222\210((\000\f\"\020\207\210(\000\000\000\000\025\000\000 \207\210= (|\222\210((\000\f\" \207\210(\000\000\000\000\025\000\000 0\207\210(\224\2= 22\210((\000\f\"0\207\210(\000\000\000\000\025\000\000 @\207\210(?\222\210(= (\000\f\"@\207\210(\000\000\000\000\025\000\000 P\207\210(?\222\210((\000\f= \"P\207\210(\000\000\000\000\025\000\000 `\207\210(?\222\210((\000\f\""...} (kgdb) p dirbuf + 552 + 34528 $11 =3D 0xc8891908
So we can see that we're falling off the end of the 512 bytes we read because *len never reaches zero as it's intended to. The bit I can't understand yet is how len reaches -35080. That number is minus the sum of all the directory reclens and the two garbage dirents that we read from dirbuf + 512 and dirbuf + 552. This seems to imply that when we actually did the readdir(), we set *len to zero and then immediately adjusted it before returning from get_next_dirent(). This is exactly the deduction that made me make the original change. Having said all that, I also have diagnostics in this kernel that should tr= igger if nfs_readdir() tries to return with inconsistent uio_offset and uio_resid values and I saw no diagnostics. This means that assuming I haven't fat fingered the diagnostics, I was just absolutely wrong about uio_resid not being adjusted. Hmm, so after all this, I know what the problem is: The first read of 512 bytes works fine and we iterate through the dirents 'till we hit the end. We then read again and get zero from readdir, but fail to handle this case. We then drop out of the "if (*len =3D=3D 0)" par= t and start using garbage at dirbuf + 512, adjusting *len to -40. I'd suggest this patch, do you agree? I'll test it tomorrow... --- vfs_default.c.orig 2010-08-22 21:35:18.000000000 -0700 +++ vfs_default.c 2010-08-25 03:38:41.000000000 -0700 @@ -280,9 +280,13 @@ if (error) return (error); =20 - *cpos =3D dirbuf; - *len =3D uio.uio_offset - *off; *off =3D uio.uio_offset; + + *cpos =3D dirbuf; + *len =3D (dirbuflen - uio.uio_resid); + + if (*len =3D=3D 0) + return (ENOENT); } =20 dp =3D (struct dirent *)(*cpos); --=20 Brian Somers Don't _EVER_ lose your sense of humour ! --Sig_/yekzVUPQiBL_wEZc.Zr+aq5 Content-Type: application/pgp-signature; name=signature.asc Content-Disposition: attachment; filename=signature.asc -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (FreeBSD) iQCVAwUBTHTzXg7tvOdmanQhAQIHDgP/XDdtOWjhu6iJcS5mmF1/QD+9bkTWXqsi /Tp902/8accI3ah353lLt6cFzTZ+pEykkVU8CZnublgH9C7InzkV8k6iwwILSaUv dHIdU+21GG060XWNsnxW4LhTp9mhtiv37GDn9JOQkoeSU6BZf1c4IgsGGV0+1zsc db41vAOFe68= =NXjx -----END PGP SIGNATURE----- --Sig_/yekzVUPQiBL_wEZc.Zr+aq5--