Fix use_ipaddr race

PATCH In review J. Bruce Fields 3 participants 16 messages Apr 20, 2012 – May 2, 2012

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2012-04-20 22:46 UTC #0 lore raw

cover Fix use_ipaddr race

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

We have a report of failed upcalls that occur when use_ipaddr is toggled. The problem appears to be that for example after turning on use_ipaddr, the kernel may still see upcalls for clients such as "*".

The following patches (untested except to check that they compile...) attempt to fix that by just letting nfsd_fh() accept either client type; does anyone see a problem with that?

The current code actually attempts to avoid tihs problem by flushing caches on a use_ipaddr change (see the cache_flush() call in utils/mountd/auth.c:check_useipaddr(). But that doesn't work, because a write to a cache/flush file doesn't actually provide useful guarantees to the caller on return:

- as far as I can tell, cache_clean leaves alone any entries that were created in the current second.
- cache_clean only removes entries from the cache, it doesn't wait for them to actually be destroyed: so an in-progress nfsd thread could still make an upcall using information from one of the flushed entries.

These seem like bugs in their own right to me: a cache-flush operation that actually guaranteed the entries were gone on return would be more useful. And I wonder whether this doesn't also cause exportfs bugs....

I'm not sure what to do about it, though.

I don't think the existing interface is really fixable, since fixing the second problem above would (I think) require the cache/flush write to wait on in-progress rpc's, but those in-progress rpc's could be waiting on mountd, creating a deadlock.

A new interface shouldn't need to accept a time--every actual user just wants to cache flushed now.

Maybe the first problem could be solved by replacing the time by a counter incremented on each insert of a cache entry.

And the second could be fixed by waiting on in-progress rpc's. That might not help mountd, but it would help exportfs at least.

--b.

J. Bruce Fields (3):
mountd: unconditionally resolve ip address
mountd: helper function for export upcall's client matching
mountd: ignore use_ipaddr and just try both client types

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

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-04-20 22:46 UTC #1 lore raw

PATCH 1/3 mountd: unconditionally resolve ip address

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

I don't see the point of waiting to the last minute to resolve an 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.

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

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

utils/mountd/cache.c +16 -9

index ac9cdbd..0d2f76c 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,8 @@ static void nfsd_fh(FILE *f)
 
 	auth_reload();
 
+	ai = lookup_client_addr(dom);
+
 	/* Now determine export point for this fsid/domain */
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
 		nfs_export *next_exp;

@@ -579,15 +594,7 @@ 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))
+				if (ai && !client_check(exp->m_client, ai))
 					continue;
 			}
 			if (!found || subexport(&exp->m_export, found)) {

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-04-20 22:46 UTC #2 lore raw

PATCH 2/3 mountd: helper function for export upcall's client matching

In reply to J. Bruce Fields

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

This is just minor cleanup. It also reorders the tests slightly--perhaps there was some performance advantage to doing them the other way, but if so I doubt it's significant.

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

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

utils/mountd/cache.c +9 -5

index 0d2f76c..a6bad07 100644
@@ -495,6 +495,14 @@ static bool match_fsid(struct parsed_fsid *parsed, nfs_export *exp, char *path)
 	return false;
 }
 
+static bool match_client(char *dom, nfs_export *exp, struct addinfo *ai)
+{
+	if (!use_ipaddr && client_member(dom, exp->m_client->m_hostname))
+		return true;
+	if (use_ipaddr && ai && client_check(exp->m_client, ai))
+		return true;
+}
+
 struct addrinfo *lookup_client_addr(char *dom)
 {
 	struct addrinfo *ret;

@@ -583,7 +591,7 @@ static void nfsd_fh(FILE *f)
 				next_exp = exp->m_next;
 			}
 
