mountd: Possible bug in next_mnt()

mountd: Possible bug in next_mnt()

[Richard Weinberger]

Hi!

next_mnt() finds submounts below a given path p.
While investigating into an issue in my crossmount patches for nfs-utils I noticed
that it does not work when fsid=root, rootdir=/some/path/ and then "/" is being exported.
In this case next_mnt() is asked to find submounts of "/" but returns none.

In my opinion this wrong because every mount is a submount of "/".

The following change fixes the problem on my side but I'm not sure whether
"/" is a special case in mountd where next_mnt() has to bail out.

diff --git a/support/export/cache.c b/support/export/cache.c
index 2497d4f48df3..be20cb34adcb 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -410,13 +410,13 @@ static char *next_mnt(void **v, char *p)
                *v = f;
        } else
                f = *v;
-       while ((me = getmntent(f)) != NULL && l > 1) {
+       while ((me = getmntent(f)) != NULL && l >= 1) {
                char *mnt_dir = nfsd_path_strip_root(me->mnt_dir);
 
                if (!mnt_dir)
                        continue;
 
-               if (strncmp(mnt_dir, p, l) == 0 && mnt_dir[l] == '/')
+               if (strncmp(mnt_dir, p, l) == 0 && (l == 1 || mnt_dir[l] == '/'))
                        return mnt_dir;
        }
        endmntent(f);

Comments? :-)

Thanks,
//richard

Re: mountd: Possible bug in next_mnt()

[Steve Dickson]

Hello,

My apologies... Eating some PTO...

On 3/8/23 10:05 AM, Richard Weinberger wrote:
> Hi!
> 
> next_mnt() finds submounts below a given path p.
> While investigating into an issue in my crossmount patches for nfs-utils I noticed
> that it does not work when fsid=root, rootdir=/some/path/ and then "/" is being exported.
> In this case next_mnt() is asked to find submounts of "/" but returns none.
I'm not clear as what you are saying... "rootdir=/some/path/" is not an
export option.

> 
> In my opinion this wrong because every mount is a submount of "/".
> 
> The following change fixes the problem on my side but I'm not sure whether
> "/" is a special case in mountd where next_mnt() has to bail out.
> 
> diff --git a/support/export/cache.c b/support/export/cache.c
> index 2497d4f48df3..be20cb34adcb 100644
> --- a/support/export/cache.c
> +++ b/support/export/cache.c
> @@ -410,13 +410,13 @@ static char *next_mnt(void **v, char *p)
>                  *v = f;
>          } else
>                  f = *v;
> -       while ((me = getmntent(f)) != NULL && l > 1) {
> +       while ((me = getmntent(f)) != NULL && l >= 1) {
>                  char *mnt_dir = nfsd_path_strip_root(me->mnt_dir);
>   
>                  if (!mnt_dir)
>                          continue;
>   
> -               if (strncmp(mnt_dir, p, l) == 0 && mnt_dir[l] == '/')
> +               if (strncmp(mnt_dir, p, l) == 0 && (l == 1 || mnt_dir[l] == '/'))
>                          return mnt_dir;
>          }
>          endmntent(f);
> 
> Comments? :-)
Putting this is the correct patch format including a Signed-off-by
signature... would help.

steved.
> 
> Thanks,
> //richard
> 

Re: mountd: Possible bug in next_mnt()

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> next_mnt() finds submounts below a given path p.
>> While investigating into an issue in my crossmount patches for nfs-utils I
>> noticed
>> that it does not work when fsid=root, rootdir=/some/path/ and then "/" is being
>> exported.
>> In this case next_mnt() is asked to find submounts of "/" but returns none.
> I'm not clear as what you are saying... "rootdir=/some/path/" is not an
> export option.

Sorry for being imprecise.
rootdir= is an nfs.conf exports option.
 
Thanks,
//richard

Re: mountd: Possible bug in next_mnt()

[Steve Dickson]

On 3/11/23 11:52 AM, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>>> next_mnt() finds submounts below a given path p.
>>> While investigating into an issue in my crossmount patches for nfs-utils I
>>> noticed
>>> that it does not work when fsid=root, rootdir=/some/path/ and then "/" is being
>>> exported.
>>> In this case next_mnt() is asked to find submounts of "/" but returns none.
>> I'm not clear as what you are saying... "rootdir=/some/path/" is not an
>> export option.
> 
> Sorry for being imprecise.
> rootdir= is an nfs.conf exports option.Point. But I still need the patch in the correct
format with the Signed-off-by...

steved.

>   
> Thanks,
> //richard
> 

Re: mountd: Possible bug in next_mnt()

[Richard Weinberger]

