[PATCH] umount: allow non-root umount of FUSE even if not in fstab

[PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Petr Uzel]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

Hi all,

I hacked the following patch with which it is possible to use 
"umount $dir" instead of "fusermount -u $dir", which IMHO is an
improvement in usability. It seems to work (at least for me), however,
I have to admit that I don't like it very much, because:
- it complicates umount
- duplicates code from fusermount
- should (???) be implemented using umount helpers

With regard to uhelpers: I spent some time looking into the fuse code,
but I don't think it could be done easily - has anybody investigated
this?

Related discussion (from 2006):
http://sourceforge.net/mailarchive/forum.php?thread_name=1253960851.6399.43.camel%40localhost.localdomain&forum_name=fuse-devel

This feature addresses:
https://fate.novell.com/310710

---------- 8< ---------- 8< -----------

This patch introduces an exception to the "no non-root unmounts
unless the mount is in fstab", which is necessary to allow
unmounting FUSE filesystems with umount (originally, one
had to use fusermount -u).

Umount makes the same checks as fusermount -u does, namely:
- if the fstype starts with fuse/fuseblk
- compares the user_id stored in /etc/mtab (or /proc/mounts) with
  real UID of the calling user

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 mount/umount.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/mount/umount.c b/mount/umount.c
index 42671f4..f887c6f 100644
--- a/mount/umount.c
+++ b/mount/umount.c
@@ -592,6 +592,33 @@ umount_file (char *arg) {
 			    _("umount: it seems %s is mounted multiple times"),
 			    file);
 
+		/* If this is fuse-based filesystem, allow user unmount even
+		 * if the FS is not in fstab.
+		 *
+		 * Based on fusermount code. */
+		if (strcmp(mc->m.mnt_type, "fuse") == 0 ||
+		    strcmp(mc->m.mnt_type, "fuseblk") == 0 ||
+		    strncmp(mc->m.mnt_type, "fuse.", 5) == 0 ||
+		    strncmp(mc->m.mnt_type, "fuseblk.", 8) == 0) {
+			char *fuse_uidstr;
+			fuse_uidstr = get_option_value(mc->m.mnt_opts, "user=");
+			if (!fuse_uidstr)
+				fuse_uidstr = get_option_value(mc->m.mnt_opts, "user_id=");
+			if (fuse_uidstr) {
+				char uidstr[32];
+				unsigned uidlen;
+				uidlen = sprintf(uidstr, "%u", getuid());
+				if (strncmp(uidstr, fuse_uidstr, uidlen) == 0) {
+					if (verbose)
+						printf(_("Unmounting %s (FUSE filesystem, mounted by uid=%s)\n"),
+						       arg, uidstr);
+					int ret = umount_one(arg, arg, arg, arg, NULL);
+					free(fuse_uidstr);
+					return ret;
+				}
+			}
+		}
+
 		/* If fstab contains the two lines
 		   /dev/sda1 /mnt/zip auto user,noauto  0 0
 		   /dev/sda4 /mnt/zip auto user,noauto  0 0
-- 
1.7.3.4


Petr

--
Petr Uzel
IRC: ptr_uzl @ freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Miklos Szeredi]

On Mon, 2011-03-28 at 16:04 +0200, Petr Uzel wrote:
> Hi all,
> 
> I hacked the following patch with which it is possible to use 
> "umount $dir" instead of "fusermount -u $dir", which IMHO is an
> improvement in usability. It seems to work (at least for me), however,
> I have to admit that I don't like it very much, because:
> - it complicates umount
> - duplicates code from fusermount

And this is not the only one that would have to be duplicated.  The
mount and umount races that were fixed in fusermount in recently and not
so recently would also have to be added to util-linux, which would
actually be a good thing, since in theory they could affect fstab based
user mounts as well (though that is much more unlikely than with fuse,
where the user chooses the mountpoint).


> - should (???) be implemented using umount helpers

The end goal is to implement permission checking for unprivileged mount
and umount in the kernel.

Thanks,
Miklos

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Karel Zak]

On Mon, Mar 28, 2011 at 04:04:56PM +0200, Petr Uzel wrote:
> I have to admit that I don't like it very much, because:

I don't like it too :-)

> - it complicates umount
> - duplicates code from fusermount
> - should (???) be implemented using umount helpers

[...]

> +		/* If this is fuse-based filesystem, allow user unmount even
> +		 * if the FS is not in fstab.
> +		 *
> +		 * Based on fusermount code. */
> +		if (strcmp(mc->m.mnt_type, "fuse") == 0 ||
> +		    strcmp(mc->m.mnt_type, "fuseblk") == 0 ||
> +		    strncmp(mc->m.mnt_type, "fuse.", 5) == 0 ||
> +		    strncmp(mc->m.mnt_type, "fuseblk.", 8) == 0) {

Wouldn't be better to always call /sbin/umount.fuse for non-roots (except
umount -i)? 

