[PATCH 0/7] nfs-utils fixes

[PATCH 0/7] nfs-utils fixes

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

This patchset fixes a couple of missed API changes in mountd to
ensure that the [exports]rootdir root jail works correctly. It
fixes up the 'same_path' function, as well as 'uuid_by_path'.
It also improves the error handling, and tries to distinguish
between bona fide path resolution problems, and other transient
issues in order to avoid having knfsd return spurious ESTALE
errors.

Trond Myklebust (7):
  mountd: Add a helper nfsd_path_statfs64() for uuid_by_path()
  nfsd: Support running nfsd_name_to_handle_at() in the root jail
  mountd: Fix up path checking helper same_path()
  Fix autoconf probe for 'struct nfs_filehandle'
  mountd: Ensure dump_to_cache() sets errno appropriately
  mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh()
  mountd: Check the stat() return values in match_fsid()

 configure.ac                |   7 +-
 support/include/nfsd_path.h |   9 ++
 support/misc/nfsd_path.c    | 109 ++++++++++++++++++++++
 utils/mountd/cache.c        | 174 ++++++++++++++++++++++++------------
 4 files changed, 242 insertions(+), 57 deletions(-)

-- 
2.25.2

[PATCH 1/7] mountd: Add a helper nfsd_path_statfs64() for uuid_by_path()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Ensure uuid_by_path() works correctly when 'rootdir'
is set in the [exports] section of nfs.conf.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 support/include/nfsd_path.h |  5 +++++
 support/misc/nfsd_path.c    | 43 +++++++++++++++++++++++++++++++++++++
 utils/mountd/cache.c        |  2 +-
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/support/include/nfsd_path.h b/support/include/nfsd_path.h
index b42416bbff58..8331ff96a277 100644
--- a/support/include/nfsd_path.h
+++ b/support/include/nfsd_path.h
@@ -6,6 +6,8 @@
 
 #include <sys/stat.h>
 
+struct statfs64;
+
 void 		nfsd_path_init(void);
 
 const char *	nfsd_path_nfsd_rootdir(void);
@@ -15,6 +17,9 @@ char *		nfsd_path_prepend_dir(const char *dir, const char *pathname);
 int 		nfsd_path_stat(const char *pathname, struct stat *statbuf);
 int 		nfsd_path_lstat(const char *pathname, struct stat *statbuf);
 
+int		nfsd_path_statfs64(const char *pathname,
+				   struct statfs64 *statbuf);
+
 char *		nfsd_realpath(const char *path, char *resolved_path);
 
 ssize_t		nfsd_path_read(int fd, char *buf, size_t len);
diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
index f078a668fb8f..ab6c98dbe395 100644
--- a/support/misc/nfsd_path.c
+++ b/support/misc/nfsd_path.c
@@ -5,6 +5,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/vfs.h>
 #include <limits.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -180,6 +181,48 @@ nfsd_path_lstat(const char *pathname, struct stat *statbuf)
 	return nfsd_run_stat(nfsd_wq, nfsd_lstatfunc, pathname, statbuf);
 }
 
