Date: Mon, 14 Apr 2014 15:27:58 -0700 From: Kirk McKusick <mckusick@mckusick.com> To: freebsd-fs@freebsd.org Subject: Re: getdirentries cookies usage outside of UFS Message-ID: <201404142227.s3EMRwIL080960@chez.mckusick.com> In-Reply-To: <53489F0D.1020702@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> Date: Fri, 11 Apr 2014 21:03:57 -0500 > From: Bryan Drewery <bdrewery@freebsd.org> > To: freebsd-fs@freebsd.org > Subject: getdirentries cookies usage outside of UFS > > Recently I was working on a compat syscall for sys_getdirentries() that > converts between our dirent and the FreeBSD dirent struct. We had never > tried using this on TMPFS and when we did ran into weird issues (hence > my recent commits to TMPFS to clarify some of the getdirentries() code). > We were not using cookies, so I referenced the Linux compat module > (linux_file.c getdents_common()) > > I ran across this code: > >> /* >> * When using cookies, the vfs has the option of reading from >> * a different offset than that supplied (UFS truncates the >> * offset to a block boundary to make sure that it never reads >> * partway through a directory entry, even if the directory >> * has been compacted). >> */ >> while (len > 0 && ncookies > 0 && *cookiep <= off) { >> bdp = (struct dirent *) inp; >> len -= bdp->d_reclen; >> inp += bdp->d_reclen; >> cookiep++; >> ncookies--; >> }=20 > > > At first it looked innocuous but then it occurred to me it was the root > of the issue I was having as it was eating my cookies based on their > value, despite tmpfs cookies being random hash values that have no > sequential relation. So I looked at how NFS was handling the same code > and found this lovely hack from r216691: > >> not_zfs = strcmp(vp->v_mount->mnt_vfc->vfc_name, "zfs"); > =2E.. >> while (cpos < cend && ncookies > 0 && >> (dp->d_fileno == 0 || dp->d_type == DT_WHT || >> (not_zfs != 0 && ((u_quad_t)(*cookiep)) <= toff))) { >> cpos += dp->d_reclen; >> dp = (struct dirent *)cpos; >> cookiep++; >> ncookies--; >> } > > I ended up doing the opposite, only running the code if getting dirents > from "ufs". > > So there's multiple issue here. > > 1. NFS is broken on TMPFS. I can see why it's gone so long unnoticed, > why would you do that? Still probably worth fixing. > > 2. Linux and SVR4 getdirentries() are both broken on TMPFS/ZFS. I am > surprised Linux+ZFS has not been noticed by now. I am aware the SVR4 is > full of other bugs too. I ran across many just reviewing the > getdirentries code alone. > > Do any other file systems besides UFS do this offset/cookie > truncation/rewind? If UFS is the only one it may be acceptable to change > this zfs check to !ufs and add it to the other modules. If we don't like > that, or there are potentially other file systems doing this too, how > about adding a flag to somewhere to indicate the file system has > monotonically increasing offsets and needs this rewind support. I'm not > sure where that is best done, struct vfsconf? > > Regards, > Bryan Drewery This code is specific to UFS. I concur with your fix of making it conditionl on UFS. I feel guilty for putting that code in unconditionally in the first place. In my defense it was 1982 and UFS was the only filesystem :-) Kirk McKusick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201404142227.s3EMRwIL080960>