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

[Sergey Kandaurov]

On 1 March 2013 04:58, Rick Macklem <rmacklem at uoguelph.ca> wrote:
> 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
>

Hi Rick
Sorry but I am no longer able to test NFSv4.

-- 
wbr,
pluknet