nfs-utils crossmnt bugfix, and cleanup

nfs-utils crossmnt bugfix, and cleanup

[J. Bruce Fields]

We've long had a bug with crossmnt handling which could cause a parent's
export options to override those of a child in some cases.

While I was there, I also did some cleanup, mainly of nfsd_fh().  It was
long and complicated, now it's short and complicated.  We could probably
simplify the logic with a little more work.

--b.

[PATCH 1/4] mountd: prefer explicit subexports over crossmnt parents

[J. Bruce Fields]

If a parent is exported with crossmnt, and if a child is also explicitly
exported, then both exports could potentially produce matches in this
loop; that isn't a bug.

Instead of warning and ignoring the second match we find, we should
instead prefer whichever export is deeper in the tree, so that
children's options can override those of their parents.

Reported-by: Olga Kornievskaia <aglo@citi.umich.edu>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 utils/mountd/cache.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 68cccdf..c3dee13 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -341,6 +341,17 @@ static char *next_mnt(void **v, char *p)
 	return me->mnt_dir;
 }
 
+/* True iff e1 is a child of e2 and e2 has crossmnt set: */
+static bool subexport(struct exportent *e1, struct exportent *e2)
+{
+	char *p1 = e1->e_path, *p2 = e2->e_path;
+	int l2 = strlen(p2);
+
+	return e2->e_flags & NFSEXP_CROSSMOUNT
+	       && strncmp(p1, p2, l2) == 0
+	       && p1[l2] == '/';
+}
+
 static void nfsd_fh(FILE *f)
 {
 	/* request are:
@@ -550,13 +561,14 @@ static void nfsd_fh(FILE *f)
 				if (!client_check(exp->m_client, ai))
 					continue;
 			}
-			/* It's a match !! */
-			if (!found) {
+			if (!found || subexport(&exp->m_export, found)) {
 				found = &exp->m_export;
+				free(found_path);
 				found_path = strdup(path);
 				if (found_path == NULL)
 					goto out;
-			} else if (strcmp(found->e_path, exp->m_export.e_path)!= 0)
+			} else if (strcmp(found->e_path, exp->m_export.e_path)
+				   && !subexport(found, &exp->m_export))
 			{
 				xlog(L_WARNING, "%s and %s have same filehandle for %s, using first",
 				     found_path, path, dom);
-- 
1.7.4.1

[PATCH 2/4] mountd: gather fsid information into one struct

[J. Bruce Fields]

A large part of nfsd_fh() is concerned with extracting
fsid-type-specific information from the fsid, then matching that
information with information from the export list and the filesystem.

Moving all that information into one struct will allow some further
simplifications.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 utils/mountd/cache.c |   79 ++++++++++++++++++++++++++++---------------------
 1 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index c3dee13..897d61d 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -352,6 +352,18 @@ static bool subexport(struct exportent *e1, struct exportent *e2)
 	       && p1[l2] == '/';
 }
 
+struct parsed_fsid {
+	int fsidtype;
+	/* We could use a union for this, but it would be more
+	 * complicated; why bother? */
+	unsigned int inode;
+	unsigned int minor;
+	unsigned int major;
+	unsigned int fsidnum;
+	int uuidlen;
+	char *fhuuid;
+};
+
 static void nfsd_fh(FILE *f)
 {
 	/* request are:
@@ -363,19 +375,16 @@ static void nfsd_fh(FILE *f)
 	char *dom;
 	int fsidtype;
 	int fsidlen;
-	unsigned int dev, major=0, minor=0;
-	unsigned int inode=0;
 	unsigned long long inode64;
-	unsigned int fsidnum=0;
+	unsigned int dev;
 	char fsid[32];
+	struct parsed_fsid parsed;
 	struct exportent *found = NULL;
 	struct addrinfo *ai = NULL;
 	char *found_path = NULL;
 	nfs_export *exp;
 	int i;
 	int dev_missing = 0;
-	int uuidlen = 0;
-	char *fhuuid = NULL;
 
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;
@@ -400,15 +409,15 @@ static void nfsd_fh(FILE *f)
 		if (fsidlen != 8)
 			goto out;
 		memcpy(&dev, fsid, 4);
-		memcpy(&inode, fsid+4, 4);
-		major = ntohl(dev)>>16;
-		minor = ntohl(dev) & 0xFFFF;
+		memcpy(&parsed.inode, fsid+4, 4);
+		parsed.major = ntohl(dev)>>16;
+		parsed.minor = ntohl(dev) & 0xFFFF;
 		break;
 
 	case FSID_NUM: /* 4 bytes - fsid */
 		if (fsidlen != 4)
 			goto out;
-		memcpy(&fsidnum, fsid, 4);
+		memcpy(&parsed.fsidnum, fsid, 4);
 		break;
 
 	case FSID_MAJOR_MINOR: /* 12 bytes: 4 major, 4 minor, 4 inode 
@@ -417,9 +426,11 @@ static void nfsd_fh(FILE *f)
 		 */
 		if (fsidlen != 12)
 			goto out;
-		memcpy(&dev, fsid, 4); major = ntohl(dev);
-		memcpy(&dev, fsid+4, 4); minor = ntohl(dev);
-		memcpy(&inode, fsid+8, 4);
+		memcpy(&dev, fsid, 4);
+		parsed.major = ntohl(dev);
+		memcpy(&dev, fsid+4, 4);
+		parsed.minor = ntohl(dev);
+		memcpy(&parsed.inode, fsid+8, 4);
 		break;
 
 	case FSID_ENCODE_DEV: /* 8 bytes: 4 byte packed device number, 4 inode */
@@ -429,37 +440,37 @@ static void nfsd_fh(FILE *f)
 		if (fsidlen != 8)
 			goto out;
 		memcpy(&dev, fsid, 4);
-		memcpy(&inode, fsid+4, 4);
-		major = (dev & 0xfff00) >> 8;
-		minor = (dev & 0xff) | ((dev >> 12) & 0xfff00);
+		memcpy(&parsed.inode, fsid+4, 4);
+		parsed.major = (dev & 0xfff00) >> 8;
+		parsed.minor = (dev & 0xff) | ((dev >> 12) & 0xfff00);
 		break;
 
 	case FSID_UUID4_INUM: /* 4 byte inode number and 4 byte uuid */
 		if (fsidlen != 8)
 			goto out;
-		memcpy(&inode, fsid, 4);
-		uuidlen = 4;
-		fhuuid = fsid+4;
+		memcpy(&parsed.inode, fsid, 4);
+		parsed.uuidlen = 4;
+		parsed.fhuuid = fsid+4;
 		break;
 	case FSID_UUID8: /* 8 byte uuid */
 		if (fsidlen != 8)
 			goto out;
-		uuidlen = 8;
-		fhuuid = fsid;
+		parsed.uuidlen = 8;
+		parsed.fhuuid = fsid;
 		break;
 	case FSID_UUID16: /* 16 byte uuid */
 		if (fsidlen != 16)
 			goto out;
-		uuidlen = 16;
-		fhuuid = fsid;
+		parsed.uuidlen = 16;
+		parsed.fhuuid = fsid;
 		break;
 	case FSID_UUID16_INUM: /* 8 byte inode number and 16 byte uuid */
 		if (fsidlen != 24)
 			goto out;
 		memcpy(&inode64, fsid, 8);
-		inode = inode64;
-		uuidlen = 16;
-		fhuuid = fsid+8;
+		parsed.inode = inode64;
+		parsed.uuidlen = 16;
+		parsed.fhuuid = fsid+8;
 		break;
 	}
 
@@ -514,20 +525,20 @@ static void nfsd_fh(FILE *f)
 			case FSID_DEV:
 			case FSID_MAJOR_MINOR:
 			case FSID_ENCODE_DEV:
-				if (stb.st_ino != inode)
+				if (stb.st_ino != parsed.inode)
 					continue;
-				if (major != major(stb.st_dev) ||
-				    minor != minor(stb.st_dev))
+				if (parsed.major != major(stb.st_dev) ||
+				    parsed.minor != minor(stb.st_dev))
 					continue;
 				break;
 			case FSID_NUM:
 				if (((exp->m_export.e_flags & NFSEXP_FSID) == 0 ||
-				     exp->m_export.e_fsid != fsidnum))
+				     exp->m_export.e_fsid != parsed.fsidnum))
 					continue;
 				break;
 			case FSID_UUID4_INUM:
 			case FSID_UUID16_INUM:
