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

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

[Wei Yongjun]

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.

Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

--- utils/mount/nfsumount.c.orig	2007-03-05 03:11:48.000000000 -0500
+++ utils/mount/nfsumount.c	2007-03-05 03:06:25.000000000 -0500
@@ -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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils umount command does not use correct TCP/UDP protocol

[Wei Yongjun]

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.

Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

--- utils/mount/nfsumount.c.orig	2007-03-05 03:11:48.000000000 -0500
+++ utils/mount/nfsumount.c	2007-03-05 03:06:25.000000000 -0500
@@ -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;
 }


> 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=3Dudp 127.0.0.1:/nfsroot /mnt
> #umount /mnt
> umount will use TCP protocol to do the umount process.
>   =


-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils umount command does not use correct TCP/UDP protocol

[Neil Brown]

On Tuesday March 13, yjwei@nanjing-fnst.com wrote:
> Nobody replied yet, but I think this is really a BUG of umount.nfs command.

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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

[PATCH 0/4] nfs-utils : fix some bugs of umount.nfs

[Wei Yongjun]

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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 0/4] nfs-utils : fix some bugs of umount.nfs

[Neil Brown]

On Wednesday March 14, yjwei@nanjing-fnst.com wrote:
> 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.

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.

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

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.

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

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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs