Date: Mon, 7 Nov 2005 20:00:32 GMT From: "Devon O'Dell" <dodell@offmyserver.com> To: freebsd-amd64@FreeBSD.org Subject: Re: amd64/88249: getdents syscall fails for devfs on amd64 linuxalator Message-ID: <200511072000.jA7K0Whm032639@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR amd64/88249; it has been noted by GNATS.
From: "Devon O'Dell" <dodell@offmyserver.com>
To: freebsd-gnats-submit@FreeBSD.org,
"Arno J. Klaassen" <arno@heho.snv.jussieu.fr>
Cc: scottl@FreeBSD.org
Subject: Re: amd64/88249: getdents syscall fails for devfs on amd64 linuxalator
Date: Mon, 7 Nov 2005 11:53:38 -0800
--vtzGhvizbBRQ85DL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
After much discussion on this issue with scottl@, I've come up with the
following `quickfix' patch. While the previous patch does solve the
issue, it does so in the wrong manner: vfs_subr.c:vfs_read_dirent() is
not at fault here. The filesystem is expected to provide storage
space for ap->a_cookies when ap->a_ncookies is non-NULL.
In the case of the linuxulator, the linux_file.c:getdents_common() code
requires the use of cookies. It is the filesystem's job to ensure that,
when cookies are provided, space is allocated, as was previously
mentioned. devfs.c:devfs_readdir() does not do this, and
vfs_subr.c:vfs_read_dirent() expects this behavior.
The long discussion ended up implying several things:
a) There are bad things going on in each layer here:
i) linux_file.c:getdents_common has issues
ii) devfs.c:devfs_readdir() will need to support cookies at some
point
iii) vfs_subr.c:vfs_read_dirent() should be used as a generic
procedure with multiple filesystems instead of having them all
use various separate methods of allocating and determining
cookie storage requirements, which results in a good bit of
duplicated code.
b) The issue isn't limited to amd64, so the PR should be migrated to
kern/88249
c) The issue is somewhat severe, so a fix that doesn't address the
architectural problems (outlined in point a) should be committed
while these architectural issues are further discussed and
developed.
d) The issue probably isn't limited to linuxulator, but to any
filesystem that uses cookies and exports devfs. Thus, panics (or
hangs) will probably occur for devfs being exported over AFS or NFS.
The attached patch does two things:
a) If we are provided with cookie information in devfs, we currently
do not support this. This means we cannot export devfs over network
mounts, which I don't view as a problem (but would be a cool
feature).
b) Do sanity checking in vfs_subr.c:vfs_read_dirent() to panic
explicitly on the condition of ap->a_cookies being NULL with a
non-NULL ap->a_ncookies. Since the API is well-defined enough,
consumers should know to never subject the code to this condition.
I'm currently working on the architectural issues outlined above and
have been discussing them with scottl@ (extensively) and phk@ (briefly).
Kind regards,
Devon H. O'Dell
--vtzGhvizbBRQ85DL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="devfs_cookie_quickfix.patch"
diff -ur /sys/fs/devfs/devfs_vnops.c sys/fs/devfs/devfs_vnops.c
--- /sys/fs/devfs/devfs_vnops.c Mon Nov 7 11:47:53 2005
+++ sys/fs/devfs/devfs_vnops.c Mon Nov 7 10:53:09 2005
@@ -797,6 +797,7 @@
struct devfs_dirent *de;
struct devfs_mount *dmp;
off_t off, oldoff;
+ int *tmp_ncookies = NULL;
if (ap->a_vp->v_type != VDIR)
return (ENOTDIR);
@@ -805,6 +806,22 @@
if (uio->uio_offset < 0)
return (EINVAL);
+ /*
+ * XXX: This is a temporary hack to get around this filesystem not
+ * supporting cookies. We store the location of the ncookies pointer
+ * in a temporary variable before calling vfs_subr.c:vfs_read_dirent()
+ * and set the number of cookies to 0. We then set the pointer to
+ * NULL so that vfs_read_dirent doesn't try to call realloc() on
+ * ap->a_cookies. Later in this function, we restore the ap->a_ncookies
+ * pointer to its original location before returning to the caller.
+ *
+ */
+ if (ap->a_ncookies != NULL) {
+ tmp_ncookies = ap->a_ncookies;
+ *ap->a_ncookies = 0;
+ ap->a_ncookies = NULL;
+ }
+
dmp = VFSTODEVFS(ap->a_vp->v_mount);
sx_xlock(&dmp->dm_lock);
devfs_populate(dmp);
@@ -833,6 +850,14 @@
}
sx_xunlock(&dmp->dm_lock);
uio->uio_offset = off;
+
+ /*
+ * Restore ap->a_ncookies if it wasn't originally NULL in the first
+ * place.
+ */
+ if (tmp_ncookies != NULL)
+ ap->a_ncookies = tmp_ncookies;
+
return (error);
}
diff -ur /sys/kern/vfs_subr.c sys/kern/vfs_subr.c
--- /sys/kern/vfs_subr.c Mon Nov 7 11:47:53 2005
+++ sys/kern/vfs_subr.c Mon Nov 7 11:47:33 2005
@@ -3875,6 +3875,10 @@
}
if (ap->a_ncookies == NULL)
return (0);
+
+ KASSERT(ap->a_cookies,
+ ("NULL ap->a_cookies value with non-NULL ap->a_ncookies!"));
+
*ap->a_cookies = realloc(*ap->a_cookies,
(*ap->a_ncookies + 1) * sizeof(u_long), M_TEMP, M_WAITOK | M_ZERO);
(*ap->a_cookies)[*ap->a_ncookies] = off;
--vtzGhvizbBRQ85DL--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200511072000.jA7K0Whm032639>