-				if (stb.st_ino != inode)
+				if (stb.st_ino != parsed.inode)
 					continue;
 				goto check_uuid;
 			case FSID_UUID8:
@@ -537,15 +548,15 @@ static void nfsd_fh(FILE *f)
 			check_uuid:
 				if (exp->m_export.e_uuid)
 					get_uuid(exp->m_export.e_uuid,
-						 uuidlen, u);
+						 parsed.uuidlen, u);
 				else
 					for (type = 0;
-					     uuid_by_path(path, type, uuidlen, u);
+					     uuid_by_path(path, type, parsed.uuidlen, u);
 					     type++)
-						if (memcmp(u, fhuuid, uuidlen) == 0)
+						if (memcmp(u, parsed.fhuuid, parsed.uuidlen) == 0)
 							break;
 
-				if (memcmp(u, fhuuid, uuidlen) != 0)
+				if (memcmp(u, parsed.fhuuid, parsed.uuidlen) != 0)
 					continue;
 				break;
 			}
-- 
1.7.4.1

[PATCH 3/4] mountd: move fsidtype-specific code to helpers

[J. Bruce Fields]

Now we can move these big switch statements into helper functions.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 utils/mountd/cache.c |  240 +++++++++++++++++++++++++++-----------------------
 1 files changed, 129 insertions(+), 111 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 897d61d..d2ae4563 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -364,60 +364,26 @@ struct parsed_fsid {
 	char *fhuuid;
 };
 