+struct nfsd_statfs64_data {
+	const char *pathname;
+	struct statfs64 *statbuf;
+	int ret;
+	int err;
+};
+
+static void
+nfsd_statfs64func(void *data)
+{
+	struct nfsd_statfs64_data *d = data;
+
+	d->ret = statfs64(d->pathname, d->statbuf);
+	if (d->ret < 0)
+		d->err = errno;
+}
+
+static int
+nfsd_run_statfs64(struct xthread_workqueue *wq,
+		  const char *pathname,
+		  struct statfs64 *statbuf)
+{
+	struct nfsd_statfs64_data data = {
+		pathname,
+		statbuf,
+		0,
+		0
+	};
+	xthread_work_run_sync(wq, nfsd_statfs64func, &data);
+	if (data.ret < 0)
+		errno = data.err;
+	return data.ret;
+}
+
+int
+nfsd_path_statfs64(const char *pathname, struct statfs64 *statbuf)
+{
+	if (!nfsd_wq)
+		return statfs64(pathname, statbuf);
+	return nfsd_run_statfs64(nfsd_wq, pathname, statbuf);
+}
+
 struct nfsd_realpath_data {
 	const char *pathname;
 	char *resolved;
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 8f54e37b7936..7d8657c91323 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -352,7 +352,7 @@ static int uuid_by_path(char *path, int type, size_t uuidlen, char *uuid)
 	const char *val;
 	int rc;
 
-	rc = statfs64(path, &st);
+	rc = nfsd_path_statfs64(path, &st);
 
 	if (type == 0 && rc == 0) {
 		const unsigned long *bad;
-- 
2.25.2

[PATCH 2/7] nfsd: Support running nfsd_name_to_handle_at() in the root jail

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

When running nfsd_name_to_handle_at(), we usually want to see the same
namespace as knfsd, so add helpers.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 support/include/nfsd_path.h |  4 +++
 support/misc/nfsd_path.c    | 66 +++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/support/include/nfsd_path.h b/support/include/nfsd_path.h
index 8331ff96a277..3b73aadd8af7 100644
--- a/support/include/nfsd_path.h
+++ b/support/include/nfsd_path.h
@@ -6,6 +6,7 @@
 
 #include <sys/stat.h>
 
+struct file_handle;
 struct statfs64;
 
 void 		nfsd_path_init(void);
@@ -25,4 +26,7 @@ char *		nfsd_realpath(const char *path, char *resolved_path);
 ssize_t		nfsd_path_read(int fd, char *buf, size_t len);
 ssize_t		nfsd_path_write(int fd, const char *buf, size_t len);
 
+int		nfsd_name_to_handle_at(int fd, const char *path,
+				       struct file_handle *fh,
+				       int *mount_id, int flags);
 #endif
diff --git a/support/misc/nfsd_path.c b/support/misc/nfsd_path.c
index ab6c98dbe395..1f6dfd4b642b 100644
--- a/support/misc/nfsd_path.c
+++ b/support/misc/nfsd_path.c
@@ -6,6 +6,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/vfs.h>
+#include <fcntl.h>
 #include <limits.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -14,6 +15,7 @@
 #include "xmalloc.h"
 #include "xlog.h"
 #include "xstat.h"
+#include "nfslib.h"
 #include "nfsd_path.h"
 #include "workqueue.h"
 
@@ -340,3 +342,67 @@ nfsd_path_write(int fd, const char *buf, size_t len)
 		return write(fd, buf, len);
 	return nfsd_run_write(nfsd_wq, fd, buf, len);
 }
+
+#if defined(HAVE_NAME_TO_HANDLE_AT)
+struct nfsd_handle_data {
+	int fd;
+	const char *path;
+	struct file_handle *fh;
+	int *mount_id;
+	int flags;
+	int ret;
+	int err;
+};
+
+static void
+nfsd_name_to_handle_func(void *data)
+{
+	struct nfsd_handle_data *d = data;
+
+	d->ret = name_to_handle_at(d->fd, d->path,
+			d->fh, d->mount_id, d->flags);
+	if (d->ret < 0)
+		d->err = errno;
+}
+
+static int
+nfsd_run_name_to_handle_at(struct xthread_workqueue *wq,
+		int fd, const char *path, struct file_handle *fh,
+		int *mount_id, int flags)
+{
+	struct nfsd_handle_data data = {
+		fd,
+		path,
+		fh,
+		mount_id,
+		flags,
+		0,
+		0
+	};
+
+	xthread_work_run_sync(wq, nfsd_name_to_handle_func, &data);
+	if (data.ret < 0)
+		errno = data.err;
+	return data.ret;
+}
+
+int
+nfsd_name_to_handle_at(int fd, const char *path, struct file_handle *fh,
+		int *mount_id, int flags)
+{
+	if (!nfsd_wq)
+		return name_to_handle_at(fd, path, fh, mount_id, flags);
+
+	return nfsd_run_name_to_handle_at(nfsd_wq, fd, path, fh,
+			mount_id, flags);
+}
+#else
+int
+nfsd_name_to_handle_at(int UNUSED(fd), const char *UNUSED(path),
+		struct file_handle *UNUSED(fh),
+		int *UNUSED(mount_id), int UNUSED(flags))
+{
+	errno = ENOSYS;
+	return -1;
+}
+#endif
-- 
2.25.2

[PATCH 3/7] mountd: Fix up path checking helper same_path()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Convert 'same_path()' so that it works when 'rootdir'
is set in the [exports] section of nfs.conf.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 utils/mountd/cache.c | 83 +++++++++++++++++++++++++++++++-------------
 1 file changed, 59 insertions(+), 24 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7d8657c91323..94e9e44b46b9 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -72,6 +72,18 @@ static ssize_t cache_write(int fd, const char *buf, size_t len)
 	return nfsd_path_write(fd, buf, len);
 }
 
