[PATCH 0/5] Possible fixes for statd / sm-notify / exportfs

[PATCH 0/5] Possible fixes for statd / sm-notify / exportfs

[Chuck Lever]

While working on another fix, I noticed that the complex hostname
matching logic in statd, exportfs, and sm-notify appears to break with
the local hostname and IP addresses.  I propose the following to
address this.

---

Chuck Lever (5):
      sm-notify: sm-notify doesn't handle localhost properly
      exportfs: matchhostname() doesn't handle localhost properly
      statd: statd_matchhostname() doesn't handle localhost properly
      sm-notify: Disable syslog messages when debugging is enabled
      statd: Report count of loaded hosts correctly


 utils/exportfs/exportfs.c |   34 +++++++++++-
 utils/statd/hostname.c    |   55 ++++++++++++++++---
 utils/statd/monitor.c     |    2 -
 utils/statd/sm-notify.c   |  133 ++++++++++++++++++++++++++++++++++++---------
 4 files changed, 186 insertions(+), 38 deletions(-)

-- 
Chuck Lever

[PATCH 1/5] statd: Report count of loaded hosts correctly

[Chuck Lever]

Fix a debugging message to report correctly the count of hosts loaded
when statd starts up.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/statd/monitor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/statd/monitor.c b/utils/statd/monitor.c
index 325dfd3..286a5e2 100644
--- a/utils/statd/monitor.c
+++ b/utils/statd/monitor.c
@@ -249,7 +249,7 @@ void load_state(void)
 
 	count = nsm_load_monitor_list(load_one_host);
 	if (count)
-		xlog(D_GENERAL, "Loaded %u previously monitored hosts");
+		xlog(D_GENERAL, "Loaded %u previously monitored hosts", count);
 }
 
 /*

[PATCH 2/5] sm-notify: Disable syslog messages when debugging is enabled

[Chuck Lever]

statd's "-F" flag disables syslog output, and specifies sm-notify's
"-d" option when it runs it.  sm-notify's "-d" option should therefore
also disable syslog output.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/statd/sm-notify.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 1f490b0..f05eadf 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -395,12 +395,14 @@ usage:		fprintf(stderr,
 		exit(1);
 	}
 
-	xlog_syslog(1);
 	if (opt_debug) {
+		xlog_syslog(0);
 		xlog_stderr(1);
 		xlog_config(D_ALL, 1);
-	} else
+	} else {
+		xlog_syslog(1);
 		xlog_stderr(0);
+	}
 
 	xlog_open(progname);
 	xlog(L_NOTICE, "Version " VERSION " starting");

[PATCH 3/5] statd: statd_matchhostname() doesn't handle localhost properly

[Chuck Lever]

The job of statd_matchhostname() is to work hard at matching two
hostnames or presentation IP addresses that may refer to the same
host.

statd_matchhostname() turns the hostname of the local system into a
list of addresses containing only the loopback address.  The actual
DNS registered address of the system does not appear in that list.

Presentation IP addresses, on the other hand, are soundly ignored by
the AI_CANONNAME option of getaddrinfo(3).  The ai_canonname string
that is returned is just the same presentation IP address.  And the
resulting list of addresses contains just that IP address.

So if the DNS registered IP address of the local host is passed in as
one argument, and the local hostname is passed as the other argument,
statd_matchhostname() whiffs and believes there is no match.  To fix
this, the logic needs to be smarter about deriving a hostname from an
IP address.

This appears to cause no end of trouble: monitor records pile up in
/var/lib/nfs/sm and sm.bak, notifications are missed, and so on.  This
has likely been around since commit cbd3a131 "statd: Introduce statd
version of matchhostname()" (Jan 14, 2010).

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/statd/hostname.c |   55 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/utils/statd/hostname.c b/utils/statd/hostname.c
index 616a3cb..746ecc7 100644
--- a/utils/statd/hostname.c
+++ b/utils/statd/hostname.c
@@ -225,6 +225,49 @@ statd_canonical_name(const char *hostname)
 	return strdup(buf);
 }
 
+/*
+ * Take care to perform an explicit reverse lookup on presentation
+ * addresses.  Otherwise we don't get a real canonical name or a
+ * complete list of addresses.
+ *
+ * Returns an addrinfo list that has ai_canonname filled in, or
+ * NULL if some error occurs.  Caller must free the returned
+ * list with freeaddrinfo(3).
+ */
+__attribute_malloc__
+static struct addrinfo *
+statd_canonical_list(const char *hostname)
+{
+	struct addrinfo hint = {
+#ifdef IPV6_SUPPORTED
+		.ai_family	= AF_UNSPEC,
+#else	/* !IPV6_SUPPORTED */
+		.ai_family	= AF_INET,
+#endif	/* !IPV6_SUPPORTED */
+		.ai_flags	= AI_NUMERICHOST,
+		.ai_protocol	= (int)IPPROTO_UDP,
+	};
+	char buf[NI_MAXHOST];
+	struct addrinfo *ai;
+
+	ai = get_addrinfo(hostname, &hint);
+	if (ai != NULL) {
+		/* @hostname was a presentation address */
+		_Bool result;
+		result = get_nameinfo(ai->ai_addr, ai->ai_addrlen,
+					buf, (socklen_t)sizeof(buf));
+		freeaddrinfo(ai);
+		if (result)
+			goto out;
+	}
+	/* @hostname was a hostname or had no reverse mapping */
+	strcpy(buf, hostname);
+
+out:
+	hint.ai_flags = AI_CANONNAME;
+	return get_addrinfo(buf, &hint);
+}
+
 /**
  * statd_matchhostname - check if two hostnames are equivalent
  * @hostname1: C string containing hostname
@@ -241,11 +284,6 @@ _Bool
 statd_matchhostname(const char *hostname1, const char *hostname2)
 {
 	struct addrinfo *ai1, *ai2, *results1 = NULL, *results2 = NULL;
-	struct addrinfo hint = {
-		.ai_family	= AF_UNSPEC,
-		.ai_flags	= AI_CANONNAME,
-		.ai_protocol	= (int)IPPROTO_UDP,
-	};
 	_Bool result = false;
 
 	if (strcasecmp(hostname1, hostname2) == 0) {
@@ -253,10 +291,10 @@ statd_matchhostname(const char *hostname1, const char *hostname2)
 		goto out;
 	}
 
-	results1 = get_addrinfo(hostname1, &hint);
+	results1 = statd_canonical_list(hostname1);
 	if (results1 == NULL)
 		goto out;
-	results2 = get_addrinfo(hostname2, &hint);
+	results2 = statd_canonical_list(hostname2);
 	if (results2 == NULL)
 		goto out;
 
@@ -276,7 +314,8 @@ out:
 	freeaddrinfo(results2);
 	freeaddrinfo(results1);
 
-	xlog(D_CALL, "%s: hostnames %s", __func__,
+	xlog(D_CALL, "%s: hostnames %s and %s %s", __func__,
+			hostname1, hostname2,
 			(result ? "matched" : "did not match"));
 	return result;
 }

[PATCH 4/5] exportfs: matchhostname() doesn't handle localhost properly

[Chuck Lever]

Same change as statd_matchhostname() is necessary for the logic in
exportfs.

Recall that these are "separate but nearly equal" because the exportfs
version requires extra expensive string checking that would be onerous
for statd.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/exportfs/exportfs.c |   34 ++++++++++++++++++++++++++++++++--
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index b107c7c..12e8bf1 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -448,6 +448,36 @@ is_hostname(const char *sp)
 	return true;
 }
 
+/*
+ * Take care to perform an explicit reverse lookup on presentation
+ * addresses.  Otherwise we don't get a real canonical name or a
+ * complete list of addresses.
+ */
+static struct addrinfo *
+address_list(const char *hostname)
+{
+	struct addrinfo *ai;
+	char *cname;
+
+	ai = host_pton(hostname);
+	if (ai != NULL) {
+		/* @hostname was a presentation address */
+		cname = host_canonname(ai->ai_addr);
+		freeaddrinfo(ai);
+		if (cname != NULL)
+			goto out;
+	}
+	/* @hostname was a hostname or had no reverse mapping */
+	cname = strdup(hostname);
+	if (cname == NULL)
+		return NULL;
+
+out:
+	ai = host_addrinfo(cname);
+	free(cname);
+	return ai;
+}
+
 static int
 matchhostname(const char *hostname1, const char *hostname2)
 {
@@ -464,10 +494,10 @@ matchhostname(const char *hostname1, const char *hostname2)
 	if (!is_hostname(hostname1) || !is_hostname(hostname2))
 		return 0;
 
-	results1 = host_addrinfo(hostname1);
+	results1 = address_list(hostname1);
 	if (results1 == NULL)
 		goto out;
-	results2 = host_addrinfo(hostname2);
+	results2 = address_list(hostname2);
 	if (results2 == NULL)
 		goto out;
 

[PATCH 5/5] sm-notify: sm-notify doesn't handle localhost properly

[Chuck Lever]

It looks like the existing algorithm for verifying the passed-in bind
address is as broken as statd_matchhostname() used to be: for IP
addresses, AI_CANONNAME is useless.  We need to have getnameinfo(3) or
equivalent in there.

Clean up: extract the logic that verifies the command line bind
address into its own function, and make it handle canonical name
lookup correctly.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/statd/sm-notify.c |  127 ++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index f05eadf..2421f8b 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -93,6 +93,101 @@ smn_lookup(const char *name)
 	return ai;
 }
 