-static void nfsd_fh(FILE *f)
+int parse_fsid(int fsidtype, int fsidlen, char *fsid, struct parsed_fsid *parsed)
 {
-	/* request are:
-	 *  domain fsidtype fsid
-	 * interpret fsid, find export point and options, and write:
-	 *  domain fsidtype fsid expiry path
-	 */
-	char *cp;
-	char *dom;
-	int fsidtype;
-	int fsidlen;
-	unsigned long long inode64;
 	unsigned int dev;
-	char fsid[32];
-	struct parsed_fsid parsed;
-	struct exportent *found = NULL;
-	struct addrinfo *ai = NULL;
-	char *found_path = NULL;
-	nfs_export *exp;
-	int i;
-	int dev_missing = 0;
-
-	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
-		return;
-
-	xlog(D_CALL, "nfsd_fh: inbuf '%s'", lbuf);
+	unsigned long long inode64;
 
-	cp = lbuf;
-	
-	dom = malloc(strlen(cp));
-	if (dom == NULL)
-		return;
-	if (qword_get(&cp, dom, strlen(cp)) <= 0)
-		goto out;
-	if (qword_get_int(&cp, &fsidtype) != 0)
-		goto out;
-	if (fsidtype < 0 || fsidtype > 7)
-		goto out; /* unknown type */
-	if ((fsidlen = qword_get(&cp, fsid, 32)) <= 0)
-		goto out;
+	parsed->fsidtype = fsidtype;
 	switch(fsidtype) {
 	case FSID_DEV: /* 4 bytes: 2 major, 2 minor, 4 inode */
 		if (fsidlen != 8)
-			goto out;
+			return -1;
 		memcpy(&dev, fsid, 4);
-		memcpy(&parsed.inode, fsid+4, 4);
-		parsed.major = ntohl(dev)>>16;
-		parsed.minor = ntohl(dev) & 0xFFFF;
+		memcpy(&parsed->inode, fsid+4, 4);
+		parsed->major = ntohl(dev)>>16;
+		parsed->minor = ntohl(dev) & 0xFFFF;
 		break;
 
 	case FSID_NUM: /* 4 bytes - fsid */
 		if (fsidlen != 4)
-			goto out;
-		memcpy(&parsed.fsidnum, fsid, 4);
+			return -1;
+		memcpy(&parsed->fsidnum, fsid, 4);
 		break;
 
 	case FSID_MAJOR_MINOR: /* 12 bytes: 4 major, 4 minor, 4 inode 
@@ -425,12 +391,12 @@ static void nfsd_fh(FILE *f)
 		 * an historical accident
 		 */
 		if (fsidlen != 12)
-			goto out;
+			return -1;
 		memcpy(&dev, fsid, 4);
-		parsed.major = ntohl(dev);
+		parsed->major = ntohl(dev);
 		memcpy(&dev, fsid+4, 4);
-		parsed.minor = ntohl(dev);
-		memcpy(&parsed.inode, fsid+8, 4);
+		parsed->minor = ntohl(dev);
+		memcpy(&parsed->inode, fsid+8, 4);
 		break;
 
 	case FSID_ENCODE_DEV: /* 8 bytes: 4 byte packed device number, 4 inode */
@@ -438,41 +404,137 @@ static void nfsd_fh(FILE *f)
 		 * no-one outside this host has any business interpreting it
 		 */
 		if (fsidlen != 8)
-			goto out;
+			return -1;
 		memcpy(&dev, fsid, 4);
-		memcpy(&parsed.inode, fsid+4, 4);
-		parsed.major = (dev & 0xfff00) >> 8;
-		parsed.minor = (dev & 0xff) | ((dev >> 12) & 0xfff00);
+		memcpy(&parsed->inode, fsid+4, 4);
+		parsed->major = (dev & 0xfff00) >> 8;
+		parsed->minor = (dev & 0xff) | ((dev >> 12) & 0xfff00);
 		break;
 
 	case FSID_UUID4_INUM: /* 4 byte inode number and 4 byte uuid */
 		if (fsidlen != 8)
-			goto out;
-		memcpy(&parsed.inode, fsid, 4);
-		parsed.uuidlen = 4;
-		parsed.fhuuid = fsid+4;
+			return -1;
+		memcpy(&parsed->inode, fsid, 4);
+		parsed->uuidlen = 4;
+		parsed->fhuuid = fsid+4;
 		break;
 	case FSID_UUID8: /* 8 byte uuid */
 		if (fsidlen != 8)
-			goto out;
-		parsed.uuidlen = 8;
-		parsed.fhuuid = fsid;
+			return -1;
+		parsed->uuidlen = 8;
+		parsed->fhuuid = fsid;
 		break;
 	case FSID_UUID16: /* 16 byte uuid */
 		if (fsidlen != 16)
-			goto out;
-		parsed.uuidlen = 16;
-		parsed.fhuuid = fsid;
+			return -1;
+		parsed->uuidlen = 16;
+		parsed->fhuuid = fsid;
 		break;
 	case FSID_UUID16_INUM: /* 8 byte inode number and 16 byte uuid */
 		if (fsidlen != 24)
-			goto out;
+			return -1;
 		memcpy(&inode64, fsid, 8);
-		parsed.inode = inode64;
-		parsed.uuidlen = 16;
-		parsed.fhuuid = fsid+8;
+		parsed->inode = inode64;
+		parsed->uuidlen = 16;
+		parsed->fhuuid = fsid+8;
 		break;
 	}
+	return 0;
+}
+
+static bool match_fsid(struct parsed_fsid *parsed, nfs_export *exp, char *path)
+{
+	struct stat stb;
+	int type;
+	char u[16];
+
+	if (stat(path, &stb) != 0)
+		return false;
+	if (!S_ISDIR(stb.st_mode) && !S_ISREG(stb.st_mode))
+		return false;
+
+	switch (parsed->fsidtype) {
+	case FSID_DEV:
+	case FSID_MAJOR_MINOR:
+	case FSID_ENCODE_DEV:
+		if (stb.st_ino != parsed->inode)
+			return false;
+		if (parsed->major != major(stb.st_dev) ||
+		    parsed->minor != minor(stb.st_dev))
+			return false;
+		return true;
+	case FSID_NUM:
+		if (((exp->m_export.e_flags & NFSEXP_FSID) == 0 ||
+		     exp->m_export.e_fsid != parsed->fsidnum))
+			return false;
+		return true;
+	case FSID_UUID4_INUM:
+	case FSID_UUID16_INUM:
+		if (stb.st_ino != parsed->inode)
+			return false;
+		goto check_uuid;
+	case FSID_UUID8:
+	case FSID_UUID16:
+		if (!is_mountpoint(path))
+			return false;
+	check_uuid:
+		if (exp->m_export.e_uuid)
+			get_uuid(exp->m_export.e_uuid, parsed->uuidlen, u);
+		else
+			for (type = 0;
+			     uuid_by_path(path, type, parsed->uuidlen, u);
+			     type++)
+				if (memcmp(u, parsed->fhuuid, parsed->uuidlen) == 0)
+					return true;
+
+		if (memcmp(u, parsed->fhuuid, parsed->uuidlen) != 0)
+			return false;
+		return true;
+	}
+	/* Well, unreachable, actually: */
+	return false;
+}
+
+static void nfsd_fh(FILE *f)
+{
+	/* request are:
+	 *  domain fsidtype fsid
+	 * interpret fsid, find export point and options, and write:
+	 *  domain fsidtype fsid expiry path
+	 */
+	char *cp;
+	char *dom;
+	int fsidtype;
+	int fsidlen;
+	char fsid[32];
+	struct parsed_fsid parsed;
+	struct exportent *found = NULL;
+	struct addrinfo *ai = NULL;
+	char *found_path = NULL;
+	nfs_export *exp;
+	int i;
+	int dev_missing = 0;
+
+	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
+		return;
+
+	xlog(D_CALL, "nfsd_fh: inbuf '%s'", lbuf);
+
+	cp = lbuf;
+
+	dom = malloc(strlen(cp));
+	if (dom == NULL)
+		return;
+	if (qword_get(&cp, dom, strlen(cp)) <= 0)
+		goto out;
+	if (qword_get_int(&cp, &fsidtype) != 0)
+		goto out;
+	if (fsidtype < 0 || fsidtype > 7)
+		goto out; /* unknown type */
+	if ((fsidlen = qword_get(&cp, fsid, 32)) <= 0)
+		goto out;
+	if (parse_fsid(fsidtype, fsidlen, fsid, &parsed))
+		goto out;
 
 	auth_reload();
 
@@ -480,10 +542,7 @@ static void nfsd_fh(FILE *f)
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
 		nfs_export *next_exp;
 		for (exp = exportlist[i].p_head; exp; exp = next_exp) {
-			struct stat stb;
-			char u[16];
 			char *path;
-			int type;
 
 			if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT) {
 				static nfs_export *prev = NULL;
@@ -516,50 +575,9 @@ static void nfsd_fh(FILE *f)
 					   exp->m_export.e_mountpoint:
 					   exp->m_export.e_path))
 				dev_missing ++;
