[PATCH 0/4] Patches for nfs-utils 1.2.4

[PATCH 0/4] Patches for nfs-utils 1.2.4

[Chuck Lever]

For review only, here are some potential bug fixes for the next
release of nfs-utils.

I have a potential patch for the remount bug (the second in this
series).  Karel, have a look at this one to see if I've captured
your idea about merging mount options.

Anyone else, please offer comments and thoughts.

---

Chuck Lever (4):
      mount.nfs: Fix memory leak in nfs_sys_mount()
      mount.nfs: Preserve options in /etc/mtab during remount
      mount.nfs: freq and pass are always zero
      nfs-utils: Remove all uses of AI_ADDRCONFIG


 support/export/hostname.c     |    6 ---
 tests/nsm_client/nsm_client.c |    2 -
 utils/mount/mount.c           |   89 ++++++++++++++++++++++++++++++++++++++---
 utils/mount/network.c         |    3 -
 utils/mount/stropts.c         |   13 ++----
 utils/statd/hostname.c        |    4 --
 utils/statd/sm-notify.c       |    5 --
 7 files changed, 90 insertions(+), 32 deletions(-)

-- 
Chuck Lever

[PATCH 1/4] nfs-utils: Remove all uses of AI_ADDRCONFIG

[Chuck Lever]

Steve Dickson reports that, if only "lo" is up,

  mount.nfs 127.0.0.1:/export /mount

fails with "Name or service not known".

"man 3 getaddrinfo" says this:

  If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4
  addresses are returned in the list pointed to by res only if the
  local system has at least one IPv4 address configured, and IPv6
  addresses are only returned if the local system has at least
  one IPv6 address configured.

The man page oversimplifies here.  A review of glibc shows that
getaddrinfo(3) explicitly ignores loopback addresses when deciding
whether an IPv4 or IPv6 address is configured.

This behavior around loopback is a problem not just for mount.nfs,
but also for RPC daemons that have to start up before a system's
networking is fully configured and started.  Given the history of
other problems with AI_ADDRCONFIG and the unpredictable behavior it
introduces, let's just remove it everywhere in nfs-utils.

This fix addresses:

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=191

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

 support/export/hostname.c     |    6 +-----
 tests/nsm_client/nsm_client.c |    2 +-
 utils/mount/network.c         |    3 ---
 utils/mount/stropts.c         |    5 -----
 utils/statd/hostname.c        |    4 ----
 utils/statd/sm-notify.c       |    5 -----
 6 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/support/export/hostname.c b/support/export/hostname.c
index 3c55ce7..efcb75c 100644
--- a/support/export/hostname.c
+++ b/support/export/hostname.c
@@ -30,10 +30,6 @@
 #include "sockaddr.h"
 #include "exportfs.h"
 
-#ifndef HAVE_DECL_AI_ADDRCONFIG
-#define AI_ADDRCONFIG	0
-#endif
-
 /**
  * host_ntop - generate presentation address given a sockaddr
  * @sap: pointer to socket address
@@ -170,7 +166,7 @@ host_addrinfo(const char *hostname)
 #endif
 		/* don't return duplicates */
 		.ai_protocol	= (int)IPPROTO_UDP,
-		.ai_flags	= AI_ADDRCONFIG | AI_CANONNAME,
+		.ai_flags	= AI_CANONNAME,
 	};
 	int error;
 
