From owner-freebsd-fs@FreeBSD.ORG Thu Feb 28 15:28:10 2013 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 49CD088A; Thu, 28 Feb 2013 15:28:10 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-annu.net.uoguelph.ca (esa-annu.mail.uoguelph.ca [131.104.91.36]) by mx1.freebsd.org (Postfix) with ESMTP id DF1EEB6A; Thu, 28 Feb 2013 15:28:09 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqEEADd1L1GDaFvO/2dsb2JhbABFhk+5AYJcgRBzgh8BAQQBIwRSBRYOCgICDRkCWQaIIAavWJIXgSOMKoETNAeCLYETA4hqjVeJY4cHgyaBSz4 X-IronPort-AV: E=Sophos;i="4.84,755,1355115600"; d="scan'208";a="16276035" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-annu.net.uoguelph.ca with ESMTP; 28 Feb 2013 10:28:03 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 15CCAB3F18; Thu, 28 Feb 2013 10:28:03 -0500 (EST) Date: Thu, 28 Feb 2013 10:28:03 -0500 (EST) From: Rick Macklem To: Konstantin Belousov Message-ID: <664298325.3403590.1362065283063.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130228070515.GK2454@kib.kiev.ua> Subject: Re: should vn_fullpath1() ever return a path with "." in it? MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.202] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) Cc: FreeBSD Filesystems , Sergey Kandaurov X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Feb 2013 15:28:10 -0000 Konstantin Belousov wrote: > On Wed, Feb 27, 2013 at 09:59:22PM -0500, Rick Macklem wrote: > > Hi, > > > > Sergey Kandaurov reported a problem where getcwd() returns a > > path with "/./" imbedded in it for an NFSv4 mount. This is > > caused by a mount point crossing on the server when at the > > server's root because vn_fullpath1() uses VV_ROOT to spot > > mount point crossings. > > > > The current workaround is to use the sysctls: > > debug.disablegetcwd=1 > > debug.disablefullpath=1 > > > > However, it would be nice to fix this when vn_fullpath1() > > is being used. > > > > A simple fix is to have vn_fullpath1() fail when it finds > > "." as a directory match in the path. When vn_fullpath1() > > fails, the syscalls fail and that allows the libc algorithm > > to be used (which works for this case because it doesn't > > depend on VV_ROOT being set, etc). > > > > So, I am wondering if a patch (I have attached one) that > > makes vn_fullpath1() fail when it matches "." will break > > anything else? (I don't think so, since the code checks > > for VV_ROOT in the loop above the check for a match of > > ".", but I am not sure?) > > > > Thanks for any input w.r.t. this, rick > > > --- kern/vfs_cache.c.sav 2013-02-27 20:44:42.000000000 -0500 > > +++ kern/vfs_cache.c 2013-02-27 21:10:39.000000000 -0500 > > @@ -1333,6 +1333,20 @@ vn_fullpath1(struct thread *td, struct v > > startvp, NULL, 0, 0); > > break; > > } > > + if (buf[buflen] == '.' && (buf[buflen + 1] == '\0' || > > + buf[buflen + 1] == '/')) { > > + /* > > + * Fail if it matched ".". This should only happen > > + * for NFSv4 mounts that cross server mount points. > > + */ > > + CACHE_RUNLOCK(); > > + vrele(vp); > > + numfullpathfail1++; > > + error = ENOENT; > > + SDT_PROBE(vfs, namecache, fullpath, return, > > + error, vp, NULL, 0, 0); > > + break; > > + } > > buf[--buflen] = '/'; > > slash_prefixed = 1; > > } > > I do not quite understand this. Did the dvp (parent) vnode returned by > VOP_VPTOCNP() equal to vp (child) vnode in the case of the "." name ? Well, the vnodes aren't the same, but the fileid (think NFS i-node#) is the value for "." and ".." (2 for a UFS exported fs). The vnodes are based on the file handles and dvp will be for the mount point in the other file system on the server. NFSv4 has 2 attributes for a server mount point directory: fileid - which is the fileid# for the root (2 for UFS) mounted_on_fileid - which is the fileid of the directory in the parent file system The parent file system has a different fsid, which becomes the st_dev and, as such, the userland algorithm in getcwd() works. The case I test is where the server mount point is one directory level below the local mount point in the client. For example: /mnt is the local mount point and /mnt/sub1 is a server mount point (different file system than /mnt). - when vn_fullpath1() gets up to /mnt/sub1 (which doesn't have VV_ROOT set on it), vn_vptocnp_locked() matches "." for the fileno. I think there is code in vn_vptocnp_locked() that avoids a match for ".." or that could match too. - then it does /mnt, which does have VV_ROOT set and it works. > It must be, for the correct operation, but also it should cause the > almost > infinite loop in the vn_fullpath1(). The loop is not really infinite > due > to a limited size of the buffer where the infinite amount of "./" is > placed. > As noted above, I think this loop is avoided by dvp != vp. Within the NFSv4 mount, there can be multiple instances of a fileid (st_ino), but the have different fsids (st_dev) and different vnodes. > Anyway, I think we should do better than this patch, even if it is > legitimate. I think that the better place to check the condition is > the > default implementation of VOP_VPTOCNP(). Am I right that this is where > it broke for you ? > Yep. I wasn't sure what the implications of putting the fix further down were. (I was planning to ask if the patch should go in a lower level function, but forgot to ask;-) I'll test this patch and let you know if it works. Thanks, rick > diff --git a/sys/kern/vfs_default.c b/sys/kern/vfs_default.c > index 00d064e..1dd0185 100644 > --- a/sys/kern/vfs_default.c > +++ b/sys/kern/vfs_default.c > @@ -856,8 +856,12 @@ vop_stdvptocnp(struct vop_vptocnp_args *ap) > error = ENOMEM; > goto out; > } > - bcopy(dp->d_name, buf + i, dp->d_namlen); > - error = 0; > + if (dp->d_namlen == 1 && dp->d_name[0] == '.') { > + error = ENOENT; > + } else { > + bcopy(dp->d_name, buf + i, dp->d_namlen); > + error = 0; > + } > goto out; > } > } while (len > 0 || !eofflag);