-			if (stat(path, &stb) != 0)
-				continue;
-			if (!S_ISDIR(stb.st_mode) && !S_ISREG(stb.st_mode)) {
+
+			if (!match_fsid(&parsed, exp, path))
 				continue;
-			}
-			switch(fsidtype){
-			case FSID_DEV:
-			case FSID_MAJOR_MINOR:
-			case FSID_ENCODE_DEV:
-				if (stb.st_ino != parsed.inode)
-					continue;
-				if (parsed.major != major(stb.st_dev) ||
-				    parsed.minor != minor(stb.st_dev))
-					continue;
-				break;
-			case FSID_NUM:
-				if (((exp->m_export.e_flags & NFSEXP_FSID) == 0 ||
-				     exp->m_export.e_fsid != parsed.fsidnum))
-					continue;
-				break;
-			case FSID_UUID4_INUM:
-			case FSID_UUID16_INUM:
-				if (stb.st_ino != parsed.inode)
-					continue;
-				goto check_uuid;
-			case FSID_UUID8:
-			case FSID_UUID16:
-				if (!is_mountpoint(path))
-					continue;
-			check_uuid:
-				if (exp->m_export.e_uuid)
-					get_uuid(exp->m_export.e_uuid,
-						 parsed.uuidlen, u);
-				else
-					for (type = 0;
-					     uuid_by_path(path, type, parsed.uuidlen, u);
-					     type++)
-						if (memcmp(u, parsed.fhuuid, parsed.uuidlen) == 0)
-							break;
-
-				if (memcmp(u, parsed.fhuuid, parsed.uuidlen) != 0)
-					continue;
-				break;
-			}
 			if (use_ipaddr) {
 				if (ai == NULL) {
 					struct addrinfo *tmp;
-- 
1.7.4.1

[PATCH 4/4] mountd: don't automatically add subexports to kernel cache

[J. Bruce Fields]

Given a crossmnt export and a child path, search for all mountpoints in
between and add exports for them to the kernel.

This isn't necessary; if the the kernel needs one of the subexports, it
can always ask.

It might be some sort of minor optimization to add exports preemptively.
But if so, why not just add them all?  Why this particular subset?

Given the chance to delete some code I can't justify, I'll take it.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 utils/mountd/cache.c |   42 +-----------------------------------------
 1 files changed, 1 insertions(+), 41 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d2ae4563..c77eb27 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -945,53 +945,13 @@ static int cache_export_ent(char *domain, struct exportent *exp, char *path)
 	if (!f)
 		return -1;
 
-	err = dump_to_cache(f, domain, exp->e_path, exp);
+	err = dump_to_cache(f, domain, path, exp);
 	if (err) {
 		xlog(L_WARNING,
 		     "Cannot export %s, possibly unsupported filesystem or"
 		     " fsid= required", exp->e_path);
 	}
 
-	while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
-		/* really an 'if', but we can break out of
-		 * a 'while' more easily */
-		/* Look along 'path' for other filesystems
-		 * and export them with the same options
-		 */
-		struct stat stb;
-		size_t l = strlen(exp->e_path);
-		__dev_t dev;
-
-		if (strlen(path) <= l || path[l] != '/' ||
-		    strncmp(exp->e_path, path, l) != 0)
-			break;
-		if (stat(exp->e_path, &stb) != 0)
-			break;
-		dev = stb.st_dev;
-		while(path[l] == '/') {
-			char c;
-			/* errors for submount should fail whole filesystem */
-			int err2;
-
-			l++;
-			while (path[l] != '/' && path[l])
-				l++;
-			c = path[l];
-			path[l] = 0;
-			err2 = lstat(path, &stb);
-			path[l] = c;
-			if (err2 < 0)
-				break;
-			if (stb.st_dev == dev)
-				continue;
-			dev = stb.st_dev;
-			path[l] = 0;
-			dump_to_cache(f, domain, path, exp);
-			path[l] = c;
-		}
-		break;
-	}
-
 	fclose(f);
 	return err;
 }