+static bool path_lookup_error(int err)
+{
+	switch (err) {
+	case ELOOP:
+	case ENAMETOOLONG:
+	case ENOENT:
+	case ENOTDIR:
+		return 1;
+	}
+	return 0;
+}
+
 /*
  * Support routines for text-based upcalls.
  * Fields are separated by spaces.
@@ -430,23 +442,9 @@ static inline int count_slashes(char *p)
 	return cnt;
 }
 
-static int same_path(char *child, char *parent, int len)
+#if defined(HAVE_STRUCT_FILE_HANDLE)
+static int check_same_path_by_handle(const char *child, const char *parent)
 {
-	static char p[PATH_MAX];
-	struct stat sc, sp;
-
-	if (len <= 0)
-		len = strlen(child);
-	strncpy(p, child, len);
-	p[len] = 0;
-	if (strcmp(p, parent) == 0)
-		return 1;
-
-	/* If number of '/' are different, they must be different */
-	if (count_slashes(p) != count_slashes(parent))
-		return 0;
-
-#if defined(HAVE_NAME_TO_HANDLE_AT) && defined(HAVE_STRUCT_FILE_HANDLE)
 	struct {
 		struct file_handle fh;
 		unsigned char handle[128];
@@ -455,13 +453,17 @@ static int same_path(char *child, char *parent, int len)
 
 	fchild.fh.handle_bytes = 128;
 	fparent.fh.handle_bytes = 128;
-	if (name_to_handle_at(AT_FDCWD, p, &fchild.fh, &mnt_child, 0) != 0) {
-		if (errno == ENOSYS)
-			goto fallback;
-		return 0;
+
+	/* This process should have the CAP_DAC_READ_SEARCH capability */
+	if (nfsd_name_to_handle_at(AT_FDCWD, child, &fchild.fh, &mnt_child, 0) < 0)
+		return -1;
+	if (nfsd_name_to_handle_at(AT_FDCWD, parent, &fparent.fh, &mnt_parent, 0) < 0) {
+		/* If the child resolved, but the parent did not, they differ */
+		if (path_lookup_error(errno))
+			return 0;
+		/* Otherwise, we just don't know */
+		return -1;
 	}
-	if (name_to_handle_at(AT_FDCWD, parent, &fparent.fh, &mnt_parent, 0) != 0)
-		return 0;
 
 	if (mnt_child != mnt_parent ||
 	    fchild.fh.handle_bytes != fparent.fh.handle_bytes ||
@@ -471,14 +473,24 @@ static int same_path(char *child, char *parent, int len)
 		return 0;
 
 	return 1;
-fallback:
+}
+#else
+static int check_same_path_by_handle(const char *child, const char *parent)
+{
+	errno = ENOSYS;
+	return -1;
+}
 #endif
 
+static int check_same_path_by_inode(const char *child, const char *parent)
+{
+	struct stat sc, sp;
+
 	/* This is nearly good enough.  However if a directory is
 	 * bind-mounted in two places and both are exported, it
 	 * could give a false positive
 	 */
-	if (nfsd_path_lstat(p, &sc) != 0)
+	if (nfsd_path_lstat(child, &sc) != 0)
 		return 0;
 	if (nfsd_path_lstat(parent, &sp) != 0)
 		return 0;
@@ -490,6 +502,29 @@ fallback:
 	return 1;
 }
 
+static int same_path(char *child, char *parent, int len)
+{
+	static char p[PATH_MAX];
+	int err;
+
+	if (len <= 0)
+		len = strlen(child);
+	strncpy(p, child, len);
+	p[len] = 0;
+	if (strcmp(p, parent) == 0)
+		return 1;
+
+	/* If number of '/' are different, they must be different */
+	if (count_slashes(p) != count_slashes(parent))
+		return 0;
+
+	/* Try to use filehandle approach before falling back to stat() */
+	err = check_same_path_by_handle(p, parent);
+	if (err != -1)
+		return err;
+	return check_same_path_by_inode(p, parent);
+}
+
 static int is_subdirectory(char *child, char *parent)
 {
 	/* Check is child is strictly a subdirectory of
-- 
2.25.2

[PATCH 4/7] Fix autoconf probe for 'struct nfs_filehandle'

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

It was failing because fcntl.h is not one of the standard
includes.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 configure.ac | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 00b32800c526..df88e58fd0d4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -531,7 +531,12 @@ AC_TYPE_PID_T
 AC_TYPE_SIZE_T
 AC_HEADER_TIME
 AC_STRUCT_TM
-AC_CHECK_TYPES([struct file_handle])
+AC_CHECK_TYPES([struct file_handle], [], [], [[
+		#define _GNU_SOURCE
+		#include <sys/types.h>
+		#include <sys/stat.h>
+		#include <fcntl.h>
+	]])
 
 dnl *************************************************************
 dnl Check for functions
-- 
2.25.2

[PATCH 5/7] mountd: Ensure dump_to_cache() sets errno appropriately

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

cache_write() will set errno if it returns -1, so that callers can
handle errors appropriately, however dump_to_cache() needs to do
so too.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 utils/mountd/cache.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 94e9e44b46b9..0f323226b12a 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -936,12 +936,13 @@ static void write_secinfo(char **bp, int *blen, struct exportent *ep, int flag_m
 
 }
 
-static int dump_to_cache(int f, char *buf, int buflen, char *domain,
+static int dump_to_cache(int f, char *buf, int blen, char *domain,
 			 char *path, struct exportent *exp, int ttl)
 {
 	char *bp = buf;
-	int blen = buflen;
 	time_t now = time(0);
+	size_t buflen;
+	ssize_t err;
 
 	if (ttl <= 1)
 		ttl = DEFAULT_TTL;
@@ -974,8 +975,18 @@ static int dump_to_cache(int f, char *buf, int buflen, char *domain,
 	} else
 		qword_adduint(&bp, &blen, now + ttl);
 	qword_addeol(&bp, &blen);
-	if (blen <= 0) return -1;
-	if (cache_write(f, buf, bp - buf) != bp - buf) return -1;
+	if (blen <= 0) {
+		errno = ENOBUFS;
+		return -1;
+	}
+	buflen = bp - buf;
+	err = cache_write(f, buf, buflen);
+	if (err < 0)
+		return err;
+	if ((size_t)err != buflen) {
+		errno = ENOSPC;
+		return -1;
+	}
 	return 0;
 }
 
-- 
2.25.2

[PATCH 6/7] mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

In nfsd_fh(), if the error returned by the downcall is transient,
then we should ignore it. Only reject the export if the filesystem
path is truly not exportable.
This fixes a case where we can see spurious NFSERR_STALE errors
being returned by knfsd.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 utils/mountd/cache.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 0f323226b12a..79d3ee085a90 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -843,8 +843,14 @@ static void nfsd_fh(int f)
 			}
 		}
 	}
-	if (found && 
-	    found->e_mountpoint &&
+
+	if (!found) {
+		/* The missing dev could be what we want, so just be
+		 * quiet rather than returning stale yet
+		 */
+		if (dev_missing)
+			goto out;
+	} else if (found->e_mountpoint &&
 	    !is_mountpoint(found->e_mountpoint[0]?
 			   found->e_mountpoint:
 			   found->e_path)) {
@@ -855,17 +861,12 @@ static void nfsd_fh(int f)
 		 */
 		/* FIXME we need to make sure we re-visit this later */
 		goto out;
+	} else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) {
+		if (!path_lookup_error(errno))
+			goto out;
+		/* The kernel is saying the path is unexportable */
+		found = NULL;
 	}
-	if (!found && dev_missing) {
-		/* The missing dev could be what we want, so just be
-		 * quite rather than returning stale yet
-		 */
-		goto out;
-	}
-
-	if (found)
-		if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
-			found = 0;
 
 	bp = buf; blen = sizeof(buf);
 	qword_add(&bp, &blen, dom);
-- 
2.25.2

[PATCH 7/7] mountd: Check the stat() return values in match_fsid()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Propagate errors from the stat() calls in match_fsid() so that the
caller can be more careful about how they are handled.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 utils/mountd/cache.c | 45 +++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 79d3ee085a90..6cba2883026f 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -659,55 +659,66 @@ static int parse_fsid(int fsidtype, int fsidlen, char *fsid,
 	return 0;
 }
 
-static bool match_fsid(struct parsed_fsid *parsed, nfs_export *exp, char *path)
+static int match_fsid(struct parsed_fsid *parsed, nfs_export *exp, char *path)
 {
 	struct stat stb;
 	int type;
 	char u[16];
 
 	if (nfsd_path_stat(path, &stb) != 0)
-		return false;
+		goto path_error;
 	if (!S_ISDIR(stb.st_mode) && !S_ISREG(stb.st_mode))
-		return false;
+		goto nomatch;
 
 	switch (parsed->fsidtype) {
 	case FSID_DEV:
 	case FSID_MAJOR_MINOR:
 	case FSID_ENCODE_DEV:
 		if (stb.st_ino != parsed->inode)
-			return false;
+			goto nomatch;
 		if (parsed->major != major(stb.st_dev) ||
 		    parsed->minor != minor(stb.st_dev))
-			return false;
-		return true;
+			goto nomatch;
+		goto match;
 	case FSID_NUM:
 		if (((exp->m_export.e_flags & NFSEXP_FSID) == 0 ||
 		     exp->m_export.e_fsid != parsed->fsidnum))
-			return false;
-		return true;
+			goto nomatch;
+		goto match;
 	case FSID_UUID4_INUM:
 	case FSID_UUID16_INUM:
 		if (stb.st_ino != parsed->inode)
-			return false;
+			goto nomatch;
 		goto check_uuid;
 	case FSID_UUID8:
 	case FSID_UUID16:
-		if (!is_mountpoint(path))
-			return false;
+		errno = 0;
+		if (!is_mountpoint(path)) {
+			if (!errno)
+				goto nomatch;
+			goto path_error;
+		}
 	check_uuid:
 		if (exp->m_export.e_uuid) {
 			get_uuid(exp->m_export.e_uuid, parsed->uuidlen, u);
 			if (memcmp(u, parsed->fhuuid, parsed->uuidlen) == 0)
-				return true;
+				goto match;
 		}
 		else
 			for (type = 0;
 			     uuid_by_path(path, type, parsed->uuidlen, u);
 			     type++)
 				if (memcmp(u, parsed->fhuuid, parsed->uuidlen) == 0)
-					return true;
+					goto match;
 	}
-	return false;
+nomatch:
+	return 0;
+match:
+	return 1;
+path_error:
+	if (path_lookup_error(errno))
+		goto nomatch;
+	return -1;
 }
 
 static struct addrinfo *lookup_client_addr(char *dom)
@@ -815,8 +826,12 @@ static void nfsd_fh(int f)
 					   exp->m_export.e_path))
 				dev_missing ++;
 