I believe more and more that we need something like

    /etc/mount.d/{fuse,foo,...}.conf

where we can define such behavior for some filesystems.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Petr Uzel]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

On Mon, Mar 28, 2011 at 04:23:20PM +0200, Miklos Szeredi wrote:
> On Mon, 2011-03-28 at 16:04 +0200, Petr Uzel wrote:
> > Hi all,
> > 
> > I hacked the following patch with which it is possible to use 
> > "umount $dir" instead of "fusermount -u $dir", which IMHO is an
> > improvement in usability. It seems to work (at least for me), however,
> > I have to admit that I don't like it very much, because:
> > - it complicates umount
> > - duplicates code from fusermount
> 
> And this is not the only one that would have to be duplicated.  The
> mount and umount races that were fixed in fusermount in recently and not
> so recently would also have to be added to util-linux, which would
> actually be a good thing, since in theory they could affect fstab based
> user mounts as well (though that is much more unlikely than with fuse,
> where the user chooses the mountpoint).

Do you mean this commit:

8b3a0c74a15e237eb4b7053774600f0ce3fff403 
Fix race if two "fusermount -u" instances are run in parallel. 

?

> > - should (???) be implemented using umount helpers
> 
> The end goal is to implement permission checking for unprivileged mount
> and umount in the kernel.

OK, that sounds reasonable. Not something I'd have guts to look into,
though :)


Petr

--
Petr Uzel
IRC: ptr_uzl @ freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Karel Zak]

On Mon, Mar 28, 2011 at 04:23:20PM +0200, Miklos Szeredi wrote:
> On Mon, 2011-03-28 at 16:04 +0200, Petr Uzel wrote:
> > Hi all,
> > 
> > I hacked the following patch with which it is possible to use 
> > "umount $dir" instead of "fusermount -u $dir", which IMHO is an
> > improvement in usability. It seems to work (at least for me), however,
> > I have to admit that I don't like it very much, because:
> > - it complicates umount
> > - duplicates code from fusermount
> 
> And this is not the only one that would have to be duplicated.  The
> mount and umount races that were fixed in fusermount in recently and not
> so recently would also have to be added to util-linux, which would
> actually be a good thing, since in theory they could affect fstab based
> user mounts as well (though that is much more unlikely than with fuse,
> where the user chooses the mountpoint).

 Maybe we need to call umount2() with UMOUNT_NOFOLLOW flag for
 non-root users in umount(8). I think it should be enough for
 umount(8) (where almost all is controlled by system admin in fstab). 
 
 See below. Comments?

    Karel


>From 5cf67485d23dc4547eb5e54cbe96cc60837e36af Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Tue, 29 Mar 2011 10:19:56 +0200
Subject: [PATCH] umount: use UMOUNT_NOFOLLOW for non-root users

Signed-off-by: Karel Zak <kzak@redhat.com>
---
 mount/umount.c |   44 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/mount/umount.c b/mount/umount.c
index 42671f4..0660b20 100644
--- a/mount/umount.c
+++ b/mount/umount.c
@@ -45,15 +45,22 @@ umount2(const char *path, int flags) {
 }
 #endif /* __NR_umount2 */
 