-- 
1.7.4.1

Re: [PATCH 4/4] mountd: don't automatically add subexports to kernel cache

[Steve Dickson]

Hey Bruce,

This patch breaks cross mounts... Here is my set up

/home/fs1 two different file systems:

Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/VolGroup00-HomeDirs
                      15236080   9439036   5010612  66% /home
RedHat# df /home/fs1
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/VolGroup00-FS1
                      10321208    154468   9642452   2% /home/fs1

The export is:
  /home *(rw,crossmnt,nohide,sec=sys:krb5:krb5i:krb5p)

when I mount from a f15 client
    mount -v -o v3 redhat:/home/fs1/tmp/tophat /mnt/tmp
I get:
    mount.nfs: mount(2): Stale NFS file handle

When I revert the patch, the mount works.

Unfortunately I'm going to be taking the next few days off
so I am not going be able to debug this... So I'm going to 
wait on the entire patch series until we can sort this out...

steved.

On 06/14/2011 10:58 AM, J. Bruce Fields wrote:
> Given a crossmnt export and a child path, search for all mountpoints in
> between and add exports for them to the kernel.
> 
> This isn't necessary; if the the kernel needs one of the subexports, it
> can always ask.
> 
> It might be some sort of minor optimization to add exports preemptively.
> But if so, why not just add them all?  Why this particular subset?
> 
> Given the chance to delete some code I can't justify, I'll take it.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  utils/mountd/cache.c |   42 +-----------------------------------------
>  1 files changed, 1 insertions(+), 41 deletions(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index d2ae4563..c77eb27 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -945,53 +945,13 @@ static int cache_export_ent(char *domain, struct exportent *exp, char *path)
>  	if (!f)
>  		return -1;
>  
> -	err = dump_to_cache(f, domain, exp->e_path, exp);
> +	err = dump_to_cache(f, domain, path, exp);
>  	if (err) {
>  		xlog(L_WARNING,
>  		     "Cannot export %s, possibly unsupported filesystem or"
>  		     " fsid= required", exp->e_path);
>  	}
>  
> -	while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
> -		/* really an 'if', but we can break out of
> -		 * a 'while' more easily */
> -		/* Look along 'path' for other filesystems
> -		 * and export them with the same options
> -		 */
> -		struct stat stb;
> -		size_t l = strlen(exp->e_path);
> -		__dev_t dev;
> -
> -		if (strlen(path) <= l || path[l] != '/' ||
> -		    strncmp(exp->e_path, path, l) != 0)
> -			break;
> -		if (stat(exp->e_path, &stb) != 0)
> -			break;
> -		dev = stb.st_dev;
> -		while(path[l] == '/') {
> -			char c;
> -			/* errors for submount should fail whole filesystem */
> -			int err2;
> -
> -			l++;
> -			while (path[l] != '/' && path[l])
> -				l++;
> -			c = path[l];
> -			path[l] = 0;
> -			err2 = lstat(path, &stb);
> -			path[l] = c;
> -			if (err2 < 0)
> -				break;
> -			if (stb.st_dev == dev)
> -				continue;
> -			dev = stb.st_dev;
> -			path[l] = 0;
> -			dump_to_cache(f, domain, path, exp);
> -			path[l] = c;
> -		}
> -		break;
> -	}
> -
>  	fclose(f);
>  	return err;
>  }