-			if (!match_fsid(&parsed, exp, path))
+			switch(match_fsid(&parsed, exp, path)) {
+			case 0:
 				continue;
+			case -1:
+				goto out;
+			}
 			if (is_ipaddr_client(dom)
 					&& !ipaddr_client_matches(exp, ai))
 				continue;
-- 
2.25.2

Re: [PATCH 6/7] mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh()

[J. Bruce Fields]

On Thu, Apr 16, 2020 at 06:12:51PM -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> In nfsd_fh(), if the error returned by the downcall is transient,
> then we should ignore it. Only reject the export if the filesystem
> path is truly not exportable.
> This fixes a case where we can see spurious NFSERR_STALE errors
> being returned by knfsd.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  utils/mountd/cache.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 0f323226b12a..79d3ee085a90 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -843,8 +843,14 @@ static void nfsd_fh(int f)
>  			}
>  		}
>  	}
> -	if (found && 
> -	    found->e_mountpoint &&
> +
> +	if (!found) {
> +		/* The missing dev could be what we want, so just be
> +		 * quiet rather than returning stale yet
> +		 */
> +		if (dev_missing)
> +			goto out;
> +	} else if (found->e_mountpoint &&
>  	    !is_mountpoint(found->e_mountpoint[0]?
>  			   found->e_mountpoint:
>  			   found->e_path)) {
> @@ -855,17 +861,12 @@ static void nfsd_fh(int f)
>  		 */
>  		/* FIXME we need to make sure we re-visit this later */
>  		goto out;
> +	} else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) {
> +		if (!path_lookup_error(errno))
> +			goto out;
> +		/* The kernel is saying the path is unexportable */
> +		found = NULL;
>  	}
> -	if (!found && dev_missing) {
> -		/* The missing dev could be what we want, so just be
> -		 * quite rather than returning stale yet
> -		 */
> -		goto out;
> -	}
> -
> -	if (found)
> -		if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
> -			found = 0;
>  
>  	bp = buf; blen = sizeof(buf);
>  	qword_add(&bp, &blen, dom);

