[PATCH 0/3] Address three recently reported bugs

[PATCH 0/3] Address three recently reported bugs

[Chuck Lever]

Hi Steve-

Some bug fixes for sm-notify and mount.nfs.  Please consider these for
nfs-utils 1.1.7.

---

Chuck Lever (3):
      mount: remove legacy version of nfs_name_to_address()
      sm-notify: Failed DNS lookups should be retried
      sm-notify: Don't orphan addrinfo structs


 utils/mount/network.c   |   88 ++++++++++++-----------------------------------
 utils/mount/network.h   |    3 +-
 utils/mount/nfsumount.c |    2 +
 utils/mount/stropts.c   |   16 ++-------
 utils/statd/sm-notify.c |   73 ++++++++++++++++++++++++++-------------
 5 files changed, 76 insertions(+), 106 deletions(-)

-- 
Chuck Lever <chuck.lever@oracle.com>

[PATCH 1/3] sm-notify: Don't orphan addrinfo structs

[Chuck Lever]

sm-notify orphans an addrinfo struct in its address list rotation
logic if only a single result was returned from getaddrinfo(3).

For each host, the first time through notify_host(), we want to
send a PMAP_GETPORT request.  ->ai is NULL, and retries is set to 100,
forcing a DNS lookup and an address rotation.  If only a single
addrinfo struct is returned, the rotation logic causes a NULL to be
planted in ->ai, copied from the ai_next field of the returned result.

This means that the second time through notify_host() (to perform the
actual SM_NOTIFY call) we do a second DNS lookup, since ->ai is NULL.
The result of the first lookup has been orphaned, and extra network
traffic is generated.

This scenario is actually fairly common.  Since we pass

  .ai_protocol = IPPROTO_UDP,

to getaddrinfo(3), for most hosts, which have a single forward and
reverse pointer in the DNS database, we get back a single addrinfo
struct as a result.

To address this problem, only perform the address list rotation if
there is more than one element on the list returned by getaddrinfo(3).

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

 utils/statd/sm-notify.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index f1fc619..78d0a59 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -424,19 +424,27 @@ notify_host(int sock, struct nsm_host *host)
 	 * point.
 	 */
 	if (host->retries >= 4) {
-		struct addrinfo *first = host->ai;
-		struct addrinfo **next = &host->ai;
-
-		/* remove the first entry from the list */
-		host->ai = first->ai_next;
-		first->ai_next = NULL;
-		/* find the end of the list */
-		next = &first->ai_next;
-		while ( *next )
-			next = & (*next)->ai_next;
-		/* put first entry at end */
-		*next = first;
-		memcpy(&host->addr, first->ai_addr, first->ai_addrlen);
+		/* don't rotate if there is only one addrinfo */
+		if (host->ai->ai_next == NULL)
+			memcpy(&host->addr, host->ai->ai_addr,
+						host->ai->ai_addrlen);
+		else {
+			struct addrinfo *first = host->ai;
+			struct addrinfo **next = &host->ai;
+
+			/* remove the first entry from the list */
+			host->ai = first->ai_next;
+			first->ai_next = NULL;
+			/* find the end of the list */
+			next = &first->ai_next;
+			while ( *next )
+				next = & (*next)->ai_next;
+			/* put first entry at end */
+			*next = first;
+			memcpy(&host->addr, first->ai_addr,
+						first->ai_addrlen);
+		}
+
 		smn_set_port((struct sockaddr *)&host->addr, 0);
 		host->retries = 0;
 	}

[PATCH 2/3] sm-notify: Failed DNS lookups should be retried

[Chuck Lever]

Currently, if getaddrinfo(3) fails when trying to resolve a hostname,
sm-notify gives up immediately on that host.  If sm-notify is started
before network service is available on a system, that means it quits
without notifying anyone.  Or, if DNS service isn't available due to
a network partition or because the DNS server crashed, sm-notify will
simply remove all of its callback files and exit.

Really, sm-notify should try harder.  We know that the hostnames
passed in to notify_host() have already been vetted by statd, which
won't monitor a hostname that it can't resolve.  So it's likely that
any DNS failure we meet here is a temporary condition.  If it isn't,
then sm-notify will stop trying to notify that host in 15 minutes
anyway.

[ The host's file is left in /var/lib/nfs/sm.bak in this case, but
  sm.bak is not read again until the next time sm-notify runs. ]

sm-notify already has retry logic for handling RPC timeouts.  We can
co-opt that to drive DNS resolution retries.

We also add AI_ADDRCONFIG because on systems whose network startup is
handled by NetworkManager, there appears to be a bug that causes
processes that started calling getaddinfo(3) before the network came
up to continue getting EAI_AGAIN even after the network is fully
operating.