Re: [PATCH 4/4] mountd: don't automatically add subexports to kernel cache

[J. Bruce Fields]

On Wed, Jun 22, 2011 at 06:30:32PM -0400, Steve Dickson wrote:
> Hey Bruce,
> 
> This patch breaks cross mounts... Here is my set up
> 
> /home/fs1 two different file systems:
> 
> Filesystem           1K-blocks      Used Available Use% Mounted on
> /dev/mapper/VolGroup00-HomeDirs
>                       15236080   9439036   5010612  66% /home
> RedHat# df /home/fs1
> Filesystem           1K-blocks      Used Available Use% Mounted on
> /dev/mapper/VolGroup00-FS1
>                       10321208    154468   9642452   2% /home/fs1
> 
> The export is:
>   /home *(rw,crossmnt,nohide,sec=sys:krb5:krb5i:krb5p)
> 
> when I mount from a f15 client
>     mount -v -o v3 redhat:/home/fs1/tmp/tophat /mnt/tmp
> I get:
>     mount.nfs: mount(2): Stale NFS file handle
> 
> When I revert the patch, the mount works.

Oh, crap, I didn't check v3.  And, OK, it makes sense that v3 might need
this.

> Unfortunately I'm going to be taking the next few days off
> so I am not going be able to debug this... So I'm going to 
> wait on the entire patch series until we can sort this out...

