I think we never noticed because a) the use_ipaddr case is rarer than the !use_ipaddr case, and b) the nfsd_fh upcall does a preemptive export downcall that renders the nfsd export call unnecessary in some cases.

Cc Chuck Lever <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 utils/mountd/cache.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

utils/mountd/cache.c +1 -0

index ac9cdbd..cf07b56 100644
@@ -1064,6 +1064,7 @@ static void nfsd_export(FILE *f)
 			goto out;
 		ai = client_resolve(tmp->ai_addr);
 		freeaddrinfo(tmp);
+		if (!ai)
 			goto out;
 	}

Signature · 1 line

1.7.7.6

Steve Dickson

2012-05-08 14:08 UTC #2 lore raw

Re: [PATCH 1/5] mountd: fix export upcall failure in use_ipaddr case.

Quoted 11 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>
After 0509d3428f523 "mountd: Replace "struct hostent" with "struct addinfo"", the export upcall fails in the use_ipaddr case.
I think we never noticed because a) the use_ipaddr case is rarer than the !use_ipaddr case, and b) the nfsd_fh upcall does a preemptive export downcall that renders the nfsd export call unnecessary in some cases.
Cc Chuck Lever <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>

All 5 committed....

steved.

Quoted 16 lines

---
 utils/mountd/cache.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

utils/mountd/cache.c +1 -0

index ac9cdbd..cf07b56 100644
@@ -1064,6 +1064,7 @@ static void nfsd_export(FILE *f)
 			goto out;
 		ai = client_resolve(tmp->ai_addr);
 		freeaddrinfo(tmp);
+		if (!ai)
 			goto out;
 	}

J. Bruce Fields

author

2012-05-02 21:56 UTC #3 lore raw

PATCH 2/5 mountd: parse ip address earlier

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

I don't see the point of waiting to the last minute to parse the ip address. If the client name isn't a legal ip address then this will fail fairly quickly, so there's not much of a performance penalty.

Also, note the previous code incorrectly assumed client_resolve would always return non-NULL.

Also factor out some common code.

Signed-off-by J. Bruce Fields <[email protected]>

---
 utils/mountd/cache.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

utils/mountd/cache.c +22 ~~-18~~

index cf07b56..d2ac258 100644
@@ -495,6 +495,19 @@ static bool match_fsid(struct parsed_fsid *parsed, nfs_export *exp, char *path)
 	return false;
 }
 
+struct addrinfo *lookup_client_addr(char *dom)
+{
+	struct addrinfo *ret;
+	struct addrinfo *tmp;
+
+	tmp = host_pton(dom);
+	if (tmp == NULL)
+		return NULL;
+	ret = client_resolve(tmp->ai_addr);
+	freeaddrinfo(tmp);
+	return ret;
+}
+
 static void nfsd_fh(FILE *f)
 {
 	/* request are:

@@ -538,6 +551,12 @@ static void nfsd_fh(FILE *f)
 
 	auth_reload();
 
+	if (use_ipaddr) {
+		ai = lookup_client_addr(dom);
+		if (!ai)
+			goto out;
+	}
+
 	/* Now determine export point for this fsid/domain */
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
 		nfs_export *next_exp;

@@ -578,18 +597,8 @@ static void nfsd_fh(FILE *f)
 
 			if (!match_fsid(&parsed, exp, path))
 				continue;
-			if (use_ipaddr) {
-				if (ai == NULL) {
-					struct addrinfo *tmp;
-					tmp = host_pton(dom);
-					if (tmp == NULL)
-						goto out;
-					ai = client_resolve(tmp->ai_addr);
-					freeaddrinfo(tmp);
-				}
-				if (!client_check(exp->m_client, ai))
-					continue;
-			}
+			if (use_ipaddr && !client_check(exp->m_client, ai))
+				continue;
 			if (!found || subexport(&exp->m_export, found)) {
 				found = &exp->m_export;
 				free(found_path);

@@ -1058,12 +1067,7 @@ static void nfsd_export(FILE *f)
 	auth_reload();
 
 	if (use_ipaddr) {
-		struct addrinfo *tmp;
-		tmp = host_pton(dom);
-		if (tmp == NULL)
-			goto out;
-		ai = client_resolve(tmp->ai_addr);
-		freeaddrinfo(tmp);
+		ai = lookup_client_addr(dom);
 		if (!ai)
 			goto out;
 	}

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-05-02 21:56 UTC #4 lore raw

PATCH 3/5 mountd: add trivial helpers for client-matching

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Pull out a tiny bit of common logic from three functions.

Possibly minor overkill, but simplifies the next patch.

Signed-off-by J. Bruce Fields <[email protected]>

---
 utils/mountd/auth.c   |   21 ++++++++++++++++++---
 utils/mountd/cache.c  |   12 ++----------
 utils/mountd/mountd.h |    4 ++++
 3 files changed, 24 insertions(+), 13 deletions(-)

utils/mountd/auth.c +18 -3

index ccc849a..1ed9a4b 100644
@@ -131,6 +131,23 @@ get_client_hostname(const struct sockaddr *caller, struct addrinfo *ai,
 	return strdup("DEFAULT");
 }
 
+bool ipaddr_client_matches(nfs_export *exp, struct addrinfo *ai)
+{
+	return client_check(exp->m_client, ai);
+}
+
+bool namelist_client_matches(nfs_export *exp, char *dom)
+{
+	return client_member(dom, exp->m_client->m_hostname);
+}
+
+bool client_matches(nfs_export *exp, char *dom, struct addrinfo *ai)
+{
+	if (use_ipaddr)
+		return ipaddr_client_matches(exp, ai);
+	return namelist_client_matches(exp, dom);
+}
+
 /* return static nfs_export with details filled in */
 static nfs_export *
 auth_authenticate_newcache(const struct sockaddr *caller,

@@ -155,9 +172,7 @@ auth_authenticate_newcache(const struct sockaddr *caller,
 		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
 			if (strcmp(path, exp->m_export.e_path))
 				continue;
-			if (!use_ipaddr && !client_member(my_client.m_hostname, exp->m_client->m_hostname))
-				continue;
-			if (use_ipaddr && !client_check(exp->m_client, ai))
+			if (!client_matches(exp, my_client.m_hostname, ai))
 				continue;
 			break;
 		}

utils/mountd/cache.c +2 ~~-10~~

index d2ac258..6e01b2f 100644
@@ -587,7 +587,7 @@ static void nfsd_fh(FILE *f)
 				next_exp = exp->m_next;
 			}
 
-			if (!use_ipaddr && !client_member(dom, exp->m_client->m_hostname))
+			if (!use_ipaddr && !namelist_client_matches(exp, dom))
 				continue;
 			if (exp->m_export.e_mountpoint &&
 			    !is_mountpoint(exp->m_export.e_mountpoint[0]?

@@ -597,7 +597,7 @@ static void nfsd_fh(FILE *f)
 
 			if (!match_fsid(&parsed, exp, path))
 				continue;
-			if (use_ipaddr && !client_check(exp->m_client, ai))
+			if (use_ipaddr && !ipaddr_client_matches(exp, ai))
 				continue;
 			if (!found || subexport(&exp->m_export, found)) {
 				found = &exp->m_export;

@@ -751,14 +751,6 @@ static int path_matches(nfs_export *exp, char *path)
 }
 
 static int
-client_matches(nfs_export *exp, char *dom, struct addrinfo *ai)
-{
-	if (use_ipaddr)
-		return client_check(exp->m_client, ai);
-	return client_member(dom, exp->m_client->m_hostname);
-}
-
-static int
 export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai)
 {
 	return path_matches(exp, path) && client_matches(exp, dom, ai);

utils/mountd/mountd.h +4 -0

index 4c184d2..c969a27 100644
@@ -56,4 +56,8 @@ struct nfs_fh_len *
 		cache_get_filehandle(nfs_export *exp, int len, char *p);
 int		cache_export(nfs_export *exp, char *path);
 
+bool ipaddr_client_matches(nfs_export *exp, struct addrinfo *ai);
+bool namelist_client_matches(nfs_export *exp, char *dom);
+bool client_matches(nfs_export *exp, char *dom, struct addrinfo *ai);
+
 #endif /* MOUNTD_H */

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-05-02 21:56 UTC #5 lore raw

PATCH 4/5 mountd: prepend '$' to make use_ipaddr clients self-describing

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Mountd is responsible for filling three interrelated kernel caches:

- auth_unix_ip maps an incoming ip addresses to a "domain".
- nfsd_fh maps (domain, filehandle-fragment) pairs to paths.
- nfsd_export maps (domain, path) pairs to export options.

Note that each export is assocated with a "client" string--the part before the parentheses in an /etc/export line--which may be a domain name, a netgroup, etc.

The "domain" string in the above three caches may be either:

- in the !use_ipaddr case, a comma-separated list of client strings.
- in the use_ipaddr case, an ip address.

In the former case, mountd does the hard work of matching an ip address to the clients when doing the auth_unix_ip mapping. In the latter case, it delays that until the nfsd_fh or nfsd_export upcall.

We're currently depending on being able to flush the kernel caches completely when switching between the use_ipaddr and !use_ipaddr cases. However, the kernel's cache-flushing doesn't really provide reliable guarantees on return; it's still possible we could see nfsd_fh or nfsd_export upcalls with the old domain-type after flushing.

So, instead, make the two domain types self-describing by prepending a "$" in the use_ipaddr case.

Reviewed-by NeilBrown <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 utils/mountd/auth.c   |   14 +++++++++++---
 utils/mountd/cache.c  |   12 ++++++++----
 utils/mountd/mountd.h |    5 +++++
 3 files changed, 24 insertions(+), 7 deletions(-)

utils/mountd/auth.c +11 -3

index 1ed9a4b..15da54c 100644
@@ -112,15 +112,23 @@ auth_reload()
 	return counter;
 }
 
+static char *get_client_ipaddr_name(const struct sockaddr *caller)
+{
+	char buf[INET6_ADDRSTRLEN + 1];
+
+	buf[0] = '$';
+	host_ntop(caller, buf + 1, sizeof(buf) - 1);
+	return strdup(buf);
+}
+
 static char *
 get_client_hostname(const struct sockaddr *caller, struct addrinfo *ai,
 		enum auth_error *error)
 {
-	char buf[INET6_ADDRSTRLEN];
 	char *n;
 
 	if (use_ipaddr)
-		return strdup(host_ntop(caller, buf, sizeof(buf)));
+		return get_client_ipaddr_name(caller);
 	n = client_compose(ai);
 	*error = unknown_host;
 	if (!n)

@@ -143,7 +151,7 @@ bool namelist_client_matches(nfs_export *exp, char *dom)
 
 bool client_matches(nfs_export *exp, char *dom, struct addrinfo *ai)
 {
-	if (use_ipaddr)
+	if (is_ipaddr_client(dom))
 		return ipaddr_client_matches(exp, ai);
 	return namelist_client_matches(exp, dom);
 }

utils/mountd/cache.c +8 -4

index 6e01b2f..6710eca 100644
@@ -500,6 +500,8 @@ struct addrinfo *lookup_client_addr(char *dom)
 	struct addrinfo *ret;
 	struct addrinfo *tmp;
 
+	dom++; /* skip initial "$" */
+
 	tmp = host_pton(dom);
 	if (tmp == NULL)
 		return NULL;

@@ -551,7 +553,7 @@ static void nfsd_fh(FILE *f)
 
 	auth_reload();
 
-	if (use_ipaddr) {
+	if (is_ipaddr_client(dom)) {
 		ai = lookup_client_addr(dom);
 		if (!ai)
 			goto out;

@@ -587,7 +589,8 @@ static void nfsd_fh(FILE *f)
 				next_exp = exp->m_next;
 			}
 
-			if (!use_ipaddr && !namelist_client_matches(exp, dom))
+			if (!is_ipaddr_client(dom)
+					&& !namelist_client_matches(exp, dom))
 				continue;
 			if (exp->m_export.e_mountpoint &&
 			    !is_mountpoint(exp->m_export.e_mountpoint[0]?

@@ -597,7 +600,8 @@ static void nfsd_fh(FILE *f)
 
 			if (!match_fsid(&parsed, exp, path))
 				continue;
-			if (use_ipaddr && !ipaddr_client_matches(exp, ai))
+			if (is_ipaddr_client(dom)
+					&& !ipaddr_client_matches(exp, ai))
 				continue;
 			if (!found || subexport(&exp->m_export, found)) {
 				found = &exp->m_export;

@@ -1058,7 +1062,7 @@ static void nfsd_export(FILE *f)
 
 	auth_reload();
 
-	if (use_ipaddr) {
+	if (is_ipaddr_client(dom)) {
 		ai = lookup_client_addr(dom);
 		if (!ai)
 			goto out;

utils/mountd/mountd.h +5 -0

index c969a27..6d358a7 100644
@@ -60,4 +60,9 @@ bool ipaddr_client_matches(nfs_export *exp, struct addrinfo *ai);
 bool namelist_client_matches(nfs_export *exp, char *dom);
 bool client_matches(nfs_export *exp, char *dom, struct addrinfo *ai);
 
+static inline bool is_ipaddr_client(char *dom)
+{
+	return dom[0] == '$';
+}
+
 #endif /* MOUNTD_H */

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-05-02 21:56 UTC #6 lore raw

PATCH 5/5 mountd: handle allocation failures in auth_unix_ip upcall

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 utils/mountd/cache.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

utils/mountd/cache.c +9 -3

index 6710eca..8f14032 100644
@@ -84,7 +84,6 @@ static void auth_unix_ip(FILE *f)
 	char ipaddr[INET6_ADDRSTRLEN];
 	char *client = NULL;
 	struct addrinfo *tmp = NULL;
-	struct addrinfo *ai = NULL;
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;

@@ -107,12 +106,16 @@ static void auth_unix_ip(FILE *f)
 
 	/* addr is a valid, interesting address, find the domain name... */
 	if (!use_ipaddr) {
+		struct addrinfo *ai = NULL;
+
 		ai = client_resolve(tmp->ai_addr);
+		if (ai == NULL)
+			goto out;
 		client = client_compose(ai);
 		freeaddrinfo(ai);
+		if (!client)
+			goto out;
 	}
-	freeaddrinfo(tmp);
-
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);
 	qword_printuint(f, time(0) + DEFAULT_TTL);

@@ -124,6 +127,9 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
+out:
+	freeaddrinfo(tmp);
+
 }
 
 static void auth_unix_gid(FILE *f)

Signature · 1 line

1.7.7.6