nfs-utils umount command does not use correct TCP/UDP protocol

After I install nfs-utils-1.0.12, the least release, I found that umount command can not use the correct TCP/UDP protocol as which it been mount.

You can test this used following commands:
#mount -t nfs -o proto=udp 127.0.0.1:/nfsroot /mnt #umount /mnt
umount will use TCP protocol to do the umount process.

Following is my patch to fix this BUG.

@@ -103,31 +103,35 @@ int nfs_call_umount(clnt_addr_t *mnt_ser
 	return 0;
 }
 
+static int
+contains(const char *list, const char *s) {
+	int n = strlen(s);
+	char *p, *rest = list;
+
+	while ((p = strstr(rest, s)) != NULL) {
+		if ((p == rest || p[-1] == ',' || p[-1] == '=')
+			&& (p[n] == '\0' || p[n] == ','))
+			return 1;
+
+                while (*rest && *rest++ != ',');
+		if(rest == NULL) break;
+        }
+        return 0;
+}
+
 u_int get_mntproto(const char *);
 u_int
 get_mntproto(const char *dirname)
 {
-	FILE *mtab;
-	struct mntent mntbuf;
-	char tmpbuf[BUFSIZ];
+	struct mntentchn *mc;
 	u_int proto = IPPROTO_TCP; /* assume tcp */
 
-	 mtab = setmntent ("/proc/mounts", "r");
-	 if (mtab == NULL)
-	 	mtab = setmntent (_PATH_MOUNTED, "r");
-	if (mtab == NULL)
-		return proto;
-
-	while(getmntent_r(mtab, &mntbuf, tmpbuf, sizeof (tmpbuf))) {
-		if (strcmp(mntbuf.mnt_type, "nfs"))
-			continue;
-		if (strcmp(dirname,  mntbuf.mnt_fsname))
-			continue;
-		if (hasmntopt(&mntbuf, "udp"))
-			proto = IPPROTO_UDP;
-		break;
-	}
-	endmntent (mtab);
+	mc = getmntdirbackward(dirname, NULL);
+	if (!mc)
+		mc = getmntdevbackward(dirname, NULL);
+
+	if(mc && contains(mc->m.mnt_opts, "udp"))
+		proto = IPPROTO_UDP;
 
 	return proto;
 }

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Nobody replied yet, but I think this is really a BUG of umount.nfs command.

Following is my reasons:
(1). Let's to see, umount used LIFO to umount file systems, but in =

nfsumount.c's function get_mntproto(), it find the first mount entry's =

opts to do the umount operation, it seems to the FIFO mode.

(2). If I just used the file system path not the name of mounted file =

system in umount, =91if (strcmp(dirname, mntbuf.mnt_fsname)) =92 condition =

will be false, default IPPROTO_TCP value will be used.

(3). The latest kernel used "...,proto=3Dudp,..." format in file =

'/proc/mounts', not the old format '...,udp,...', so 'hasmntopt(&mntbuf, =

"udp")' condition will be always false under latest kernel, that is to =

say UDP protocol will be never used.

In my patch those problem has been fixed. This patch is for =

nfs-utils-1.0.12.

@@ -103,31 +103,35 @@ int nfs_call_umount(clnt_addr_t *mnt_ser
 	return 0;
 }
 =
 
+static int
+hasmntvalue(const char *list, const char *s) {
+	int n =3D strlen(s);
+	char *p, *rest =3D list;
+
+	while ((p =3D strstr(rest, s)) !=3D NULL) {
+		if ((p =3D=3D rest || p[-1] =3D=3D ',' || p[-1] =3D=3D '=3D')
+			&& (p[n] =3D=3D '\0' || p[n] =3D=3D ','))
+			return 1;
+
+		while (*rest && *rest++ !=3D ',');
+		if(rest =3D=3D NULL) break;
+	}
+	return 0;
+}
+
 u_int get_mntproto(const char *);
 u_int
 get_mntproto(const char *dirname)
 {
-	FILE *mtab;
-	struct mntent mntbuf;
-	char tmpbuf[BUFSIZ];
+	struct mntentchn *mc;
 	u_int proto =3D IPPROTO_TCP; /* assume tcp */
 =
 
-	 mtab =3D setmntent ("/proc/mounts", "r");
-	 if (mtab =3D=3D NULL)
-	 	mtab =3D setmntent (_PATH_MOUNTED, "r");
-	if (mtab =3D=3D NULL)
-		return proto;
-
-	while(getmntent_r(mtab, &mntbuf, tmpbuf, sizeof (tmpbuf))) {
-		if (strcmp(mntbuf.mnt_type, "nfs"))
-			continue;
-		if (strcmp(dirname,  mntbuf.mnt_fsname))
-			continue;
-		if (hasmntopt(&mntbuf, "udp"))
-			proto =3D IPPROTO_UDP;
-		break;
-	}
-	endmntent (mtab);
+	mc =3D getmntdirbackward(dirname, NULL);
+	if (!mc)
+		mc =3D getmntdevbackward(dirname, NULL);
+
+	if(mc && hasmntvalue(mc->m.mnt_opts, "udp"))
+		proto =3D IPPROTO_UDP;
 =

return proto;
}

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=3Djoin.php&p=3Dsourceforge&CID=3DDE=
VDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Thank you for drawing my attention to this code. It is rather ..... interesting...