No problem.  But if you want to apply the rest of the series and drop
just this one patch, that would be fine too.  (That's probably what will
end up being the fix anyway.)

--b.

Re: [PATCH 4/4] mountd: don't automatically add subexports to kernel cache

[Steve Dickson]

On 06/22/2011 06:34 PM, J. Bruce Fields wrote:
> On Wed, Jun 22, 2011 at 06:30:32PM -0400, Steve Dickson wrote:
>> Hey Bruce,
>>
>> This patch breaks cross mounts... Here is my set up
>>
>> /home/fs1 two different file systems:
>>
>> Filesystem           1K-blocks      Used Available Use% Mounted on
>> /dev/mapper/VolGroup00-HomeDirs
>>                       15236080   9439036   5010612  66% /home
>> RedHat# df /home/fs1
>> Filesystem           1K-blocks      Used Available Use% Mounted on
>> /dev/mapper/VolGroup00-FS1
>>                       10321208    154468   9642452   2% /home/fs1
>>
>> The export is:
>>   /home *(rw,crossmnt,nohide,sec=sys:krb5:krb5i:krb5p)
>>
>> when I mount from a f15 client
>>     mount -v -o v3 redhat:/home/fs1/tmp/tophat /mnt/tmp
>> I get:
>>     mount.nfs: mount(2): Stale NFS file handle
>>
>> When I revert the patch, the mount works.
> 
> Oh, crap, I didn't check v3.  And, OK, it makes sense that v3 might need
> this.
> 
>> Unfortunately I'm going to be taking the next few days off
>> so I am not going be able to debug this... So I'm going to 
>> wait on the entire patch series until we can sort this out...
> 
> No problem.  But if you want to apply the rest of the series and drop
> just this one patch, that would be fine too.  (That's probably what will
> end up being the fix anyway.)
Ok... I just committed the first three patches of this series...

