diff mbox

Autofs and mount namespaces

Message ID 20160407181911.GA9018@vader (mailing list archive)
State New, archived
Headers show

Commit Message

Omar Sandoval April 7, 2016, 6:19 p.m. UTC 
Hi, Ian,

I wanted to bring up the issue of autofs and mount namespaces again,
which recently resurfaced in the thread here [1]. In that thread, you
mentioned that you had some patches to make autofs namespace-aware that
you were holding on to. Do you think you could put those up so we can
all work out the issues you mentioned?

For some background, we've also been bitten by the lack of
namespace-awareness in autofs many times. The simple case that breaks
can be reproduced as follows, assuming that /mnt/foo is an indirect
mount:

ls /mnt/foo            # do the initial automount
unshare -m sleep 10 &  # hold the automount in a new namespace
umount /mnt/foo        # pretend the mount timed out
ls /mnt/foo            # try to access it again
ls: cannot open directory '/mnt/foo': Too many levels of symbolic links

For us, the unshare step is actually setting up a container. Our
workaround was to make sure that we clean up all of the base system's
mounts when setting up the container, but in the general case this is
still broken.

In this particular case, I believe the problem is that we're using
`d_mountpoint()` (or `have_submounts()`) in several places to check
whether something is already mounted, and that's not namespace-aware.
This hack mitigates the problem I saw above, although it probably
ignores edge cases that the old code was handling:

----
----

Thanks.

1: http://thread.gmane.org/gmane.linux.kernel.autofs/6868/focus=7260
2: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/autofs4/root.c?h=v4.6-rc2#n381

--
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

kernel test robot April 7, 2016, 7:08 p.m. UTC | #1 
Hi Omar,

[auto build test ERROR on v4.6-rc2]
[also build test ERROR on next-20160407]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Omar-Sandoval/Autofs-and-mount-namespaces/20160408-022034
config: i386-randconfig-i0-201614 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "lookup_mnt" [fs/autofs4/autofs4.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Omar Sandoval April 7, 2016, 7:42 p.m. UTC | #2 
On Fri, Apr 08, 2016 at 03:08:42AM +0800, kbuild test robot wrote:
> Hi Omar,
> 
> [auto build test ERROR on v4.6-rc2]
> [also build test ERROR on next-20160407]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Omar-Sandoval/Autofs-and-mount-namespaces/20160408-022034
> config: i386-randconfig-i0-201614 (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
> >> ERROR: "lookup_mnt" [fs/autofs4/autofs4.ko] undefined!
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation


Oh, yeah, that needs an EXPORT_SYMBOL(lookup_mnt).
Ian Kent April 8, 2016, 1:21 a.m. UTC | #3 
On Thu, 2016-04-07 at 11:19 -0700, Omar Sandoval wrote:
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 0146d911f468..b4e901d2aa6b 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -332,8 +332,6 @@ static int validate_request(struct
> autofs_wait_queue **wait,
>  					dentry = new;
>  			}
>  		}
> -		if (have_submounts(dentry))
> -			valid = 0;

LOL, that's going to break rootless multi-mounts.

Consider the case where you have:

<key>	/one   a_server:/path \
        /two   any_other:/any_other_path \
        ....

The ability to check that the non-mountpoint dentry is in fact the root
of a mounted tree of mounts is gone, ;)

Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 7ab923940d18..a620822342c8 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -378,39 +378,29 @@  static struct vfsmount *autofs4_d_automount(struct path *path)
 		goto done;
 	}
 
-	if (!d_mountpoint(dentry)) {
-		/*
-		 * It's possible that user space hasn't removed directories
-		 * after umounting a rootless multi-mount, although it
-		 * should. For v5 have_submounts() is sufficient to handle
-		 * this because the leaves of the directory tree under the
-		 * mount never trigger mounts themselves (they have an autofs
-		 * trigger mount mounted on them). But v4 pseudo direct mounts
-		 * do need the leaves to trigger mounts. In this case we
-		 * have no choice but to use the list_empty() check and
-		 * require user space behave.
-		 */
-		if (sbi->version > 4) {
-			if (have_submounts(dentry)) {
-				spin_unlock(&sbi->fs_lock);
-				goto done;
-			}
-		} else {
-			if (!simple_empty(dentry)) {
-				spin_unlock(&sbi->fs_lock);
-				goto done;
-			}
-		}
-		ino->flags |= AUTOFS_INF_PENDING;
-		spin_unlock(&sbi->fs_lock);
-		status = autofs4_mount_wait(dentry, 0);
-		spin_lock(&sbi->fs_lock);
-		ino->flags &= ~AUTOFS_INF_PENDING;
-		if (status) {
+	if (d_mountpoint(dentry)) {
+		/* Make sure this is a mountpoint in this namespace. */
+		struct vfsmount *mounted = lookup_mnt(path);
+		if (mounted) {
+			mntput(mounted);
 			spin_unlock(&sbi->fs_lock);
-			return ERR_PTR(status);
+			goto done;
 		}
 	}
+
+	if (!simple_empty(dentry)) {
+		spin_unlock(&sbi->fs_lock);
+		goto done;
+	}
+	ino->flags |= AUTOFS_INF_PENDING;
+	spin_unlock(&sbi->fs_lock);
+	status = autofs4_mount_wait(dentry, 0);
+	spin_lock(&sbi->fs_lock);
+	ino->flags &= ~AUTOFS_INF_PENDING;
+	if (status) {
+		spin_unlock(&sbi->fs_lock);
+		return ERR_PTR(status);
+	}
 	spin_unlock(&sbi->fs_lock);
 done:
 	/* Mount succeeded, check if we ended up with a new dentry */
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 0146d911f468..b4e901d2aa6b 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -332,8 +332,6 @@  static int validate_request(struct autofs_wait_queue **wait,
 					dentry = new;
 			}
 		}
-		if (have_submounts(dentry))
-			valid = 0;
 
 		if (new)
 			dput(new);
diff --git a/fs/internal.h b/fs/internal.h
index b71deeecea17..c26ba59eec1b 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -58,7 +58,6 @@  extern int vfs_path_lookup(struct dentry *, struct vfsmount *,
 extern void *copy_mount_options(const void __user *);
 extern char *copy_mount_string(const void __user *);
 
-extern struct vfsmount *lookup_mnt(struct path *);
 extern int finish_automount(struct vfsmount *, struct path *);
 
 extern int sb_prepare_remount_readonly(struct super_block *);
diff --git a/include/linux/mount.h b/include/linux/mount.h
index f822c3c11377..ff971448152f 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -72,6 +72,7 @@  struct vfsmount {
 struct file; /* forward dec */
 struct path;
 
+extern struct vfsmount *lookup_mnt(struct path *);
 extern int mnt_want_write(struct vfsmount *mnt);
 extern int mnt_want_write_file(struct file *file);
 extern int mnt_clone_write(struct vfsmount *mnt);