-#if !defined(MNT_FORCE)
-/* dare not try to include <linux/mount.h> -- lots of errors */
-#define MNT_FORCE 1
+#ifndef MNT_FORCE
+# define MNT_FORCE        0x00000001	/* Attempt to forcibily umount */
 #endif
 
 #endif /* MNT_FORCE */
 
-#if !defined(MNT_DETACH)
-#define MNT_DETACH 2
+#ifndef MNT_DETACH
+# define MNT_DETACH       0x00000002	/* Just detach from the tree */
+#endif
+
+#ifndef UMOUNT_NOFOLLOW
+# define UMOUNT_NOFOLLOW  0x00000008	/* Don't follow symlink on umount */
+#endif
+
+#ifndef UMOUNT_UNUSED
+#define UMOUNT_UNUSED     0x80000000	/* Flag guaranteed to be unused */
 #endif
 
 
@@ -197,6 +204,21 @@ static void complain(int err, const char *dev) {
   }
 }
 
+/* Check whether the kernel supports UMOUNT_NOFOLLOW flag */
+static int umount_nofollow_support(void)
+{
+	int res = umount2("", UMOUNT_UNUSED);
+	if (res != -1 || errno != EINVAL)
+		return 0;
+
+	res = umount2("", UMOUNT_NOFOLLOW);
+	if (res != -1 || errno != ENOENT)
+		return 0;
+
+	return 1;
+}
+
+
 /* Umount a single device.  Return a status code, so don't exit
    on a non-fatal error.  We lock/unlock around each umount.  */
 static int
@@ -206,6 +228,7 @@ umount_one (const char *spec, const char *node, const char *type,
 	int isroot;
 	int res = 0;
 	int status;
+	int extra_flags = 0;
 	const char *loopdev;
 	int myloop = 0;
 
@@ -237,15 +260,18 @@ umount_one (const char *spec, const char *node, const char *type,
 	if (delloop && is_loop_device(spec))
 		myloop = 1;
 
+	if (restricted && umount_nofollow_support())
+		extra_flags |= UMOUNT_NOFOLLOW;
+
 	if (lazy) {
-		res = umount2 (node, MNT_DETACH);
+		res = umount2 (node, MNT_DETACH | extra_flags);
 		if (res < 0)
 			umnt_err = errno;
 		goto writemtab;
 	}
 
 	if (force) {		/* only supported for NFS */
-		res = umount2 (node, MNT_FORCE);
+		res = umount2 (node, MNT_FORCE | extra_flags);
 		if (res == -1) {
 			int errsv = errno;
 			perror("umount2");
@@ -256,7 +282,9 @@ umount_one (const char *spec, const char *node, const char *type,
 				res = umount (node);
 			}
 		}
-	} else
+	} else if (extra_flags)
+		res = umount2 (node, extra_flags);
+	else
 		res = umount (node);
 
 	if (res < 0)
-- 
1.7.3.4

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Petr Uzel]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

On Tue, Mar 29, 2011 at 12:03:44AM +0200, Karel Zak wrote:
> On Mon, Mar 28, 2011 at 04:04:56PM +0200, Petr Uzel wrote:
> > I have to admit that I don't like it very much, because:
> 
> I don't like it too :-)

I'm not surprised :)

> 
> > - it complicates umount
> > - duplicates code from fusermount
> > - should (???) be implemented using umount helpers
> 
> [...]
> 
> > +		/* If this is fuse-based filesystem, allow user unmount even
> > +		 * if the FS is not in fstab.
> > +		 *
> > +		 * Based on fusermount code. */
> > +		if (strcmp(mc->m.mnt_type, "fuse") == 0 ||
> > +		    strcmp(mc->m.mnt_type, "fuseblk") == 0 ||
> > +		    strncmp(mc->m.mnt_type, "fuse.", 5) == 0 ||
> > +		    strncmp(mc->m.mnt_type, "fuseblk.", 8) == 0) {
> 
> Wouldn't be better to always call /sbin/umount.fuse for non-roots (except
> umount -i)? 

Yes, that's what I meant by umount helpers. The problem is that AFAICS
fuse does not provide this helper.

@Miklos: what do you think? Would it make sense to have umount.fuse
(most likely as symlink to /sbin/mount.fuse)? I could look into it.