We are given a name (in variable 'spec') on the command line for something that needs to be unmounted. It is expected to be either a path name to the mountpoint, or a host:/remote/path name of what was mounted.

We then look for that in /etc/mtab or, if that file does not exist or is empty, in /proc/mounts. We first look for an entry with the name as the mountpoint, and then for an entry the name as the remote path, looking from the end of the file.

So far, so good.

If an entry was found we call _nfsumount passing the remote name and the options from mtab (or mounts). The job of _nfsumount is to tell the server that we don't have it mounted any more. _nfsumount needs to know what protocol to use and despite having the option in a string that was passed to it, it calls get_mntproto which looks in /proc/mounts (or maybe /etc/mtab) for the option. Why not just use the options that were passed, at least if you have them?
Odd.

Then, if _nfsumount fails (maybe the server was down) nfsumount() does *not* call add_mtab2. So if the server is down, you cannot unmount an nfs filesystem. That is a bit sad.

It is also kind-of odd that add_mtab2 does the actual unmount. You would think that with a name like that it would at most remove an entry from mtab... (I wonder what was wrong with 'del_mtab').

Now, suppose the entry wasn't found in mtab (maybe mtab got corrupted
- stranger things have happened).
In that case the name is given to _nfsumount with NULL as the options. So _nfsumount needs to depend on the hostname in the name (which won't be there if you give a local path name...) but goes to /proc/mounts to find the protocol. Odd. If it is going to /proc/mounts, may as well look there for addr= or mounthost= etc.

And again if _nfsumount fails (guaranteed if a local path was passed) there is no attempt to unmount the filesystem, or remove it from mtab (not that it is in mtab... we didn't find it, remember).

There is obviously a lot of room for cleaning this code up.

1/ One would think that when unmounting, we should try
/proc/mounts first. Only then in /etc/mtab if it wasn't found.
2/ We should look at the name we are passed - if it starts with a '/',
then assume it is a path name, if it doesn't and it contains a ':', we assume a remote address. Otherwise we reject.
3/ Then the three actions: tell server, tell kernel, tell mtab
should be done independently (if appropriate information is available).

As for your patch, it definitely identifies a problem, but I'm not sure it is a good fix.
The only real reason for get_mntproto is to look in /proc/mounts, and you have changed it to not look there.
Also hasmntvalue really isn't needed. Just use
hasmntopt(options,"udp") || hasmntopt(options,"proto=udp")

Anyone want to put together a little patch series to fix some/all of the above issues? :-)

NeilBrown

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

This patch series has 4 patches to fix some bugs of umount.nfs command.

[PATCH 1/4]  Use correct UMNT version to do umount
[PATCH 2/4]  Fix nfs4 umount to not used umount procedure
[PATCH 3/4]  Use correct UMNT protocol to do umount
[PATCH 4/4]  Rename add_mtab2() to del_mtab()

These is also some work to do with umount.nfs, such as following:

1. getmntdirbackward() does not free the memory it malloc, which may cause memory leak, also can be found it my patch.

2. If file /proc/mounts it wasn't found and we can not found mount entry in /etc/mtab(it has been removed from this file), umount will fail with message "umount: %s: not found"

3. If the server is down, we can not umount an nfs filesystem, I can make a patch, but I don't know if the server is down, do we have permission to umount? Can umount fail from remote server because of permission problem?

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

I don't think this is a big problem. mount.nfs is a short-lived program so a little memory not being freed is not a concern.

In this case there is nothing for mount.nfs to do anyway as we don't know which server to talk to. Presumably /bin/mount will try to call unmount on the path, which should be enough.

We certainly should be able to unmount an NFS filesystem if the server is down. unmount is mostly a local operation. We don't need any permission from the server to do it.
As a courtesy, we tell the NFS server that we have unmounted it so it can update it's 'rmtab' file. But there isn't even any guarantee of that if the client simply crashes.

So if it doesn't currently work when the server is down, and you can make a patch to fix that, I would really appreciate it.

Thanks,
NeilBrown

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs