svn commit: r211684 - head/sys/kern
Brian Somers
brian at Awfulhak.org
Wed Aug 25 10:58:24 UTC 2010
On Wed, 25 Aug 2010 02:46:51 -0700 Brian Somers <brian at FreeBSD.org> wrote:
> On Mon, 23 Aug 2010 13:28:58 +0300 Kostik Belousov <kostikbel at 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
> > >
> > > 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.
> >
> > >
> > > Without this change, we run off the end of the directory data
> > > we're reading and panic horribly for nfs filesystems.
> > >
> > > MFC after: 1 week
> > >
> > > Modified:
> > > head/sys/kern/vfs_default.c
> > >
> > > Modified: head/sys/kern/vfs_default.c
> > > ==============================================================================
> > > --- 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);
> > >
> > > - *off = uio.uio_offset;
> > > -
> > > *cpos = dirbuf;
> > > - *len = (dirbuflen - uio.uio_resid);
> > > + *len = uio.uio_offset - *off;
> > > + *off = uio.uio_offset;
> > > }
> > >
> > > dp = (struct dirent *)(*cpos);
>
> 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 = -35080
(kgdb) p uio.uio_offset
$3 = 512
(kgdb) p uio.uio_resid
$4 = 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 = {d_fileno = 680038892, d_reclen = 40, d_type = 12 '\f', d_namlen = 34 '"',
d_name = "?\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((\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\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\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\222\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"...}
(kgdb) p *(struct dirent *)(dirbuf + 552)
$10 = {d_fileno = 536870933, d_reclen = 34528, d_type = 136 '\210', d_namlen = 40 '(',
d_name = "\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\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\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\222\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 = 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 trigger
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 == 0)" part 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);
- *cpos = dirbuf;
- *len = uio.uio_offset - *off;
*off = uio.uio_offset;
+
+ *cpos = dirbuf;
+ *len = (dirbuflen - uio.uio_resid);
+
+ if (*len == 0)
+ return (ENOENT);
}
dp = (struct dirent *)(*cpos);
--
Brian Somers <brian at Awfulhak.org>
Don't _EVER_ lose your sense of humour ! <brian at FreeBSD.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 306 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20100825/233b4d41/signature.pgp
More information about the svn-src-all
mailing list