> 
> I believe more and more that we need something like
> 
>     /etc/mount.d/{fuse,foo,...}.conf
> 
> where we can define such behavior for some filesystems.

Hm, I fail to understand what (regarding to this 'issue') would
you configure there. I can imagine e.g. per-fs-type default mount
options or enable/disable 'user mounts' (-i) per fs-type.

Is this what you think that should be configured via /etc/mount.d/*,
or something else?

Thanks,

Petr

--
Petr Uzel
IRC: ptr_uzl @ freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Miklos Szeredi]

On Tue, 2011-03-29 at 10:28 +0200, Karel Zak wrote:
> On Mon, Mar 28, 2011 at 04:23:20PM +0200, Miklos Szeredi wrote:
> > On Mon, 2011-03-28 at 16:04 +0200, Petr Uzel wrote:
> > > Hi all,
> > > 
> > > I hacked the following patch with which it is possible to use 
> > > "umount $dir" instead of "fusermount -u $dir", which IMHO is an
> > > improvement in usability. It seems to work (at least for me), however,
> > > I have to admit that I don't like it very much, because:
> > > - it complicates umount
> > > - duplicates code from fusermount
> > 
> > And this is not the only one that would have to be duplicated.  The
> > mount and umount races that were fixed in fusermount in recently and not
> > so recently would also have to be added to util-linux, which would
> > actually be a good thing, since in theory they could affect fstab based
> > user mounts as well (though that is much more unlikely than with fuse,
> > where the user chooses the mountpoint).
> 
>  Maybe we need to call umount2() with UMOUNT_NOFOLLOW flag for
>  non-root users in umount(8). I think it should be enough for
>  umount(8) (where almost all is controlled by system admin in fstab). 
>  
>  See below. Comments?

UMOUNT_NOFOLLOW is a good idea but not enough, it will only deal with
last path component changing to a symlink.  If previous path component
is changed then UMOUNT_NOFOLLOW will not have any effect.

What fusermount does is change cwd to the parent directory, check if cwd
matches that of the intended path, and then umount with UMOUNT_NOFOLLOW.

Thanks,
Miklos

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Miklos Szeredi]

On Tue, 2011-03-29 at 10:46 +0200, Petr Uzel wrote:
> > Wouldn't be better to always call /sbin/umount.fuse for non-roots (except
> > umount -i)? 
> 
> Yes, that's what I meant by umount helpers. The problem is that AFAICS
> fuse does not provide this helper.
> 
> @Miklos: what do you think? Would it make sense to have umount.fuse
> (most likely as symlink to /sbin/mount.fuse)? I could look into it.

Could umount.fuse allow umount of fuse filesystem even if not in fstab?

If so, then I don't see any problem with providing /sbin/umount.fuse.

Thanks,
Miklos

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Petr Uzel]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Tue, Mar 29, 2011 at 10:51:30AM +0200, Miklos Szeredi wrote:
> On Tue, 2011-03-29 at 10:46 +0200, Petr Uzel wrote:
> > > Wouldn't be better to always call /sbin/umount.fuse for non-roots (except
> > > umount -i)? 
> > 
> > Yes, that's what I meant by umount helpers. The problem is that AFAICS
> > fuse does not provide this helper.
> > 
> > @Miklos: what do you think? Would it make sense to have umount.fuse
> > (most likely as symlink to /sbin/mount.fuse)? I could look into it.
> 
> Could umount.fuse allow umount of fuse filesystem even if not in fstab?

Yes, as far as I understand.

> If so, then I don't see any problem with providing /sbin/umount.fuse.

OK, I'll try to look into it.

> 
> Thanks,
> Miklos

Thanks,

        Petr

--
Petr Uzel
IRC: ptr_uzl @ freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Karel Zak]

On Tue, Mar 29, 2011 at 10:56:01AM +0200, Petr Uzel wrote:
> On Tue, Mar 29, 2011 at 10:51:30AM +0200, Miklos Szeredi wrote:
> > On Tue, 2011-03-29 at 10:46 +0200, Petr Uzel wrote:
> > > > Wouldn't be better to always call /sbin/umount.fuse for non-roots (except
> > > > umount -i)? 
> > > 
> > > Yes, that's what I meant by umount helpers. The problem is that AFAICS
> > > fuse does not provide this helper.
> > > 
> > > @Miklos: what do you think? Would it make sense to have umount.fuse
> > > (most likely as symlink to /sbin/mount.fuse)? I could look into it.
> > 
> > Could umount.fuse allow umount of fuse filesystem even if not in fstab?

By default umount(8) requires an entry in /etc/fstab. This behaviour
could be changed by uhelper= mount option (the option has to be in
mtab). The solution based on uhelper= also sucks, because it requires
userspace specific mount option...

This is reason why I think that /etc/mount.d/fuse.conf would be better
than add an exception to the code or use uhelper= mount option. The
same problem we have with HAL (udisks) where non-root user stuff is
not in fstab.
 
> > If so, then I don't see any problem with providing /sbin/umount.fuse.
> 
> OK, I'll try to look into it.

It will require the exception for fuse.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Karel Zak]

On Tue, Mar 29, 2011 at 10:48:44AM +0200, Miklos Szeredi wrote:
> On Tue, 2011-03-29 at 10:28 +0200, Karel Zak wrote:
> > On Mon, Mar 28, 2011 at 04:23:20PM +0200, Miklos Szeredi wrote:
> > > On Mon, 2011-03-28 at 16:04 +0200, Petr Uzel wrote:
> > > > Hi all,
> > > > 
> > > > I hacked the following patch with which it is possible to use 
> > > > "umount $dir" instead of "fusermount -u $dir", which IMHO is an
> > > > improvement in usability. It seems to work (at least for me), however,
> > > > I have to admit that I don't like it very much, because:
> > > > - it complicates umount
> > > > - duplicates code from fusermount
> > > 
> > > And this is not the only one that would have to be duplicated.  The
> > > mount and umount races that were fixed in fusermount in recently and not
> > > so recently would also have to be added to util-linux, which would
> > > actually be a good thing, since in theory they could affect fstab based
> > > user mounts as well (though that is much more unlikely than with fuse,
> > > where the user chooses the mountpoint).
> > 
> >  Maybe we need to call umount2() with UMOUNT_NOFOLLOW flag for
> >  non-root users in umount(8). I think it should be enough for
> >  umount(8) (where almost all is controlled by system admin in fstab). 
> >  
> >  See below. Comments?
> 
> UMOUNT_NOFOLLOW is a good idea but not enough, it will only deal with
> last path component changing to a symlink.  If previous path component
> is changed then UMOUNT_NOFOLLOW will not have any effect.
> 
> What fusermount does is change cwd to the parent directory, check if cwd
> matches that of the intended path, and then umount with UMOUNT_NOFOLLOW.

Ah.. I see the code. Thanks.

It would be nice to have

        mountat(dirfd, ...)
        umountat(dirfd, ...)

syscalls to avoid any races :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] umount: allow non-root umount of FUSE even if not in fstab

[Karel Zak]

On Tue, Mar 29, 2011 at 10:46:23AM +0200, Petr Uzel wrote:
> > I believe more and more that we need something like
> > 
> >     /etc/mount.d/{fuse,foo,...}.conf
> > 
> > where we can define such behavior for some filesystems.
> 
> Hm, I fail to understand what (regarding to this 'issue') would
> you configure there. I can imagine e.g. per-fs-type default mount
> options or enable/disable 'user mounts' (-i) per fs-type.
> 
> Is this what you think that should be configured via /etc/mount.d/*,
> or something else?

The mount(8) and umount(8) are very strict about user mounts and about
the way how it calls /sbin/[u]mount helpers. It was fine many years
ago, but now we have things like pam_mount, fuse, HAL/udisks where
mount/umount stuff is managed in a different way. It means that we
need exceptions... and exceptions suck.

I think about:

 $ cat /etc/mount.d/fuse.conf

    [usermount]
    enabled = true
    require_fstab_entry = false
    require_helper = true


and yes, things like per-fs-type default mount options is another
possibility. 

Maybe this is something what should be added to the libmount.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com