Is everybody else better at reading this kind of patch?  Between the
code reshuffling and the conditionals, it's almost totally opaque to me.
I'd have split it up something like the below.--b.

commit b0b623281017
Author: Trond Myklebust <trond.myklebust@hammerspace.com>
Date:   Thu Apr 16 18:12:51 2020 -0400

    mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh()
    
    In nfsd_fh(), if the error returned by the downcall is transient,
    then we should ignore it. Only reject the export if the filesystem
    path is truly not exportable.
    This fixes a case where we can see spurious NFSERR_STALE errors
    being returned by knfsd.
    
    Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index bcd4b3ce5bb2..0426b171f040 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -826,8 +826,12 @@ static void nfsd_fh(int f)
 		 */
 		/* FIXME we need to make sure we re-visit this later */
 		goto out;
-	} else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
-			found = 0;
+	} else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) {
+		if (!path_lookup_error(errno))
+			goto out;
+		/* The kernel is saying the path is unexportable */
+		found = NULL;
+	}
 
 	bp = buf; blen = sizeof(buf);
 	qword_add(&bp, &blen, dom);

commit 931a2e77f954
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Apr 27 13:22:30 2020 -0400

    rearrange logic; no change in behavior

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 8f54e37b7936..bcd4b3ce5bb2 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -808,8 +808,14 @@ static void nfsd_fh(int f)
 			}
 		}
 	}