As I understand it, legacy glibc (before AI_ADDRCONFIG was exposed in
headers) sets AI_ADDRCONFIG by default, although I haven't checked
this.  In any event, pre-glibc-2.2 systems probably won't run
NetworkManager anyway, so this may not be much of a problem for them.

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

 utils/statd/sm-notify.c |   39 +++++++++++++++++++++++++++------------
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 78d0a59..72dcff4 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -118,17 +118,33 @@ static void smn_set_port(struct sockaddr *sap, const unsigned short port)
 	}
 }
 
-static struct addrinfo *smn_lookup(const sa_family_t family, const char *name)
+static struct addrinfo *smn_lookup(const char *name)
 {
 	struct addrinfo	*ai, hint = {
-		.ai_family	= family,
+#if HAVE_DECL_AI_ADDRCONFIG
+		.ai_flags	= AI_ADDRCONFIG,
+#endif	/* HAVE_DECL_AI_ADDRCONFIG */
+		.ai_family	= AF_INET,
 		.ai_protocol	= IPPROTO_UDP,
 	};
+	int error;
+
+	error = getaddrinfo(name, NULL, &hint, &ai);
+	switch (error) {
+	case 0:
+		return ai;
+	case EAI_SYSTEM: 
+		if (opt_debug)
+			nsm_log(LOG_ERR, "getaddrinfo(3): %s",
+					strerror(errno));
+		break;
+	default:
+		if (opt_debug)
+			nsm_log(LOG_ERR, "getaddrinfo(3): %s",
+					gai_strerror(error));
+	}
 
-	if (getaddrinfo(name, NULL, &hint, &ai) != 0)
-		return NULL;
-
-	return ai;
+	return NULL;
 }
 
 static void smn_forget_host(struct nsm_host *host)
@@ -291,7 +307,7 @@ notify(void)
 
 	/* Bind source IP if provided on command line */
 	if (opt_srcaddr) {
-		struct addrinfo *ai = smn_lookup(AF_INET, opt_srcaddr);
+		struct addrinfo *ai = smn_lookup(opt_srcaddr);
 		if (!ai) {
 			nsm_log(LOG_ERR,
 				"Not a valid hostname or address: \"%s\"",
@@ -402,13 +418,12 @@ notify_host(int sock, struct nsm_host *host)
 		host->xid = xid++;
 
 	if (host->ai == NULL) {
-		host->ai = smn_lookup(AF_UNSPEC, host->name);
+		host->ai = smn_lookup(host->name);
 		if (host->ai == NULL) {
 			nsm_log(LOG_WARNING,
-				"%s doesn't seem to be a valid address,"
-				" skipped", host->name);
-			smn_forget_host(host);
-			return 1;
+				"DNS resolution of %s failed; "
+				"retrying later", host->name);
+			return 0;
 		}
 	}
 

[PATCH 3/3] mount: remove legacy version of nfs_name_to_address()

[Chuck Lever]

Currently we have two separate copies of nfs_name_to_address() since
some older glibc's don't define AI_ADDRCONFIG.  This means extra
work to build- and run-test both functions when code is changed in
this area.

It is also the case that gethostbyname(3) is deprecated, and should
not be used in new code.

Remove the legacy code in favor of always using getaddrinfo(3).

We can also get rid of nfs_name_to_address()'s @family argument as
well.

Note also this addresses a bug in nfsumount.c -- it was calling
nfs_name_to_address() with AF_UNSPEC unconditionally, even if the
legacy version of nfs_name_to_address(), which doesn't support
AF_UNSPEC, was in use.

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

 utils/mount/network.c   |   88 ++++++++++++-----------------------------------
 utils/mount/network.h   |    3 +-
 utils/mount/nfsumount.c |    2 +
 utils/mount/stropts.c   |   16 ++-------
 4 files changed, 28 insertions(+), 81 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 72f4b84..d93e98d 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -185,39 +185,32 @@ static void nfs_set_port(struct sockaddr *sap, const unsigned short port)
 	}
 }
 
