[PATCH 0/3] Fix use_ipaddr race

[PATCH 0/3] Fix use_ipaddr race

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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(-)

-- 
1.7.7.6

[PATCH 1/3] mountd: unconditionally resolve ip address

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/cache.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ac9cdbd..0d2f76c 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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)) {
-- 
1.7.7.6

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

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/cache.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 0d2f76c..a6bad07 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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);
-- 
1.7.7.6

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

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/cache.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index a6bad07..181922b 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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)
-- 
1.7.7.6

Re: [PATCH 0/3] Fix use_ipaddr race

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3743 bytes --]

On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <bfields@redhat.com>
wrote:

> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> 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.

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.

> 	- 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?

> 
> 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.

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

> 
> 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".


>   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.


>   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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/3] Fix use_ipaddr race

[J. Bruce Fields]

On Mon, Apr 23, 2012 at 11:04:19AM +1000, NeilBrown wrote:
> On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <bfields@redhat.com>
> 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.

...
> >   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.

[PATCH 1/3] mountd: parse ip address earlier

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/cache.c |   33 +++++++++++++++++++++------------
 1 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ac9cdbd..08ff150 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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);
-- 
1.7.7.6

[PATCH 2/3] mountd: add trivial helpers for client-matching

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index ccc849a..7aa00c4 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -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;
 		}
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 08ff150..02d5313 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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 *
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index 4c184d2..189db7e 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -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 */
-- 
1.7.7.6

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

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/auth.c  |   16 ++++++++++++----
 utils/mountd/cache.c |    2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 7aa00c4..f5bdfa7 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -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)
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 02d5313..0e270ba 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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;
-- 
1.7.7.6

Re: [PATCH 0/3] Fix use_ipaddr race

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On Sat, 28 Apr 2012 07:26:39 -0400 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Mon, Apr 23, 2012 at 11:04:19AM +1000, NeilBrown wrote:
> > On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <bfields@redhat.com>
> > 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.

Ahhh. I see that now.  Thanks.

> 
> ...
> > >   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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/3] Fix use_ipaddr race

[J. Bruce Fields]

On Sat, Apr 28, 2012 at 09:47:26PM +1000, NeilBrown wrote:
> On Sat, 28 Apr 2012 07:26:39 -0400 "J. Bruce Fields" <bfields@fieldses.org>
> wrote:
> 
> > On Mon, Apr 23, 2012 at 11:04:19AM +1000, NeilBrown wrote:
> > > On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <bfields@redhat.com>
> > > 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.
> 
> Ahhh. I see that now.  Thanks.
> 
> > 
> > ...
> > > >   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.

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.

Re: [PATCH 0/3] Fix use_ipaddr race

[J. Bruce Fields]

On Sat, Apr 28, 2012 at 11:59:17AM -0400, J. Bruce Fields wrote:
> 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.

[PATCH 1/3] mountd: parse ip address earlier

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/cache.c |   33 +++++++++++++++++++++------------
 1 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ac9cdbd..08ff150 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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);
-- 
1.7.7.6

[PATCH 2/3] mountd: add trivial helpers for client-matching

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 utils/mountd/auth.c   |   21 ++++++++++++++++++---
 utils/mountd/cache.c  |   12 ++----------
 utils/mountd/mountd.h |    4 ++++
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index ccc849a..1ed9a4b 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -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;
 		}
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 08ff150..6cc58d2 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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);
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index 4c184d2..c969a27 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -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 */
-- 
1.7.7.6

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

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 1ed9a4b..15da54c 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -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);
 }
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 6cc58d2..1833b3a 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -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;
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index c969a27..6d358a7 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -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 */
-- 
1.7.7.6

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

On Tue,  1 May 2012 21:43:34 -0400 "J. Bruce Fields" <bfields@redhat.com>
wrote:
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 6cc58d2..1833b3a 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -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 <neilb@suse.de>

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]