-			if (!use_ipaddr && !client_member(dom, exp->m_client->m_hostname))
+			if (!match_client(dom, exp, ai))
 				continue;
 			if (exp->m_export.e_mountpoint &&
 			    !is_mountpoint(exp->m_export.e_mountpoint[0]?

@@ -593,10 +601,6 @@ static void nfsd_fh(FILE *f)
 
 			if (!match_fsid(&parsed, exp, path))
 				continue;
-			if (use_ipaddr) {
-				if (ai && !client_check(exp->m_client, ai))
-					continue;
-			}
 			if (!found || subexport(&exp->m_export, found)) {
 				found = &exp->m_export;
 				free(found_path);

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-04-20 22:46 UTC #3 lore raw

PATCH 3/3 mountd: ignore use_ipaddr and just try both client types

In reply to J. Bruce Fields

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

There are races when switching use_ipaddr on or off: it's possible the kernel may still do export upcalls with the previous client type. So, let's just match either one.

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

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

utils/mountd/cache.c +4 -3

index a6bad07..181922b 100644
@@ -495,12 +495,13 @@ static bool match_fsid(struct parsed_fsid *parsed, nfs_export *exp, char *path)
 	return false;
 }
 
-static bool match_client(char *dom, nfs_export *exp, struct addinfo *ai)
+static bool match_client(char *dom, nfs_export *exp, struct addrinfo *ai)
 {
-	if (!use_ipaddr && client_member(dom, exp->m_client->m_hostname))
+	if (client_member(dom, exp->m_client->m_hostname))
 		return true;
-	if (use_ipaddr && ai && client_check(exp->m_client, ai))
+	if (ai && client_check(exp->m_client, ai))
 		return true;
+	return false;
 }
 
 struct addrinfo *lookup_client_addr(char *dom)

Signature · 1 line

1.7.7.6

NeilBrown

2012-04-23 01:04 UTC #4 lore raw

In reply to J. Bruce Fields

Quoted 11 lines · On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <[email protected]> wrote:

From: "J. Bruce Fields" <[email protected]>
We have a report of failed upcalls that occur when use_ipaddr is toggled. The problem appears to be that for example after turning on use_ipaddr, the kernel may still see upcalls for clients such as "*".
The following patches (untested except to check that they compile...) attempt to fix that by just letting nfsd_fh() accept either client type; does anyone see a problem with that?

Quoted 8 lines

The current code actually attempts to avoid tihs problem by flushing caches on a use_ipaddr change (see the cache_flush() call in utils/mountd/auth.c:check_useipaddr(). But that doesn't work, because a write to a cache/flush file doesn't actually provide useful guarantees to the caller on return:
- as far as I can tell, cache_clean leaves alone any entries that were created in the current second.

We only do one thing each second - right?

Maybe a simple work around is:
- update state in mountd
- sleep(1)
- flush cache.

?? Then anything dependant on the old behaviour will be gone.

Quoted 4 lines

- cache_clean only removes entries from the cache, it doesn't wait for them to actually be destroyed: so an in-progress nfsd thread could still make an upcall using information from one of the flushed entries.

Ideally we would like those threads to abort and re-start from the beginning. That's not really very easy with all the NFSv4 state issues is it?

I think waiting for them to complete would be problematic. As you say, deadlocks and such.
So we need to continue to serve them faithfully.

Does that just mean changing nfsd_fh to *not* test use_ipaddr, but rather to inspect the 'dom' and if it looks like an IP address, act as though use_ipaddr was set, else not?

Quoted 7 lines

These seem like bugs in their own right to me: a cache-flush operation that actually guaranteed the entries were gone on return would be more useful. And I wonder whether this doesn't also cause exportfs bugs....
I'm not sure what to do about it, though.

Quoted 7 lines

I don't think the existing interface is really fixable, since fixing the second problem above would (I think) require the cache/flush write to wait on in-progress rpc's, but those in-progress rpc's could be waiting on mountd, creating a deadlock.
A new interface shouldn't need to accept a time--every actual user just wants to cache flushed now.

Having a time-stamp seems like a good idea at the time .... I don't remember why though. :-(

Quoted 11 lines

Maybe the first problem could be solved by replacing the time by a counter incremented on each insert of a cache entry.
And the second could be fixed by waiting on in-progress rpc's. That might not help mountd, but it would help exportfs at least.
--b.
J. Bruce Fields (3):
mountd: unconditionally resolve ip address

Not a good idea. If /etc/exports only contains IP address and subnets, then we don't ever want to do any address resolution. The "resolve ip address" must at least be conditional on "are there any domain-name, wild-cards, netgroups in /etc/exports".

Quoted 1 line

mountd: helper function for export upcall's client matching

This depends on the earlier patch? and does the host-name lookup earlier. That really can cause a delay sometimes so it should be as late as possible.

Quoted 1 line

mountd: ignore use_ipaddr and just try both client types

Maybe ... though if we could syntactically distinguish "use_ipaddr" domains from "!use_ipaddr" domain and still just choose one test to perform, I think I'd prefer that.

NeilBrown

J. Bruce Fields

2012-04-28 11:26 UTC #5 lore raw

Quoted 9 lines · NeilBrown

Quoted 1 line · On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <[email protected]> wrote:
mountd: unconditionally resolve ip address
Not a good idea. If /etc/exports only contains IP address and subnets, then we don't ever want to do any address resolution. The "resolve ip address" must at least be conditional on "are there any domain-name, wild-cards, netgroups in /etc/exports".

The bug is my changelog. All we're really doing there is parsing the ip address, not resolving anything.

...

Quoted 5 lines

Quoted 1 line
mountd: ignore use_ipaddr and just try both client types
Maybe ... though if we could syntactically distinguish "use_ipaddr" domains from "!use_ipaddr" domain and still just choose one test to perform, I think I'd prefer that.

Hm, OK. Something like the following? (Totally untested.)

--b.

J. Bruce Fields

author

2012-04-28 11:28 UTC #6 lore raw

PATCH 1/3 mountd: parse ip address earlier

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.

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

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

utils/mountd/cache.c +21 ~~-12~~

index ac9cdbd..08ff150 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);

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-04-28 11:28 UTC #7 lore raw

PATCH 2/3 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   |   20 +++++++++++++++++---
 utils/mountd/cache.c  |   14 +++-----------
 utils/mountd/mountd.h |    4 ++++
 3 files changed, 24 insertions(+), 14 deletions(-)

utils/mountd/auth.c +17 -3

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

@@ -155,9 +171,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(my_client.m_hostname, exp, ai))
 				continue;
 			break;
 		}