-#ifdef HAVE_DECL_AI_ADDRCONFIG
-/**
- * nfs_name_to_address - resolve hostname to an IPv4 or IPv6 socket address
- * @hostname: pointer to C string containing DNS hostname to resolve
- * @af_hint: hint to restrict resolution to one address family
- * @sap: pointer to buffer to fill with socket address
- * @len: IN: size of buffer to fill; OUT: size of socket address
- *
- * Returns 1 and places a socket address at @sap if successful;
- * otherwise zero.
- */
-int nfs_name_to_address(const char *hostname,
-			const sa_family_t af_hint,
-			struct sockaddr *sap, socklen_t *salen)
+static int nfs_lookup(const char *hostname, const sa_family_t family,
+		      struct sockaddr *sap, socklen_t *salen)
 {
 	struct addrinfo *gai_results;
 	struct addrinfo gai_hint = {
-		.ai_family	= af_hint,
+#ifdef HAVE_DECL_AI_ADDRCONFIG
 		.ai_flags	= AI_ADDRCONFIG,
+#endif	/* HAVE_DECL_AI_ADDRCONFIG */
+		.ai_family	= family,
 	};
 	socklen_t len = *salen;
 	int error, ret = 0;
 
-	if (af_hint == AF_INET6)
-		gai_hint.ai_flags |= AI_V4MAPPED|AI_ALL;
-
 	*salen = 0;
 
 	error = getaddrinfo(hostname, NULL, &gai_hint, &gai_results);
-	if (error) {
+	switch (error) {
+	case 0:
+		break;
+	case EAI_SYSTEM:
 		nfs_error(_("%s: DNS resolution failed for %s: %s"),
-			progname, hostname, (error == EAI_SYSTEM ?
-				strerror(errno) : gai_strerror(error)));
+			progname, hostname, strerror(errno));
+		return ret;
+	default:
+		nfs_error(_("%s: DNS resolution failed for %s: %s"),
+			progname, hostname, gai_strerror(error));
 		return ret;
 	}
 
@@ -240,61 +233,25 @@ int nfs_name_to_address(const char *hostname,
 	freeaddrinfo(gai_results);
 	return ret;
 }
-#else	/* HAVE_DECL_AI_ADDRCONFIG */
+
 /**
- * nfs_name_to_address - resolve hostname to an IPv4 socket address
+ * nfs_name_to_address - resolve hostname to an IPv4 or IPv6 socket address
  * @hostname: pointer to C string containing DNS hostname to resolve
- * @af_hint: hint to restrict resolution to one address family
  * @sap: pointer to buffer to fill with socket address
  * @len: IN: size of buffer to fill; OUT: size of socket address
  *
  * Returns 1 and places a socket address at @sap if successful;
  * otherwise zero.
- *
- * Some older getaddrinfo(3) implementations don't support
- * AI_ADDRCONFIG or AI_V4MAPPED properly.  For those cases, a DNS
- * resolver based on the traditional gethostbyname(3) is provided.
  */
 int nfs_name_to_address(const char *hostname,
-			const sa_family_t af_hint,
 			struct sockaddr *sap, socklen_t *salen)
 {
-	struct sockaddr_in *sin = (struct sockaddr_in *)sap;
-	socklen_t len = *salen;
-	struct hostent *hp;
-
-	*salen = 0;
-
-	if (af_hint != AF_INET) {
-		nfs_error(_("%s: address family not supported by DNS resolver\n"),
-				progname, hostname);
-		return 0;
-	}
-
-	sin->sin_family = AF_INET;
-	if (inet_aton(hostname, &sin->sin_addr)) {
-		*salen = sizeof(*sin);
-		return 1;
-	}
-
-	hp = gethostbyname(hostname);
-	if (hp == NULL) {
-		nfs_error(_("%s: DNS resolution failed for %s: %s"),
-				progname, hostname, hstrerror(h_errno));
-		return 0;
-	}
-
-	if (hp->h_length > len) {
-		nfs_error(_("%s: DNS resolution results too long for buffer\n"),
-				progname);
-		return 0;
-	}
-
-	memcpy(&sin->sin_addr, hp->h_addr, hp->h_length);
-	*salen = sizeof(struct sockaddr_in);
-	return 1;
+#ifdef IPV6_SUPPORTED
+	return nfs_lookup(hostname, AF_UNSPEC, sap, salen);
+#else	/* !IPV6_SUPPORTED */
+	return nfs_lookup(hostname, AF_INET, sap, salen);
+#endif	/* !IPV6_SUPPORTED */
 }
-#endif	/* HAVE_DECL_AI_ADDRCONFIG */
 
 /**
  * nfs_gethostbyname - resolve a hostname to an IPv4 address
@@ -307,8 +264,7 @@ int nfs_gethostbyname(const char *hostname, struct sockaddr_in *sin)
 {
 	socklen_t len = sizeof(*sin);
 
-	return nfs_name_to_address(hostname, AF_INET,
-					(struct sockaddr *)sin, &len);
+	return nfs_lookup(hostname, AF_INET, (struct sockaddr *)sin, &len);
 }
 
 /**
diff --git a/utils/mount/network.h b/utils/mount/network.h
index 0dd90f8..b3f9bd2 100644
--- a/utils/mount/network.h
+++ b/utils/mount/network.h
@@ -44,8 +44,7 @@ int nfs_probe_bothports(const struct sockaddr *, const socklen_t,
 			struct pmap *, const struct sockaddr *,
 			const socklen_t, struct pmap *);
 int nfs_gethostbyname(const char *, struct sockaddr_in *);
-int nfs_name_to_address(const char *, const sa_family_t,
-		struct sockaddr *, socklen_t *);
+int nfs_name_to_address(const char *, struct sockaddr *, socklen_t *);
 int nfs_string_to_sockaddr(const char *, const size_t,
 			   struct sockaddr *, socklen_t *);
 int nfs_present_sockaddr(const struct sockaddr *,
diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index 78ebd4a..4b2e530 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -182,7 +182,7 @@ static int nfs_umount_do_umnt(struct mount_options *options,
 		return EX_FAIL;
 	}
 
-	if (nfs_name_to_address(*hostname, AF_UNSPEC, sap, &salen)) {
+	if (nfs_name_to_address(*hostname, sap, &salen)) {
 		if (nfs_advise_umount(sap, salen, &mnt_pmap, dirname) != 0)
 			return EX_SUCCESS;
 		else
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index c369136..ec95b78 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -87,8 +87,6 @@ struct nfsmount_info {
 	int			flags,		/* MS_ flags */
 				fake,		/* actually do the mount? */
 				child;		/* forked bg child? */
-
-	sa_family_t		family;		/* supported address family */
 };
 
 /*
@@ -198,8 +196,7 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
  * Resolve the 'mounthost=' hostname and append a new option using
  * the resulting address.
  */
