[PATCH 0/3] Incremental against [exports] rootdir patchset

[PATCH 0/3] Incremental against [exports] rootdir patchset

[Trond Myklebust]

These patches fix up a couple of bugs and issues against the [exports]
rootdir patchset for nfs-utils.

Trond Myklebust (3):
  mountd: Fix up incorrect comparison in next_mnt()
  mountd: Ensure nfsd_path_strip_root() uses the canonicalised path
  mountd: Canonicalise the rootdir in exportent_mkrealpath()

 support/export/export.c  | 12 ++++++++++--
 support/misc/nfsd_path.c | 17 +++++++++++++----
 utils/mountd/cache.c     |  2 +-
 3 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.21.0

[PATCH 1/3] mountd: Fix up incorrect comparison in next_mnt()

[Trond Myklebust]

We want to ensure that we compare the full name of the last component.

Reported-by: "J. Bruce Fields" <bfields@redhat.com>
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 utils/mountd/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d818c971bded..d616d526667e 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -400,7 +400,7 @@ static char *next_mnt(void **v, char *p)
 	while ((me = getmntent(f)) != NULL && l > 1) {
 		mnt_dir = nfsd_path_strip_root(me->mnt_dir);
 
-		if (strncmp(mnt_dir, p, l) == 0 && mnt_dir[l] != '/')
+		if (strncmp(mnt_dir, p, l) == 0 && mnt_dir[l] == '/')
 			return mnt_dir;
 	}
 	endmntent(f);
-- 
2.21.0

[PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

[Trond Myklebust]

When attempting to strip the root path, we should first canonicalise
the root pathname.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 support/misc/nfsd_path.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
index 2f41a793c534..9b38dd96007f 100644
--- a/support/misc/nfsd_path.c
+++ b/support/misc/nfsd_path.c
@@ -1,6 +1,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <limits.h>
 #include <stdlib.h>
 #include <unistd.h>
 
@@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
 char *
 nfsd_path_strip_root(char *pathname)
 {
+	char buffer[PATH_MAX];
 	const char *dir = nfsd_path_nfsd_rootdir();
 	char *ret;
 
-	ret = strstr(pathname, dir);
-	if (!ret || ret != pathname)
-		return pathname;
-	return pathname + strlen(dir);
+	if (!dir)
+		goto out;
+	if (realpath(dir, buffer)) {
+		ret = strstr(pathname, buffer);
+		if (ret == pathname)
+			return pathname + strlen(dir);
+	} else
+		xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
+				__func__, dir);
+out:
+	return pathname;
 }
 
 char *
-- 
2.21.0

[PATCH 3/3] mountd: Canonicalise the rootdir in exportent_mkrealpath()

[Trond Myklebust]

Ensure that we canonicalise the export path when generating the
real path.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 support/export/export.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/support/export/export.c b/support/export/export.c
index 82bbb54c5e9e..c753f68e4d63 100644
--- a/support/export/export.c
+++ b/support/export/export.c
@@ -14,6 +14,7 @@
 #include <sys/types.h>
 #include <sys/param.h>
 #include <netinet/in.h>
+#include <limits.h>
 #include <stdlib.h>
 #include <dirent.h>
 #include <errno.h>
@@ -21,6 +22,7 @@
 #include "nfslib.h"
 #include "exportfs.h"
 #include "nfsd_path.h"
+#include "xlog.h"
 
 exp_hash_table exportlist[MCL_MAXTYPES] = {{NULL, {{NULL,NULL}, }}, }; 
 static int export_hash(char *);
@@ -38,8 +40,14 @@ exportent_mkrealpath(struct exportent *eep)
 	const char *chroot = nfsd_path_nfsd_rootdir();
 	char *ret = NULL;
 
-	if (chroot)
-		ret = nfsd_path_prepend_dir(chroot, eep->e_path);
+	if (chroot) {
+		char buffer[PATH_MAX];
+		if (realpath(chroot, buffer))
+			ret = nfsd_path_prepend_dir(buffer, eep->e_path);
+		else
+			xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
+					__func__, chroot);
+	}
 	if (!ret)
 		ret = xstrdup(eep->e_path);
 	eep->e_realpath = ret;
-- 
2.21.0

Re: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

[J. Bruce Fields]

On Mon, Jun 03, 2019 at 01:12:26PM -0400, Trond Myklebust wrote:
> When attempting to strip the root path, we should first canonicalise
> the root pathname.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  support/misc/nfsd_path.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
> index 2f41a793c534..9b38dd96007f 100644
> --- a/support/misc/nfsd_path.c
> +++ b/support/misc/nfsd_path.c
> @@ -1,6 +1,7 @@
>  #include <errno.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#include <limits.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  
> @@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
>  char *
>  nfsd_path_strip_root(char *pathname)
>  {
> +	char buffer[PATH_MAX];
>  	const char *dir = nfsd_path_nfsd_rootdir();
>  	char *ret;
>  
> -	ret = strstr(pathname, dir);
> -	if (!ret || ret != pathname)
> -		return pathname;
> -	return pathname + strlen(dir);
> +	if (!dir)
> +		goto out;
> +	if (realpath(dir, buffer)) {
> +		ret = strstr(pathname, buffer);
> +		if (ret == pathname)
> +			return pathname + strlen(dir);
> +	} else
> +		xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
> +				__func__, dir);
> +out:
> +	return pathname;

