Date: Thu, 28 Feb 2013 10:28:03 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Konstantin Belousov <kostikbel@gmail.com> Cc: FreeBSD Filesystems <freebsd-fs@freebsd.org>, Sergey Kandaurov <pluknet@freebsd.org> Subject: Re: should vn_fullpath1() ever return a path with "." in it? Message-ID: <664298325.3403590.1362065283063.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130228070515.GK2454@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?664298325.3403590.1362065283063.JavaMail.root>