-static int nfs_fix_mounthost_option(const sa_family_t family,
-				    struct mount_options *options)
+static int nfs_fix_mounthost_option(struct mount_options *options)
 {
 	struct sockaddr_storage dummy;
 	struct sockaddr *sap = (struct sockaddr *)&dummy;
@@ -210,7 +207,7 @@ static int nfs_fix_mounthost_option(const sa_family_t family,
 	if (!mounthost)
 		return 1;
 
-	if (!nfs_name_to_address(mounthost, family, sap, &salen)) {
+	if (!nfs_name_to_address(mounthost, sap, &salen)) {
 		nfs_error(_("%s: unable to determine mount server's address"),
 				progname);
 		return 0;
@@ -270,14 +267,14 @@ static int nfs_validate_options(struct nfsmount_info *mi)
 	if (!nfs_parse_devname(mi->spec, &mi->hostname, NULL))
 		return 0;
 
-	if (!nfs_name_to_address(mi->hostname, mi->family, sap, &salen))
+	if (!nfs_name_to_address(mi->hostname, sap, &salen))
 		return 0;
 
 	if (strncmp(mi->type, "nfs4", 4) == 0) {
 		if (!nfs_append_clientaddr_option(sap, salen, mi->options))
 			return 0;
 	} else {
-		if (!nfs_fix_mounthost_option(mi->family, mi->options))
+		if (!nfs_fix_mounthost_option(mi->options))
 			return 0;
 		if (!mi->fake && !nfs_verify_lock_option(mi->options))
 			return 0;
@@ -785,11 +782,6 @@ int nfsmount_string(const char *spec, const char *node, const char *type,
 		.flags		= flags,
 		.fake		= fake,
 		.child		= child,
-#ifdef IPV6_SUPPORTED
-		.family		= AF_UNSPEC,	/* either IPv4 or v6 */
-#else
-		.family		= AF_INET,	/* only IPv4 */
-#endif
 	};
 	int retval = EX_FAIL;
 

Re: [PATCH 0/3] Address three recently reported bugs

[Steve Dickson]

Chuck Lever wrote:
> Hi Steve-
> 
> Some bug fixes for sm-notify and mount.nfs.  Please consider these for
> nfs-utils 1.1.7.
> 
> ---
> 
> Chuck Lever (3):
>       mount: remove legacy version of nfs_name_to_address()
>       sm-notify: Failed DNS lookups should be retried
>       sm-notify: Don't orphan addrinfo structs
> 
> 
>  utils/mount/network.c   |   88 ++++++++++++-----------------------------------
>  utils/mount/network.h   |    3 +-
>  utils/mount/nfsumount.c |    2 +
>  utils/mount/stropts.c   |   16 ++-------
>  utils/statd/sm-notify.c |   73 ++++++++++++++++++++++++++-------------
>  5 files changed, 76 insertions(+), 106 deletions(-)
> 
Committed....

steved.