I still don't get this.

So in the case strstr doesn't find anything, it returns the path
unchanged.

That means that if the next_mnt() caller asks whether there are any
mounts underneath /rootdir/a/b, and nextdir finds a mountpoint at
/a/b/c, it can return that, right?

--b.



>  }
>  
>  char *
> -- 
> 2.21.0

Re: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

[Trond Myklebust]

On Tue, 2019-06-04 at 11:46 -0400, J. Bruce Fields wrote:
> On Mon, Jun 03, 2019 at 01:12:26PM -0400, Trond Myklebust wrote:
> > When attempting to strip the root path, we should first
> > canonicalise
> > the root pathname.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  support/misc/nfsd_path.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
> > index 2f41a793c534..9b38dd96007f 100644
> > --- a/support/misc/nfsd_path.c
> > +++ b/support/misc/nfsd_path.c
> > @@ -1,6 +1,7 @@
> >  #include <errno.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> > +#include <limits.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> >  
> > @@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
> >  char *
> >  nfsd_path_strip_root(char *pathname)
> >  {
> > +	char buffer[PATH_MAX];
> >  	const char *dir = nfsd_path_nfsd_rootdir();
> >  	char *ret;
> >  
> > -	ret = strstr(pathname, dir);
> > -	if (!ret || ret != pathname)
> > -		return pathname;
> > -	return pathname + strlen(dir);
> > +	if (!dir)
> > +		goto out;
> > +	if (realpath(dir, buffer)) {
> > +		ret = strstr(pathname, buffer);
> > +		if (ret == pathname)
> > +			return pathname + strlen(dir);
> > +	} else
> > +		xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
> > +				__func__, dir);
> > +out:
> > +	return pathname;
> 
> I still don't get this.
> 
> So in the case strstr doesn't find anything, it returns the path
> unchanged.
> 
> That means that if the next_mnt() caller asks whether there are any
> mounts underneath /rootdir/a/b, and nextdir finds a mountpoint at
> /a/b/c, it can return that, right?
> 

Ack. Sending out a v2 of these patches.

Thanks Bruce!

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 2/3] mountd: Ensure nfsd_path_strip_root() uses the canonicalised path

[bfields]

On Tue, Jun 04, 2019 at 05:58:59PM +0000, Trond Myklebust wrote:
> On Tue, 2019-06-04 at 11:46 -0400, J. Bruce Fields wrote:
> > On Mon, Jun 03, 2019 at 01:12:26PM -0400, Trond Myklebust wrote:
> > > When attempting to strip the root path, we should first
> > > canonicalise
> > > the root pathname.
> > > 
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  support/misc/nfsd_path.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
> > > index 2f41a793c534..9b38dd96007f 100644
> > > --- a/support/misc/nfsd_path.c
> > > +++ b/support/misc/nfsd_path.c
> > > @@ -1,6 +1,7 @@
> > >  #include <errno.h>
> > >  #include <sys/types.h>
> > >  #include <sys/stat.h>
> > > +#include <limits.h>
> > >  #include <stdlib.h>
> > >  #include <unistd.h>
> > >  
> > > @@ -62,13 +63,21 @@ nfsd_path_nfsd_rootdir(void)
> > >  char *
> > >  nfsd_path_strip_root(char *pathname)
> > >  {
> > > +	char buffer[PATH_MAX];
> > >  	const char *dir = nfsd_path_nfsd_rootdir();
> > >  	char *ret;
> > >  
> > > -	ret = strstr(pathname, dir);
> > > -	if (!ret || ret != pathname)
> > > -		return pathname;
> > > -	return pathname + strlen(dir);
> > > +	if (!dir)
> > > +		goto out;
> > > +	if (realpath(dir, buffer)) {
> > > +		ret = strstr(pathname, buffer);
> > > +		if (ret == pathname)
> > > +			return pathname + strlen(dir);
> > > +	} else
> > > +		xlog(D_GENERAL, "%s: failed to resolve path %s: %m",
> > > +				__func__, dir);
> > > +out:
> > > +	return pathname;
> > 
> > I still don't get this.
> > 
> > So in the case strstr doesn't find anything, it returns the path
> > unchanged.
> > 
> > That means that if the next_mnt() caller asks whether there are any
> > mounts underneath /rootdir/a/b, and nextdir finds a mountpoint at
> > /a/b/c, it can return that, right?
> > 
> 
> Ack. Sending out a v2 of these patches.

Oh, good, thanks, I thought I was going crazy.

(Always a possibility, especially when I'm looking at code.)

--b.

Re: [PATCH 0/3] Incremental against [exports] rootdir patchset

[Steve Dickson]

On 6/3/19 1:12 PM, Trond Myklebust wrote:
> These patches fix up a couple of bugs and issues against the [exports]
> rootdir patchset for nfs-utils.
> 
> Trond Myklebust (3):
>   mountd: Fix up incorrect comparison in next_mnt()
>   mountd: Ensure nfsd_path_strip_root() uses the canonicalised path
>   mountd: Canonicalise the rootdir in exportent_mkrealpath()
> 
>  support/export/export.c  | 12 ++++++++++--
>  support/misc/nfsd_path.c | 17 +++++++++++++----
>  utils/mountd/cache.c     |  2 +-
>  3 files changed, 24 insertions(+), 7 deletions(-)
> 
Committed!

steved.