should vn_fullpath1() ever return a path with "." in it?

[Rick Macklem]

Kostik 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 ?
> 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.
> 
> 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 ?
> 
> 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);

Yes, this patch fixes the problem too. If you think it is safe to
do this, I can commit the patch in mid-April. Maybe Sergey can
test it?

Thanks yet again, rick