diff --git a/tests/nsm_client/nsm_client.c b/tests/nsm_client/nsm_client.c
index 0d1159a..0fa3422 100644
--- a/tests/nsm_client/nsm_client.c
+++ b/tests/nsm_client/nsm_client.c
@@ -205,7 +205,7 @@ nsm_client_get_rpcclient(const char *node)
 {
 	unsigned short		port;
 	struct addrinfo		*ai;
-	struct addrinfo		hints = { .ai_flags	= AI_ADDRCONFIG };
+	struct addrinfo		hints = { };
 	int			err;
 	CLIENT			*client = NULL;
 
diff --git a/utils/mount/network.c b/utils/mount/network.c
index 5b515c3..21a7a2c 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -210,9 +210,6 @@ int nfs_lookup(const char *hostname, const sa_family_t family,
 {
 	struct addrinfo *gai_results;
 	struct addrinfo gai_hint = {
-#ifdef HAVE_DECL_AI_ADDRCONFIG
-		.ai_flags	= AI_ADDRCONFIG,
-#endif	/* HAVE_DECL_AI_ADDRCONFIG */
 		.ai_family	= family,
 	};
 	socklen_t len = *salen;
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 50a1a2a..bcc36f3 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -49,10 +49,6 @@
 #include "parse_dev.h"
 #include "conffile.h"
 
-#ifndef HAVE_DECL_AI_ADDRCONFIG
-#define AI_ADDRCONFIG	0
-#endif
-
 #ifndef NFS_PROGRAM
 #define NFS_PROGRAM	(100003)
 #endif
@@ -343,7 +339,6 @@ static int nfs_validate_options(struct nfsmount_info *mi)
 {
 	struct addrinfo hint = {
 		.ai_protocol	= (int)IPPROTO_UDP,
-		.ai_flags	= AI_ADDRCONFIG,
 	};
 	sa_family_t family;
 	int error;
diff --git a/utils/statd/hostname.c b/utils/statd/hostname.c
index 38f2265..616a3cb 100644
--- a/utils/statd/hostname.c
+++ b/utils/statd/hostname.c
@@ -39,10 +39,6 @@
 #include "statd.h"
 #include "xlog.h"
 
-#ifndef HAVE_DECL_AI_ADDRCONFIG
-#define AI_ADDRCONFIG	0
-#endif
-
 /**
  * statd_present_address - convert sockaddr to presentation address
  * @sap: pointer to socket address to convert
diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 437e37a..b7f4371 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -34,10 +34,6 @@
 #include "nsm.h"
 #include "nfsrpc.h"
 
-#ifndef HAVE_DECL_AI_ADDRCONFIG
-#define AI_ADDRCONFIG	0
-#endif
-
 #define NSM_TIMEOUT	2
 #define NSM_MAX_TIMEOUT	120	/* don't make this too big */
 
@@ -78,7 +74,6 @@ smn_lookup(const char *name)
 {
 	struct addrinfo	*ai = NULL;
 	struct addrinfo hint = {
-		.ai_flags	= AI_ADDRCONFIG,
 		.ai_family	= (nsm_family == AF_INET ? AF_INET: AF_UNSPEC),
 		.ai_protocol	= (int)IPPROTO_UDP,
 	};

[PATCH 2/4] mount.nfs: freq and pass are always zero

[Chuck Lever]

Clean up.

No need to pass constant zeros to add_mtab() from its only call site.

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

 utils/mount/mount.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index b4da21f..8fb03e0 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -249,6 +249,7 @@ create_mtab (void) {
 		mnt.mnt_fsname = xstrdup(fstab->m.mnt_fsname);
 		mnt.mnt_type = fstab->m.mnt_type;
 		mnt.mnt_opts = fix_opts_string (flags, extra_opts);
+		/* these are always zero for NFS */
 		mnt.mnt_freq = mnt.mnt_passno = 0;
 		free(extra_opts);
 
@@ -273,7 +274,7 @@ create_mtab (void) {
 }
 
 static int add_mtab(char *spec, char *mount_point, char *fstype,
-			int flags, char *opts, int freq, int pass)
+			int flags, char *opts)
 {
 	struct mntent ment;
 	int result = EX_SUCCESS;
@@ -282,8 +283,8 @@ static int add_mtab(char *spec, char *mount_point, char *fstype,
 	ment.mnt_dir = mount_point;
 	ment.mnt_type = fstype;
 	ment.mnt_opts = fix_opts_string(flags & ~MS_NOMTAB, opts);
-	ment.mnt_freq = freq;
-	ment.mnt_passno = pass;
+	/* these are always zero for NFS */
+	ment.mnt_freq = ment.mnt_passno = 0;
 
 	if (!nomtab && mtab_does_not_exist()) {
 		if (verbose > 1)
@@ -441,9 +442,7 @@ static int try_mount(char *spec, char *mount_point, int flags,
 	if (!fake)
 		print_one(spec, mount_point, fs_type, mount_opts);
 
-	ret = add_mtab(spec, mount_point, fs_type, flags, *extra_opts,
-			0, 0 /* these are always zero for NFS */ );
-	return ret;
+	return add_mtab(spec, mount_point, fs_type, flags, *extra_opts);
 }
 
 int main(int argc, char *argv[])

[PATCH 3/4] mount.nfs: Preserve options in /etc/mtab during remount

[Chuck Lever]

It appears that, for a long while, NFS "remount" mounts have
completely wiped the existing mount options in /etc/mtab for a given
mount point.  This is a problem for umount.nfs, since it reads its
options out of /etc/mtab to find out how to do the unmount.

umount.nfs could read /proc/mounts instead, since it always has the
authoritative set of mount options in effect for that mount point.
However, /proc/mounts contains the options that were negotiated, not
the options that were specified on the command line.

For example, if the server's mountd had been restarted and has acquired
a different port since the client mounted the server, the specified
command line mount options would probably allow the UMNT to renegotiate
and complete successfully.  But the "in effect" mount options in
/proc/mounts, which always include a specific mountport=, would most
likely fail in this case.

In addition, older kernels won't show much of the information in
/proc/mounts that umount.nfs needs.

Therefore mount.nfs (and umount.nfs) must put the correct set of mount
options in /etc/mtab to ensure that the final umount.nfs will do the
right thing.

This is a fix for:

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=188

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

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

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 8fb03e0..45ae5f6 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -273,6 +273,81 @@ create_mtab (void) {
 	reset_mtab_info();
 }
 
+static int string_is_empty(const char *s)
+{
+	if (s != NULL && *s != '\0')
+		return 0;
+	return 1;
+}
+
+static char *concat_opts(const char *left, const char *right)
+{
+	char *retval;
+
+	if (string_is_empty(left)) {
+		if (string_is_empty(right))
+			return NULL;
+		return strdup(right);
+	}
+	if (string_is_empty(right))
+		return strdup(left);
+
+	retval = malloc(strlen(left) + strlen(",") + strlen(right) + 1);
+	if (retval == NULL)
+		return retval;
+
+	retval[0] = '\0';
+	strcat(retval, left);
+	strcat(retval, ",");
+	strcat(retval, right);
+	return retval;
+}
+
+/*
+ * Return new set of MS_ flags and dynamically allocated string
+ * containing mount options to write to /etc/mtab after a remount.
+ */
+static void remount_opts(char *spec, char *cmdline,
+		int *flags, char **extra_opts)
+{
+	char *old_opts, *tmp_opts;
+	struct mntentchn *mc;
+
+	/*
+	 * Retrieve the existing mount options for this mount
+	 * point from /etc/mtab (or /proc/mounts).
+	 */
+	if (*spec == '/')
+		mc = getmntdirbackward(spec, NULL);
+	else
+		mc = getmntdevbackward(spec, NULL);
+	if (mc == NULL) {
+		if (verbose)
+			printf(_("Could not find %s in mtab\n"), spec);
+		/* don't change flags and extra_opts */
+		return;
+	}
+
+	/*
+	 * To construct the correct set of flags, concatenate the
+	 * command line options to the old options from mtab, then
+	 * parse them, left to right.  This ensures we handle the
+	 * default setting of "ro" and "rw" correctly, and prevents
+	 * duplicate MS_ options from showing up.
+	 *
+	 * Later, fix_opts_string() will re-assemble these into a
+	 * single canonical mount options string.
+	 */
+	tmp_opts = concat_opts(mc->m.mnt_opts, cmdline);
+	if (tmp_opts == NULL)
+		return;
+
+	xfree(*extra_opts);
+	*flags = 0;
+	parse_opts(tmp_opts, flags, extra_opts);
+	free(tmp_opts);
+}
+
 static int add_mtab(char *spec, char *mount_point, char *fstype,
 			int flags, char *opts)
 {
@@ -442,6 +517,9 @@ static int try_mount(char *spec, char *mount_point, int flags,
 	if (!fake)
 		print_one(spec, mount_point, fs_type, mount_opts);
 
+	if (flags & MS_REMOUNT)
+		remount_opts(spec, mount_opts, &flags, extra_opts);
+
 	return add_mtab(spec, mount_point, fs_type, flags, *extra_opts);
 }
 

[PATCH 4/4] mount.nfs: Fix memory leak in nfs_sys_mount()

[Chuck Lever]

This appears to have been left behind by last year's adjustments to
how the extra_opts string is constructed.

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

 utils/mount/stropts.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index bcc36f3..e3a25be 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -565,16 +565,18 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
 	char *options = NULL;
 	int result;
 
+	if (mi->fake)
+		return 1;
+
 	if (po_join(opts, &options) == PO_FAILED) {
 		errno = EIO;
 		return 0;
 	}
 
-	if (mi->fake)
-		return 1;
-
 	result = mount(mi->spec, mi->node, mi->type,
 			mi->flags & ~(MS_USER|MS_USERS), options);
+	free(options);
+
 	if (verbose && result) {
 		int save = errno;
 		nfs_error(_("%s: mount(2): %s"), progname, strerror(save));