steved.

Re: nfs-utils crossmnt bugfix, and cleanup

[Steve Dickson]

Hey Bruce,

On 06/14/2011 10:58 AM, J. Bruce Fields wrote:
> We've long had a bug with crossmnt handling which could cause a parent's
> export options to override those of a child in some cases.
> 
> While I was there, I also did some cleanup, mainly of nfsd_fh().  It was
> long and complicated, now it's short and complicated.  We could probably
> simplify the logic with a little more work.
> 
> --b.
Which bug are you referring to?

steved. 

Re: nfs-utils crossmnt bugfix, and cleanup

[J. Bruce Fields]

On Sat, Jun 18, 2011 at 09:27:30AM -0400, Steve Dickson wrote:
> Hey Bruce,
> 
> On 06/14/2011 10:58 AM, J. Bruce Fields wrote:
> > We've long had a bug with crossmnt handling which could cause a parent's
> > export options to override those of a child in some cases.
> > 
> > While I was there, I also did some cleanup, mainly of nfsd_fh().  It was
> > long and complicated, now it's short and complicated.  We could probably
> > simplify the logic with a little more work.
> > 
> > --b.
> Which bug are you referring to?

Suppose /etc/exports contains:

	/foo		*(rw,crossmnt)
	/foo/bar	*(ro)

The "crossmnt" tells us to export all filesystems under /foo recursively
with the same options as /foo is exported with.  So there's a conflict:
should /foo/bar be exported ro, or rw?

The logical thing to do is to make the "ro" on the explicit export of
/foo/bar override the inherited "rw" from /foo, and in practice that's
what people have obviously expected in every case I've seen.

But we don't do that.  Actually, it's worse than that: the /foo export
wins in the nfsd_fh upcall, and the /foo/bar export wins in the
nfsd_export upcall.  So you can get inconsistent results depending on
which order things happen in.

So the first patch fixes nfsd_fh to behave like nfsd_export does, and
prefer /foo/bar.

This was most commonly a problem with people doing fsid=0 exports, so
it's less of a problem now that we have the v4root stuff, but definitely
it still needs fixing.

--b.