+#ifdef HAVE_GETNAMEINFO
+static char *
+smn_get_hostname(const struct sockaddr *sap, const socklen_t salen,
+		const char *name)
+{
+	char buf[NI_MAXHOST];
+	int error;
+
+	error = getnameinfo(sap, salen, buf, sizeof(buf), NULL, 0, NI_NAMEREQD);
+	if (error != 0) {
+		xlog(L_ERROR, "my_name '%s' is unusable: %s",
+			name, gai_strerror(error));
+		return NULL;
+	}
+	return strdup(buf);
+}
+#else	/* !HAVE_GETNAMEINFO */
+static char *
+smn_get_hostname(const struct sockaddr *sap,
+		__attribute__ ((unused)) const socklen_t salen,
+		const char *name)
+{
+	const struct sockaddr_in *sin = (const struct sockaddr_in *)(char *)sap;
+	const struct in_addr *addr = &sin->sin_addr;
+	struct hostent *hp;
+
+	if (sap->sa_family != AF_INET) {
+		xlog(L_ERROR, "my_name '%s' is unusable: Bad address family",
+			name);
+		return NULL;
+	}
+
+	hp = gethostbyaddr(addr, (socklen_t)sizeof(addr), AF_INET);
+	if (hp == NULL) {
+		xlog(L_ERROR, "my_name '%s' is unusable: %s",
+			name, hstrerror(h_errno));
+		return NULL;
+	}
+	return strdup(hp->h_name);
+}
+#endif	/* !HAVE_GETNAMEINFO */
+
+/*
+ * Presentation addresses are converted to their canonical hostnames.
+ * If the IP address does not map to a hostname, it is an error:
+ * we never send a presentation address as the argument of SM_NOTIFY.
+ *
+ * If "name" is not a presentation address, it is left alone.  This
+ * allows the administrator some flexibility if DNS isn't configured
+ * exactly how sm-notify prefers it.
+ *
+ * Returns NUL-terminated C string containing the result, or NULL
+ * if the canonical name doesn't exist or cannot be determined.
+ * The caller must free the result with free(3).
+ */
+__attribute_malloc__
+static char *
+smn_verify_my_name(const char *name)
+{
+	struct addrinfo *ai = NULL;
+	struct addrinfo hint = {
+#ifdef IPV6_SUPPORTED
+		.ai_family	= AF_UNSPEC,
+#else	/* !IPV6_SUPPORTED */
+		.ai_family	= AF_INET,
+#endif	/* !IPV6_SUPPORTED */
+		.ai_flags	= AI_NUMERICHOST,
+	};
+	char *retval;
+	int error;
+
+	error = getaddrinfo(name, NULL, &hint, &ai);
+	switch (error) {
+	case 0:
+		/* @name was a presentation address */
+		retval = smn_get_hostname(ai->ai_addr, ai->ai_addrlen, name);
+		freeaddrinfo(ai);
+		if (retval == NULL)
+			return NULL;
+		break;
+	case EAI_NONAME:
+		/* @name was not a presentation address */
+		retval = strdup(name);
+		break;
+	default:
+		xlog(L_ERROR, "my_name '%s' is unusable: %s",
+			name, gai_strerror(error));
+		return NULL;
+	}
+
+	xlog(D_GENERAL, "Canonical name for my_name '%s': %s",
+			name, retval);
+	return retval;
+}
+
 __attribute_malloc__
 static struct nsm_host *
 smn_alloc_host(const char *hostname, const char *mon_name,
@@ -416,32 +511,14 @@ usage:		fprintf(stderr,
 	}
 
 	if (opt_srcaddr != NULL) {
-		struct addrinfo *ai = NULL;
-		struct addrinfo hint = {
-			.ai_family	= AF_UNSPEC,
-			.ai_flags	= AI_NUMERICHOST,
-		};
-
-		if (getaddrinfo(opt_srcaddr, NULL, &hint, &ai))
-			/* not a presentation address - use it */
-			strncpy(nsm_hostname, opt_srcaddr, sizeof(nsm_hostname));
-		else {
-			/* was a presentation address - look it up in
-			 * /etc/hosts, so it can be used for my_name */
-			int error;
+		char *name;
 
-			freeaddrinfo(ai);
-			hint.ai_flags = AI_CANONNAME;
-			error = getaddrinfo(opt_srcaddr, NULL, &hint, &ai);
-			if (error != 0) {
-				xlog(L_ERROR, "Bind address %s is unusable: %s",
-						opt_srcaddr, gai_strerror(error));
-				exit(1);
-			}
-			strncpy(nsm_hostname, ai->ai_canonname,
-							sizeof(nsm_hostname));
-			freeaddrinfo(ai);
-		}
+		name = smn_verify_my_name(opt_srcaddr);
+		if (name == NULL)
+			exit(1);
+
+		strncpy(nsm_hostname, name, sizeof(nsm_hostname));
+		free(name);
 	}
 
 	(void)nsm_retire_monitored_hosts();

Re: [PATCH 0/5] Possible fixes for statd / sm-notify / exportfs

[Steve Dickson]

On 08/24/2011 11:33 AM, Chuck Lever wrote:
> While working on another fix, I noticed that the complex hostname
> matching logic in statd, exportfs, and sm-notify appears to break with
> the local hostname and IP addresses.  I propose the following to
> address this.
> 
> ---
> 
> Chuck Lever (5):
>       sm-notify: sm-notify doesn't handle localhost properly
>       exportfs: matchhostname() doesn't handle localhost properly
>       statd: statd_matchhostname() doesn't handle localhost properly
>       sm-notify: Disable syslog messages when debugging is enabled
>       statd: Report count of loaded hosts correctly
> 
> 
>  utils/exportfs/exportfs.c |   34 +++++++++++-
>  utils/statd/hostname.c    |   55 ++++++++++++++++---
>  utils/statd/monitor.c     |    2 -
>  utils/statd/sm-notify.c   |  133 ++++++++++++++++++++++++++++++++++++---------
>  4 files changed, 186 insertions(+), 38 deletions(-)
> 
All 5 patches committed...

steved.