-	if (found && 
-	    found->e_mountpoint &&
+
+	if (!found) {
+		/* The missing dev could be what we want, so just be
+		 * quiet rather than returning stale yet
+		 */
+		if (dev_missing)
+			goto out;
+	} else if (found->e_mountpoint &&
 	    !is_mountpoint(found->e_mountpoint[0]?
 			   found->e_mountpoint:
 			   found->e_path)) {
@@ -820,16 +826,7 @@ static void nfsd_fh(int f)
 		 */
 		/* FIXME we need to make sure we re-visit this later */
 		goto out;
-	}
-	if (!found && dev_missing) {
-		/* The missing dev could be what we want, so just be
-		 * quite rather than returning stale yet
-		 */
-		goto out;
-	}
-
-	if (found)
-		if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
+	} else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
 			found = 0;
 
 	bp = buf; blen = sizeof(buf);

Re: [PATCH 6/7] mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh()

[J. Bruce Fields]

Ack to the series, otherwise, looks good.--b.

Re: [PATCH 0/7] nfs-utils fixes

[Steve Dickson]

On 4/16/20 6:12 PM, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> This patchset fixes a couple of missed API changes in mountd to
> ensure that the [exports]rootdir root jail works correctly. It
> fixes up the 'same_path' function, as well as 'uuid_by_path'.
> It also improves the error handling, and tries to distinguish
> between bona fide path resolution problems, and other transient
> issues in order to avoid having knfsd return spurious ESTALE
> errors.
> 
> Trond Myklebust (7):
>   mountd: Add a helper nfsd_path_statfs64() for uuid_by_path()
>   nfsd: Support running nfsd_name_to_handle_at() in the root jail
>   mountd: Fix up path checking helper same_path()
>   Fix autoconf probe for 'struct nfs_filehandle'
>   mountd: Ensure dump_to_cache() sets errno appropriately
>   mountd: Ignore transient and non-fatal filesystem errors in nfsd_fh()
>   mountd: Check the stat() return values in match_fsid()
> 
>  configure.ac                |   7 +-
>  support/include/nfsd_path.h |   9 ++
>  support/misc/nfsd_path.c    | 109 ++++++++++++++++++++++
>  utils/mountd/cache.c        | 174 ++++++++++++++++++++++++------------
>  4 files changed, 242 insertions(+), 57 deletions(-)
> 
Committed the series (tag: nfs-utils-2-4-4-rc4)

steved.