----- Ursprüngliche Mail -----
> On 3/11/23 11:52 AM, Richard Weinberger wrote:
>> ----- Ursprüngliche Mail -----
>>>> next_mnt() finds submounts below a given path p.
>>>> While investigating into an issue in my crossmount patches for nfs-utils I
>>>> noticed
>>>> that it does not work when fsid=root, rootdir=/some/path/ and then "/" is being
>>>> exported.
>>>> In this case next_mnt() is asked to find submounts of "/" but returns none.
>>> I'm not clear as what you are saying... "rootdir=/some/path/" is not an
>>> export option.
>> 
>> Sorry for being imprecise.
>> rootdir= is an nfs.conf exports option.Point. But I still need the patch in the
>> correct
> format with the Signed-off-by...

Well, the goal of my mail was not sending a ready-to-apply patch.
It was a question. To me next_mnt() looks wrong but I'm not sure whether
the current handling of "/" is desired for some special case I'm not aware of.

I'll happily send a patch after we agree that next_mnt() is wrong.

Thanks,
//richard

Re: mountd: Possible bug in next_mnt()

[Steve Dickson]

On 3/12/23 9:36 AM, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> On 3/11/23 11:52 AM, Richard Weinberger wrote:
>>> ----- Ursprüngliche Mail -----
>>>>> next_mnt() finds submounts below a given path p.
>>>>> While investigating into an issue in my crossmount patches for nfs-utils I
>>>>> noticed
>>>>> that it does not work when fsid=root, rootdir=/some/path/ and then "/" is being
>>>>> exported.
>>>>> In this case next_mnt() is asked to find submounts of "/" but returns none.
>>>> I'm not clear as what you are saying... "rootdir=/some/path/" is not an
>>>> export option.
>>>
>>> Sorry for being imprecise.
>>> rootdir= is an nfs.conf exports option.Point. But I still need the patch in the
>>> correct
>> format with the Signed-off-by...
> 
> Well, the goal of my mail was not sending a ready-to-apply patch.
> It was a question. To me next_mnt() looks wrong but I'm not sure whether
> the current handling of "/" is desired for some special case I'm not aware of.
> 
> I'll happily send a patch after we agree that next_mnt() is wrong.
I'm still trying to reproduce problem... I have

/etc/nfs.conf: rootdir=/export

/etc/exports:
/home *(rw,sec=sys:krb5:krb5i:krb5p)
/tmp *(rw,fsid=666,all_squash)
/ *(rw,fsid=root,all_squash)

I'm not seeing the problem... Where does the crossmount come in?

steved.

Re: mountd: Possible bug in next_mnt()

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> Well, the goal of my mail was not sending a ready-to-apply patch.
>> It was a question. To me next_mnt() looks wrong but I'm not sure whether
>> the current handling of "/" is desired for some special case I'm not aware of.
>> 
>> I'll happily send a patch after we agree that next_mnt() is wrong.
> I'm still trying to reproduce problem... I have
> 
> /etc/nfs.conf: rootdir=/export
> 
> /etc/exports:
> /home *(rw,sec=sys:krb5:krb5i:krb5p)
> /tmp *(rw,fsid=666,all_squash)
> / *(rw,fsid=root,all_squash)
> 
> I'm not seeing the problem... Where does the crossmount come in?

Chris reported the problem to me while he was testing my re-export/crossmount patches.
I can try reproducing without re-exporting later.

In theory you should see the problem as follows:

1. Have rootdir=/export in your nfs.conf
2. /export is some filesystem that contains more mounts
3. /export/fs1 is a different filesytem
4. /export/fs2 is a different filesytem too
5. /etc/exports contains: / *(rw,fsid=root,all_squash,crossmount)

Client mounts / to /nfs and then tries to access /nfs/fs1.
Then nfsd_fh() iterates over all exports, finds one with NFSEXP_CROSSMOUNT set.
Using next_mnt() it finds possible sub-mounts. But for the "/" case next_mnt()
returns none -> nfsd_fh() fails -> client cannot enter /nfs/fs1.

Thanks,
//richard

Re: mountd: Possible bug in next_mnt()

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> I'm still trying to reproduce problem... I have
>> 
>> /etc/nfs.conf: rootdir=/export
>> 
>> /etc/exports:
>> /home *(rw,sec=sys:krb5:krb5i:krb5p)
>> /tmp *(rw,fsid=666,all_squash)
>> / *(rw,fsid=root,all_squash)
>> 
>> I'm not seeing the problem... Where does the crossmount come in?
> 
> Chris reported the problem to me while he was testing my re-export/crossmount
> patches.
> I can try reproducing without re-exporting later.

Finally I found some cycles to reproduce without my re-exporting setup.

1. /etc/exports contains:
/       *(rw,crossmnt,no_subtree_check,fsid=root)

2. /etc/nfs.conf contains:

[exports]
 rootdir=/nfs_srv
 
3. Mounts:

/root/fs1.ext4 on /nfs_srv type ext4 (rw,relatime)
/root/fs2.ext4 on /nfs_srv/fs2 type ext4 (rw,relatime)

4. On the client:

# ls /nfs_client/fs2
ls: cannot open directory '/nfs_client/fs2': Stale file handle

I'll send a proper patch ASAP.
next_mnt() has to deal with "/" too.

Thanks,
//richard