Date: Wed, 25 Aug 2010 03:41:27 -0700 From: Brian Somers <brian@Awfulhak.org> To: Brian Somers <brian@FreeBSD.org> Cc: Kostik Belousov <kostikbel@gmail.com>, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r211684 - head/sys/kern 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <brian@FreeBSD.org> wrote: > On Mon, 23 Aug 2010 13:28:58 +0300 Kostik Belousov <kostikbel@gmail.com> = 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 <Address 0xc8891908 out of bounds> 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 <brian@Awfulhak.org> Don't _EVER_ lose your sense of humour ! <brian@FreeBSD.org> --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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100825034127.174d03ab>