amd64/88249: getdents syscall fails for devfs on amd64 linuxalator

[Devon O'Dell]

The following reply was made to PR amd64/88249; it has been noted by GNATS.

From: "Devon O'Dell" <dodell at offmyserver.com>
To: freebsd-gnats-submit at FreeBSD.org,
        "Arno J. Klaassen" <arno at heho.snv.jussieu.fr>
Cc: scottl at 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--