utils/mountd/cache.c +3 ~~-11~~

index 08ff150..02d5313 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 (!namelist_client_matches(dom, exp, ai))
 				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 (!ipaddr_client_matches(dom, exp, ai))
 				continue;
 			if (!found || subexport(&exp->m_export, found)) {
 				found = &exp->m_export;

@@ -751,17 +751,9 @@ 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);
+	return path_matches(exp, path) && client_matches(dom, exp, ai);
 }
 
 static nfs_export *

utils/mountd/mountd.h +4 -0

index 4c184d2..189db7e 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(char *dom, nfs_export *exp, struct addrinfo *ai);
+bool namelist_client_matches(char *dom, nfs_export *exp, struct addrinfo *ai);
+bool client_matches(char *dom, nfs_export *exp, struct addrinfo *ai);
+
 #endif /* MOUNTD_H */

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-04-28 11:28 UTC #8 lore raw

PATCH 3/3 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.

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

---
 utils/mountd/auth.c  |   16 ++++++++++++----
 utils/mountd/cache.c |    2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

utils/mountd/auth.c +12 -4

index 7aa00c4..f5bdfa7 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)

@@ -133,12 +141,12 @@ get_client_hostname(const struct sockaddr *caller, struct addrinfo *ai,
 
 bool ipaddr_client_matches(char *dom, nfs_export *exp, struct addrinfo *ai)
 {
-	return use_ipaddr && client_check(exp->m_client, ai);
+	return (dom[0] == '?') && client_check(exp->m_client, ai);
 }
 
 bool namelist_client_matches(char *dom, nfs_export *exp, struct addrinfo *ai)
 {
-	return !use_ipaddr && client_member(dom, exp->m_client->m_hostname);
+	return (dom[0] != '?') && client_member(dom, exp->m_client->m_hostname);
 }
 
 bool client_matches(char *dom, nfs_export *exp, struct addrinfo *ai)

utils/mountd/cache.c +1 -1

index 02d5313..0e270ba 100644
@@ -551,7 +551,7 @@ static void nfsd_fh(FILE *f)
 
 	auth_reload();
 
-	if (use_ipaddr) {
+	if (dom[0] == '?') {
 		ai = lookup_client_addr(dom);
 		if (!ai)
 			goto out;

Signature · 1 line

1.7.7.6

NeilBrown

2012-04-28 11:47 UTC #9 lore raw

In reply to J. Bruce Fields

Quoted 6 lines · On Sat, 28 Apr 2012 07:26:39 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 5 lines · NeilBrown
Quoted 1 line · On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <[email protected]> wrote:
mountd: unconditionally resolve ip address

Quoted 7 lines

Quoted 4 lines
Not a good idea. If /etc/exports only contains IP address and subnets, then we don't ever want to do any address resolution. The "resolve ip address" must at least be conditional on "are there any domain-name, wild-cards, netgroups in /etc/exports".
The bug is my changelog. All we're really doing there is parsing the ip address, not resolving anything.

Ahhh. I see that now. Thanks.

Quoted 9 lines

...
Quoted 5 lines
Quoted 1 line
mountd: ignore use_ipaddr and just try both client types
Maybe ... though if we could syntactically distinguish "use_ipaddr" domains from "!use_ipaddr" domain and still just choose one test to perform, I think I'd prefer that.
Hm, OK. Something like the following? (Totally untested.)

Yes, that looks good.

I could probably bike-shed for a while about the leading '?', and wonder if '!' or '$' might be better choices, or if leading '[' and trailing ']' is best but I won't. He who writes the code makes the choice.

Thanks,
NeilBrown

J. Bruce Fields

2012-04-28 15:59 UTC #10 lore raw

Quoted 27 lines · NeilBrown

Quoted 6 lines · On Sat, 28 Apr 2012 07:26:39 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 5 lines · NeilBrown
Quoted 1 line · On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <[email protected]> wrote:
mountd: unconditionally resolve ip address
Quoted 7 lines
Quoted 4 lines
Not a good idea. If /etc/exports only contains IP address and subnets, then we don't ever want to do any address resolution. The "resolve ip address" must at least be conditional on "are there any domain-name, wild-cards, netgroups in /etc/exports".
The bug is my changelog. All we're really doing there is parsing the ip address, not resolving anything.
Ahhh. I see that now. Thanks.
Quoted 8 lines
...
Quoted 5 lines
Quoted 1 line
mountd: ignore use_ipaddr and just try both client types
Maybe ... though if we could syntactically distinguish "use_ipaddr" domains from "!use_ipaddr" domain and still just choose one test to perform, I think I'd prefer that.

Quoted 7 lines

Quoted 1 line
Hm, OK. Something like the following? (Totally untested.)
Yes, that looks good.
I could probably bike-shed for a while about the leading '?', and wonder if '!' or '$' might be better choices, or if leading '[' and trailing ']' is best but I won't. He who writes the code makes the choice.

Hah. I think I had some vague idea like "the ? reminds us there's still some question to be answered about this". Anyway, my one worry is that I'm not sure what the syntax of the existing client types.

Hm, the documentation says "?" and "[character set]" can be used in wildcard domains, so maybe we do want '!' or '$'.

--b.

J. Bruce Fields

2012-05-02 01:41 UTC #11 lore raw

Quoted 6 lines · J. Bruce Fields

Hah. I think I had some vague idea like "the ? reminds us there's still some question to be answered about this". Anyway, my one worry is that I'm not sure what the syntax of the existing client types.
Hm, the documentation says "?" and "[character set]" can be used in wildcard domains, so maybe we do want '!' or '$'.

OK, going with '$', and also fixing a logic error found in testing, I get the following.

I've done simple mounts but *haven't* actually tested the use_ipaddr case; I'll report results when I get them.

It might be reasonable to go ahead and apply them anyway; up to you.

--b.

J. Bruce Fields

author

2012-05-02 01:43 UTC #12 lore raw

PATCH 1/3 mountd: parse ip address earlier

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

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

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

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

utils/mountd/cache.c +21 ~~-12~~

index ac9cdbd..08ff150 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);

Signature · 1 line

1.7.7.6

J. Bruce Fields

author

2012-05-02 01:43 UTC #13 lore raw

PATCH 2/3 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 08ff150..6cc58d2 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 01:43 UTC #14 lore raw

PATCH 3/3 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.

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

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

---
 utils/mountd/auth.c   |   14 +++++++++++---
 utils/mountd/cache.c  |    8 +++++---
 utils/mountd/mountd.h |    5 +++++
 3 files changed, 21 insertions(+), 6 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 +5 -3

index 6cc58d2..1833b3a 100644
@@ -551,7 +551,7 @@ static void nfsd_fh(FILE *f)
 
 	auth_reload();
 
-	if (use_ipaddr) {
+	if (dom[0] == '$') {
 		ai = lookup_client_addr(dom);
 		if (!ai)
 			goto out;

@@ -587,7 +587,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 +598,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;

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

NeilBrown

Reviewed-by on v1 3/3

2012-05-02 02:07 UTC #15 lore raw

Re: [PATCH 3/3] mountd: prepend '$' to make use_ipaddr clients self-describing

utils/mountd/cache.c static void nfsd_fh(FILE *f) · line 554

Quoted 10 lines · On Tue, 1 May 2012 21:43:34 -0400 "J. Bruce Fields" <[email protected]> wrote:

utils/mountd/cache.c +1 -1

index 6cc58d2..1833b3a 100644
@@ -551,7 +551,7 @@ static void nfsd_fh(FILE *f)
 
 	auth_reload();
 
-	if (use_ipaddr) {
+	if (dom[0] == '$') {

        if (is_ipaddr_client(dom)) {

Otherwise:

Reviewed-by NeilBrown <[email protected]>

Thanks,
NeilBrown