DoS with unprivileged mounts

There's a simple and effective way to prevent unlink(2) and rename(2) from operating on any file or directory by simply mounting something on it. In any mount instance in any namespace.

Was this considered in the unprivileged mount design?

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

Quoted 6 lines · Miklos Szeredi

There's a simple and effective way to prevent unlink(2) and rename(2) from operating on any file or directory by simply mounting something on it. In any mount instance in any namespace.

Was this considered in the unprivileged mount design?

Quoted 2 lines

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

I'd actually prefer the reverse: unprivileged mounts don't prevent unlink and rename. If the dentry goes away, then the mount could still exist, sans underlying file. (This is already supported on network filesystems.)

Quoted 7 lines · Andy Lutomirski

Quoted 6 lines · Miklos Szeredi

There's a simple and effective way to prevent unlink(2) and rename(2) from operating on any file or directory by simply mounting something on it. In any mount instance in any namespace.

Was this considered in the unprivileged mount design?

Quoted 7 lines

Quoted 2 lines

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

I'd actually prefer the reverse: unprivileged mounts don't prevent unlink and rename. If the dentry goes away, then the mount could still exist, sans underlying file. (This is already supported on network filesystems.)

Of course we do this in network filesystems by pretending the rename/unlink did not actually happen. The vfs insists that it be lied to instead of mirroring what actually happened.

Again all of this is a question about efficient data structures and not really one of semantics. Can either semantic be implemented in such a way that it does not slow down the vfs?

Quoted 17 lines · Eric W. Biederman

Quoted 7 lines · Andy Lutomirski

Quoted 6 lines · Miklos Szeredi

There's a simple and effective way to prevent unlink(2) and rename(2) from operating on any file or directory by simply mounting something on it. In any mount instance in any namespace.

Was this considered in the unprivileged mount design?

Quoted 7 lines

Quoted 2 lines

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

I'd actually prefer the reverse: unprivileged mounts don't prevent unlink and rename. If the dentry goes away, then the mount could still exist, sans underlying file. (This is already supported on network filesystems.)

Quoted 7 lines

Of course we do this in network filesystems by pretending the rename/unlink did not actually happen. The vfs insists that it be lied to instead of mirroring what actually happened.

Again all of this is a question about efficient data structures and not really one of semantics. Can either semantic be implemented in such a way that it does not slow down the vfs?

Given that vfs_unlink has:

	if (d_mountpoint(dentry))
		error = -EBUSY;

I think it's just a matter of changing / deleting that code.

There's a simple and effective way to prevent unlink(2) and rename(2) from operating on any file or directory by simply mounting something on it. In any mount instance in any namespace.

Was this considered in the unprivileged mount design?

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

Such volatile mounts would be useful in general too.

Quoted 4 lines

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

Such volatile mounts would be useful in general too.

Agreed.

This is a problem that is a general pain with mount namespaces in general.

I think the real technical hurdle is finding the mounts t in some random mount namespace. Once we can do that relatively efficiently the rest becomes simple.

Quoted 5 lines · Eric W. Biederman

Quoted 4 lines

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

Such volatile mounts would be useful in general too.

Quoted 8 lines

Agreed.

This is a problem that is a general pain with mount namespaces in general.

I think the real technical hurdle is finding the mounts t in some random mount namespace. Once we can do that relatively efficiently the rest becomes simple.

We already have a "struct mountpoint" hashed on the dentry. Chaining mounts on that mountpoint would be trivial. And we need a MNT_VOLATILE flag and that's it. If we fear that traversing the list of mounts on the dentry to check for non-volatile ones then we could also add a separate volatile counter to struct mountpoint and a matching flag to the dentry. But I don't think that's really necessary.

Quoted 17 lines · Miklos Szeredi

Quoted 5 lines · Eric W. Biederman

Quoted 4 lines

The solution is also theoretically simple: mounts in unpriv namespaces are marked "volatile" and are dissolved on an unlink type operation.

Such volatile mounts would be useful in general too.

Quoted 8 lines

Agreed.

This is a problem that is a general pain with mount namespaces in general.

I think the real technical hurdle is finding the mounts t in some random mount namespace. Once we can do that relatively efficiently the rest becomes simple.

Quoted 7 lines

We already have a "struct mountpoint" hashed on the dentry. Chaining mounts on that mountpoint would be trivial. And we need a MNT_VOLATILE flag and that's it. If we fear that traversing the list of mounts on the dentry to check for non-volatile ones then we could also add a separate volatile counter to struct mountpoint and a matching flag to the dentry. But I don't think that's really necessary.

*Blink* I had overlooked "struct mountpoint". That indeed makes things easier.

I agree we can chain "struct mount" on "struct mountpoint" and then we would have an efficient implementation, that does not impact the vfs fast path.

After that it becomes a question of permissions and semantics.

I am in the process of adopting the rule that something that is not visible at the time we copy a set of mounts should not become visible in the child mount namespace. Grr. This has been a busy month and despite having been reviewed I haven't gotten around to pushing that patch to linux-next.

But MNT_VOLATILE by definition can not reveal anything becasue the underlying mount point is removed, so that all of that weirdness is in propogating mounts between mount namespaces is not relevant here.

This is however the propogation of an unmount between mount namespaces.

In general we don't even need the MNT_VOLATILE flag we just need the appropriate permission checks. However we do need something like MNT_VOLATILE to prevent surprises. MNT_VOLATILE would be used to prevent things like:

# mount --bind /  /mnt
# rmdir /mnt/usr

I think the root user would be rather annoyed if that worked, so it does appear we need something like MNT_VOLATILE.

Part of me does prefer the semantics Andy has suggested where instead of unmounting things we have something like a skeleton of the mount tree unioned with dcaches of the filesystems themselves. With "struct mountpoint" we are amazing close to that already.

Quoted 6 lines · Eric W. Biederman

Miklos Szeredi <[email protected]> writes:

Part of me does prefer the semantics Andy has suggested where instead of unmounting things we have something like a skeleton of the mount tree unioned with dcaches of the filesystems themselves. With "struct mountpoint" we are amazing close to that already.

Two possible nasty cases:

1. mount whatever /tmp/foo/bar; rmdir /tmp/foo/bar; rmdir /tmp/foo

Presumably ls /tmp shouldn't show foo. Should cd /tmp/foo/bar work? What about umount /tmp/foo/bar? What about cd /tmp/foo?

2. mount whatever /tmp/foo; rmdir /tmp/foo; mkdir /tmp/foo

Ugh.

Part of me does prefer the semantics Andy has suggested where instead of unmounting things we have something like a skeleton of the mount tree unioned with dcaches of the filesystems themselves. With "struct mountpoint" we are amazing close to that already.

A mount skeleton would allow us to always remove and rename directories and files without really caring, about what mounts were present. Probably with just a quick lookup to see if we need to set DCACHE_MOUNTED.

The big practical problem I can see with MNT_VOLATILE is mount points in shared directories like /tmp but without the sticky set. At which point it would be possible to delete another users mount points. Perhaps we need restrictions on where a user can mount.

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/mount.h     |    2 ++
 fs/namespace.c |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

fs/mount.h +2 -0

index 64a858143ff9..e4342b8dfab1 100644
@@ -21,6 +21,7 @@ struct mnt_pcp {
 struct mountpoint {
 	struct list_head m_hash;
 	struct dentry *m_dentry;
+	struct list_head m_list;
 	int m_count;
 };
 

@@ -47,6 +48,7 @@ struct mount {
 	struct mount *mnt_master;	/* slave is on master->mnt_slave_list */
 	struct mnt_namespace *mnt_ns;	/* containing namespace */
 	struct mountpoint *mnt_mp;	/* where is it mounted */
+	struct list_head mnt_mp_list;	/* list mounts with the same mountpoint */
 #ifdef CONFIG_FSNOTIFY
 	struct hlist_head mnt_fsnotify_marks;
 	__u32 mnt_fsnotify_mask;

fs/namespace.c +2 -0

index da5c49483430..d092964fe7f9 100644
@@ -197,6 +197,7 @@ static struct mount *alloc_vfsmnt(const char *name)
 		INIT_LIST_HEAD(&mnt->mnt_share);
 		INIT_LIST_HEAD(&mnt->mnt_slave_list);
 		INIT_LIST_HEAD(&mnt->mnt_slave);
+		INIT_LIST_HEAD(&mnt->mnt_mp_list);
 #ifdef CONFIG_FSNOTIFY
 		INIT_HLIST_HEAD(&mnt->mnt_fsnotify_marks);
 #endif

@@ -636,6 +637,7 @@ static struct mountpoint *new_mountpoint(struct dentry *dentry)
 	mp->m_dentry = dentry;
 	mp->m_count = 1;
 	list_add(&mp->m_hash, chain);
+	INIT_LIST_HEAD(&mp->m_list);
 	return mp;
 }
 

fs/namespace.c +1 -0

 }
 
 /*

@@ -705,6 +708,7 @@ void mnt_set_mountpoint(struct mount *mnt,
 	child_mnt->mnt_mountpoint = dget(mp->m_dentry);
 	child_mnt->mnt_parent = mnt;
 	child_mnt->mnt_mp = mp;
+	list_add_tail(&child_mnt->mnt_mp_list, &mp->m_list);
 }
 
 /*

fs/namespace.c +0 -0

 		}
 		change_mnt_propagation(p, MS_PRIVATE);
 	}

Signature · 1 line

1.7.5.4

fs/namespace.c static void detach_mnt(struct mount *mnt, struct path *old_path) · line 696

Quoted 5 lines

fs/namespace.c +1 -0

@@ -691,6 +693,7 @@ static void detach_mnt(struct mount *mnt, struct path *old_path)
 	list_del_init(&mnt->mnt_hash);
 	put_mountpoint(mnt->mnt_mp);
 	mnt->mnt_mp = NULL;
+	list_del_init(&mnt->mnt_mp_list);

Should be done *before* put_mountpoint(), for obvious reasons.

And a BUG_ON(!list_empty(&mp->m_list)) in put_mountpoint() for good measure (and no, WARN_ON() is not better here, since use-after-free is definitely worse than a BUG).

Quoted 12 lines

fs/namespace.c +1 -0

 }
 
 /*

@@ -705,6 +708,7 @@ void mnt_set_mountpoint(struct mount *mnt,
 	child_mnt->mnt_mountpoint = dget(mp->m_dentry);
 	child_mnt->mnt_parent = mnt;
 	child_mnt->mnt_mp = mp;
+	list_add_tail(&child_mnt->mnt_mp_list, &mp->m_list);
 }
 
 /*

fs/namespace.c void umount_tree(struct mount *mnt, int propagate) · line 1200

Quoted 4 lines

fs/namespace.c +1 -0

@@ -1193,6 +1197,7 @@ void umount_tree(struct mount *mnt, int propagate)
 			p->mnt_parent->mnt_ghosts++;
 			put_mountpoint(p->mnt_mp);
 			p->mnt_mp = NULL;
+			list_del_init(&mnt->mnt_mp_list);

Ditto.

Quoted 6 lines

fs/namespace.c +0 -0

 		}
 		change_mnt_propagation(p, MS_PRIVATE);
 	}

Signature · 1 line

1.7.5.4

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/mount.h     |    1 +
 fs/namespace.c |   24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

fs/mount.h +1 -0

index e4342b8dfab1..7a6a2bb3f290 100644
@@ -79,6 +79,7 @@ static inline int is_mounted(struct vfsmount *mnt)
 }
 
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *, int);
+extern void detach_mounts(struct dentry *dentry);
 
 static inline void get_mnt_ns(struct mnt_namespace *ns)
 {

fs/namespace.c +2 -0

index d092964fe7f9..8eaee0c14fdb 100644
@@ -1294,6 +1294,30 @@ static int do_umount(struct mount *mnt, int flags)
 	return retval;
 }
 
+void detach_mounts(struct dentry *dentry)
+{

fs/namespace.c +12 -0

+		return;
+	}
+	br_write_lock(&vfsmount_lock);
+	list_for_each_entry_safe(mnt, next, &mp->m_list, mnt_mp_list) {
+		if (!list_empty(&mnt->mnt_list))
+			umount_tree(mnt, 1);
+	}
+	br_write_unlock(&vfsmount_lock);
+	put_mountpoint(mp);
+	namespace_unlock();
+}
+
 /* 
  * Is the caller allowed to modify his namespace?
  */

Signature · 1 line

1.7.5.4

Quoted 27 lines · Eric W. Biederman

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/mount.h     |    1 +
 fs/namespace.c |   24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

fs/mount.h +1 -0

index e4342b8dfab1..7a6a2bb3f290 100644
@@ -79,6 +79,7 @@ static inline int is_mounted(struct vfsmount *mnt)
 }
 
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *, int);
+extern void detach_mounts(struct dentry *dentry);
 
 static inline void get_mnt_ns(struct mnt_namespace *ns)
 {

fs/namespace.c +0 -0

index d092964fe7f9..8eaee0c14fdb 100644
@@ -1294,6 +1294,30 @@ static int do_umount(struct mount *mnt, int flags)
 	return retval;
 }
 

fs/namespace.c static int do_umount(struct mount *mnt, int flags) · line 1308

Quoted 12 lines

fs/namespace.c +12 -0

+void detach_mounts(struct dentry *dentry)
+{
+	struct mount *mnt, *next;
+	struct mountpoint *mp;
+
+	namespace_lock();
+	if (!d_mountpoint(dentry)) {
+		namespace_unlock();
+		return;
+	}
+	mp = new_mountpoint(dentry);
+	if (IS_ERR(mp)) {

	namespace_unlock();

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/mount.h     |    1 +
 fs/namespace.c |   24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

fs/mount.h +1 -0

index e4342b8dfab1..7a6a2bb3f290 100644
@@ -79,6 +79,7 @@ static inline int is_mounted(struct vfsmount *mnt)
 }
 
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *, int);
+extern void detach_mounts(struct dentry *dentry);
 
 static inline void get_mnt_ns(struct mnt_namespace *ns)
 {

fs/namespace.c +6 -0

index d092964fe7f9..8eaee0c14fdb 100644
@@ -1294,6 +1294,30 @@ static int do_umount(struct mount *mnt, int flags)
 	return retval;
 }
 
+void detach_mounts(struct dentry *dentry)
+{
+	struct mount *mnt, *next;
+	struct mountpoint *mp;
+
+	namespace_lock();

fs/namespace.c +8 -0

+		if (!list_empty(&mnt->mnt_list))
+			umount_tree(mnt, 1);
+	}
+	br_write_unlock(&vfsmount_lock);
+	put_mountpoint(mp);
+	namespace_unlock();
+}
+
 /* 
  * Is the caller allowed to modify his namespace?
  */

Signature · 1 line

1.7.5.4

Quoted 14 lines · Eric W. Biederman

fs/namespace.c +13 -0

+void detach_mounts(struct dentry *dentry)
+{
+	struct mount *mnt, *next;
+	struct mountpoint *mp;
+
+	namespace_lock();
+	if (!d_mountpoint(dentry)) {
+		namespace_unlock();
+		return;
+	}
+	mp = new_mountpoint(dentry);
+	if (IS_ERR(mp)) {
+		return;

fs/namespace.c static int do_umount(struct mount *mnt, int flags) · line 1312

Quoted 3 lines

fs/namespace.c +3 -0

+	}
+	br_write_lock(&vfsmount_lock);
+	list_for_each_entry_safe(mnt, next, &mp->m_list, mnt_mp_list) {

I don't think list_for_each_entry_safe is actually safe enough here. Because propageted umounts will likely remove other mounts from the same mountpoint as well. Doing it with "while (!list_empty(&mp->m_list))" should be better, and AFAICS it will always make progress.

With the introduction of mount namespaces and bind mounts it because possible to access files and directories that in other locations in were used as mount points. Especially with mount namespaces has become very confusing why rm -rf somedir return -EBUSY because some directory is mounted somewhere else. With the addition of user namespaces allowing unprivileged mounts this condition has gone from annoying to allowing a DOS attack on more privileged users.

The simplest approach appears to be to remove the -EBUSY message, allow unlink and rename, and lazily unmount the mount point.

In most cases this is less surprising as this is an implementation of the normal unix behavior of allowing unlinking of files.

The change implemented in this patch allows the following to succeed:

The vfs does not currently follow paths up to the final component for the rename and unlink system calls making the boldest version of this idea the simplest to implement. Which should it simple to spot problems with this idea.

While different from our historical behavior this change does not look like it will break anything, or introduce any security vulnerabilities. In a quick survey of all of the common mount points on linux systems I found mount points in directories owned and modifiable by root, and fuse fuse mounts in directories owned by the ``mounter'' of the fuse filesystem. In both of these cases relying on the permissions of the directory does not practically change the user who is allowed to unmount the filesystem.

Attempting to anticipate cases I have not witnessed I observe that every directory in a trusted path to a file must limit modification such that no one else may modify that directory. For files trusted by suid root executables root most own and be the only user capable of modifying the directory and all parent directories for the files to be safe. Therefore for mount points part of a trusted path only root should be able to unlink any directory or file on that path. Which means after this change for a secured path only root can unmount directories.

For mount points part of a path we can not trust we should not care if the just disappear, as that is just another kind of arbitrary manipulation.

So I conclude that the existing conditions will ensure that the permissions on directories will be sufficiently limited that the new unmount on unlink behavior will not cause problems.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/afs/dir.c           |    3 +-
 fs/dcache.c            |   80 ++++++++++++++++++++----------------------------
 fs/fuse/dir.c          |    3 +-
 fs/gfs2/dentry.c       |    4 +--
 fs/namei.c             |   31 ++++++------------
 fs/nfs/dir.c           |    5 +--
 fs/sysfs/dir.c         |    9 +-----
 include/linux/dcache.h |    3 +-
 8 files changed, 51 insertions(+), 87 deletions(-)

fs/afs/dir.c +1 -2

index 646337dc5201..7fb69d45f1b9 100644
@@ -686,8 +686,7 @@ not_found:
 
 out_bad:
 	/* don't unhash if we have submounts */
-	if (check_submounts_and_drop(dentry) != 0)
-		goto out_skip;
+	shrink_submounts_and_drop(dentry);
 
 	_debug("dropping dentry %s/%s",
 	       parent->d_name.name, dentry->d_name.name);

fs/dcache.c +21 -46

index 41000305d716..1e9bf96b0132 100644
@@ -1373,7 +1373,7 @@ int d_set_mounted(struct dentry *dentry)
 	int ret = -ENOENT;
 	write_seqlock(&rename_lock);
 	for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
-		/* Need exclusion wrt. check_submounts_and_drop() */
+		/* Need exclusion wrt. shrink_submounts_and_drop() */
 		spin_lock(&p->d_lock);
 		if (unlikely(d_unhashed(p))) {
 			spin_unlock(&p->d_lock);

@@ -1478,70 +1478,56 @@ void shrink_dcache_parent(struct dentry *parent)
 }
 EXPORT_SYMBOL(shrink_dcache_parent);
 
-static enum d_walk_ret check_and_collect(void *_data, struct dentry *dentry)
+struct detach_data {
+	struct dentry *found;
+};
+static enum d_walk_ret do_detach_submounts(void *ptr, struct dentry *dentry)
 {
-	struct select_data *data = _data;
-
-	if (d_mountpoint(dentry)) {
-		data->found = -EBUSY;
-		return D_WALK_QUIT;
-	}
-
-	return select_collect(_data, dentry);
-}
+	struct detach_data *data = ptr;
 
-static void check_and_drop(void *_data)
-{
-	struct select_data *data = _data;
+	if (d_mountpoint(dentry))
+		data->found = dentry;
 
-	if (d_mountpoint(data->start))
-		data->found = -EBUSY;
-	if (!data->found)
-		__d_drop(data->start);
+	return data->found ? D_WALK_QUIT : D_WALK_CONTINUE;
 }
 
 /**
- * check_submounts_and_drop - prune dcache, check for submounts and drop
+ * detach_submounts - check for submounts and detach them.
  *
- * All done as a single atomic operation relative to has_unlinked_ancestor().
- * Returns 0 if successfully unhashed @parent.  If there were submounts then
- * return -EBUSY.
+ * @dentry: dentry to find mount points under.
  *
- * @dentry: dentry to prune and drop
+ * If dentry or any of it's children is a mount point detach those mounts.
  */
-int check_submounts_and_drop(struct dentry *dentry)
+void detach_submounts(struct dentry *dentry)
 {
-	int ret = 0;
-
-	/* Negative dentries can be dropped without further checks */
-	if (!dentry->d_inode) {
-		d_drop(dentry);
-		goto out;
-	}
-
+	struct detach_data data;
 	for (;;) {
-		struct select_data data;
-
-		INIT_LIST_HEAD(&data.dispose);
-		data.start = dentry;
-		data.found = 0;
+		data.found = NULL;
+		d_walk(dentry, &data, do_detach_submounts, NULL);
 
-		d_walk(dentry, &data, check_and_collect, check_and_drop);
-		ret = data.found;
-
-		if (!list_empty(&data.dispose))
-			shrink_dentry_list(&data.dispose);
-
-		if (ret <= 0)
+		if (!data.found)
 			break;
 
+		detach_mounts(data.found);
 		cond_resched();
 	}
+	detach_mounts(dentry);
+}
 
-out:
-	return ret;
+/**

fs/dcache.c +2 -1

+	shrink_dcache_parent(dentry);
 }
-EXPORT_SYMBOL(check_submounts_and_drop);
+EXPORT_SYMBOL(shrink_submounts_and_drop);
 
 /**
  * __d_alloc	-	allocate a dcache entry

fs/fuse/dir.c +1 -2

index 62b43b577bfc..b1cd7b79a325 100644
@@ -259,8 +259,7 @@ out:
 
 invalid:
 	ret = 0;
-	if (check_submounts_and_drop(entry) != 0)
-		ret = 1;
+	shrink_submounts_and_drop(entry);
 	goto out;
 }
 

fs/gfs2/dentry.c +1 -3

index d3a5d4e29ba5..2ecc2b873829 100644
@@ -93,9 +93,7 @@ invalid_gunlock:
 	if (!had_lock)
 		gfs2_glock_dq_uninit(&d_gh);
 invalid:
-	if (check_submounts_and_drop(dentry) != 0)
-		goto valid;
-
+	shrink_submounts_and_drop(dentry);
 	dput(parent);
 	return 0;
 

fs/namei.c +6 -11

index 645268f23eb6..b18b017c946b 100644
@@ -3560,10 +3560,6 @@ int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 	dget(dentry);
 	mutex_lock(&dentry->d_inode->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(dentry))
-		goto out;
-
 	error = security_inode_rmdir(dir, dentry);
 	if (error)
 		goto out;

@@ -3575,6 +3571,7 @@ int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 
 	dentry->d_inode->i_flags |= S_DEAD;
 	dont_mount(dentry);
+	detach_mounts(dentry);
 
 out:
 	mutex_unlock(&dentry->d_inode->i_mutex);

@@ -3657,14 +3654,12 @@ int vfs_unlink(struct inode *dir, struct dentry *dentry)
 		return -EPERM;
 
 	mutex_lock(&dentry->d_inode->i_mutex);
-	if (d_mountpoint(dentry))
-		error = -EBUSY;
-	else {
-		error = security_inode_unlink(dir, dentry);
+	error = security_inode_unlink(dir, dentry);
+	if (!error) {
+		error = dir->i_op->unlink(dir, dentry);
 		if (!error) {
-			error = dir->i_op->unlink(dir, dentry);
-			if (!error)
-				dont_mount(dentry);
+			dont_mount(dentry);
+			detach_mounts(dentry);
 		}
 	}
 	mutex_unlock(&dentry->d_inode->i_mutex);

fs/namei.c +1 -4

 	error = -EMLINK;
 	if (max_links && !target && new_dir != old_dir &&
 	    new_dir->i_nlink >= max_links)

@@ -4006,6 +3997,7 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
 	if (target) {
 		target->i_flags |= S_DEAD;
 		dont_mount(new_dentry);
+		detach_mounts(new_dentry);
 	}
 out:
 	if (target)

@@ -4031,16 +4023,15 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 	if (target)
 		mutex_lock(&target->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
-		goto out;
-

fs/namei.c +0 -0

 	if (!(old_dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE))
 		d_move(old_dentry, new_dentry);
 out:

fs/nfs/dir.c +1 -4

index 854a8f05a610..e8e35acd8850 100644
@@ -1142,10 +1142,7 @@ out_zap_parent:
 		if (dentry->d_flags & DCACHE_DISCONNECTED)
 			goto out_valid;
 	}
-	/* If we have submounts, don't unhash ! */
-	if (check_submounts_and_drop(dentry) != 0)
-		goto out_valid;
-
+	shrink_submounts_and_drop(dentry);
 	dput(parent);
 	dfprintk(LOOKUPCACHE, "NFS: %s(%s/%s) is invalid\n",
 			__func__, dentry->d_parent->d_name.name,

fs/sysfs/dir.c +1 -8

index 4d83cedb9fcb..477c66d4e2a8 100644
@@ -327,7 +327,6 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, unsigned int flags)
 	}
 
 	mutex_unlock(&sysfs_mutex);
-out_valid:
 	return 1;
 out_bad:
 	/* Remove the dentry from the dcache hashes.

@@ -341,13 +340,7 @@ out_bad:
 	 * to the dcache hashes.
 	 */
 	mutex_unlock(&sysfs_mutex);
-
-	/* If we have submounts we must allow the vfs caches
-	 * to lie about the state of the filesystem to prevent
-	 * leaks and other nasty things.
-	 */
-	if (check_submounts_and_drop(dentry) != 0)
-		goto out_valid;
+	shrink_submounts_and_drop(dentry);
 
 	return 0;
 }

include/linux/dcache.h +2 -1

index 59066e0b4ff1..17948b49f3d5 100644
@@ -254,7 +254,8 @@ extern void d_prune_aliases(struct inode *);
 
 /* test whether we have any submounts in a subdir tree */
 extern int have_submounts(struct dentry *);
-extern int check_submounts_and_drop(struct dentry *);
+extern void detach_submounts(struct dentry *dentry);
+extern void shrink_submounts_and_drop(struct dentry *);
 
 /*
  * This adds the entry to the hash queues.

Signature · 1 line

1.7.5.4

fs/dcache.c void shrink_dcache_parent(struct dentry *parent) · line 1527

Quoted 11 lines · Eric W. Biederman

fs/dcache.c +11 -0

+/**
+ * shrink_submounts_and_drop - detach submounts, prune dcache, and drop
+ *
+ * All done as a single atomic operation reletaive to d_set_mounted().
+ *
+ * @dentry: dentry to detach, prune and drop
+ */
+void shrink_submounts_and_drop(struct dentry *dentry)
+{
+	d_drop(dentry);
+	detach_submounts(dentry);

And here, between detach_submounts() and shrink_dcache_parent() a new mount can be added.

It's not accidental that check_submounts_and_drop() did the check and the drop together, protected by rename_lock and d_lock.

Quoted 7 lines

fs/dcache.c +2 -1

+	shrink_dcache_parent(dentry);
 }
-EXPORT_SYMBOL(check_submounts_and_drop);
+EXPORT_SYMBOL(shrink_submounts_and_drop);

fs/dcache.c static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry, · line 3994

Quoted 7 lines

fs/dcache.c +0 -4

@@ -3988,10 +3983,6 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
 	if (target)
 		mutex_lock(&target->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(old_dentry) || d_mountpoint(new_dentry))
-		goto out;
-

I know of at least one app that relied at some point on a mountpoint (directory or non-directory) not being movable: fusermount uses this to ensure that unprivileged userspace didn't try replacing a fuse mount with a symlink to trick fusermount into umounting an arbitrary path. The code that relied on this was replaced by UMOUNT_NOFOLLOW on kernels where it is supported. But in theory there may exist a running binary without UMOUNT_NOFOLLOW and relying on EBUSY.

And there may be other such horrid hacks out there.

Quoted 23 lines

fs/namei.c +1 -4

 	error = -EMLINK;
 	if (max_links && !target && new_dir != old_dir &&
 	    new_dir->i_nlink >= max_links)

@@ -4006,6 +3997,7 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
 	if (target) {
 		target->i_flags |= S_DEAD;
 		dont_mount(new_dentry);
+		detach_mounts(new_dentry);
 	}
 out:
 	if (target)

@@ -4031,16 +4023,15 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 	if (target)
 		mutex_lock(&target->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
-		goto out;
-
 	error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
 	if (error)
 		goto out;
 

fs/namei.c static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry, · line 4034

Quoted 6 lines

fs/namei.c +4 -1

-	if (target)
+	if (target) {
 		dont_mount(new_dentry);
+		detach_mounts(new_dentry);
+	}
+	detach_mounts(old_dentry);

Why exactly? "Moved file changes contents" is not the least surprising result, IMO. And why the difference between rename-dir and rename-other in this regard?

But I will go through and read the old fusermount code before I get too much farther just so I understand what I am potentially breaking.

Quoted 2 lines · [email protected] (Eric W. Biederman) writes:

But I will go through and read the old fusermount code before I get too much farther just so I understand what I am potentially breaking.

Grr.

So I have just read the fusermount umount code and the hack that it uses before there was UMOUNT_NOFOLLOW support in the vm.

If I walk this path of lazy unmounts and detaching directories, anyone with a new kernel and an old copy of fusermount and a nfs mounted home directory will be able to exploit the fusermount umount symlink race.

Unless we can declare that old fusermount binaries are buggy beyond supporting this patchset as it exists is dead.

Quoted 10 lines · Eric W. Biederman

Quoted 2 lines · [email protected] (Eric W. Biederman) writes:

But I will go through and read the old fusermount code before I get too much farther just so I understand what I am potentially breaking.

Grr.

So I have just read the fusermount umount code and the hack that it uses before there was UMOUNT_NOFOLLOW support in the vm.

Quoted 6 lines

If I walk this path of lazy unmounts and detaching directories, anyone with a new kernel and an old copy of fusermount and a nfs mounted home directory will be able to exploit the fusermount umount symlink race.

Unless we can declare that old fusermount binaries are buggy beyond supporting this patchset as it exists is dead.

What's the hack that it does?

After thinking about it removing the restrictions on mount points appears safe, because it is just plain dumb to have a mount point in a directory that is not restricted to root only modifications.

This is a change in user visible semantics, so I want to be very careful about this. Are there any reasons to not make this change?

Quoted 7 lines · Eric W. Biederman

After thinking about it removing the restrictions on mount points appears safe, because it is just plain dumb to have a mount point in a directory that is not restricted to root only modifications.

This is a change in user visible semantics, so I want to be very careful about this. Are there any reasons to not make this change?

At least one worry: people are very used to 'rmdir()' not removing empty directories, and I've written code myself that just does an 'rmdir()' without worrying about it. I think git has code like "remove file, then try to remove directory file is in, and recurse until it fails or you hit the top of tree". And it all depends on knowing that rmdir() is harmless, and returns the appropriate error when the directory isn't empty.

And you're now changing that. If it's a mount-point, the rmdir just succeeds, afaik.

Does anybody care? I dunno. But it looks like a _big_ semantic change.

That said, I like the _concept_ of being able to remove a mount-point and the mount just goes away. But I do think that for sanity sake, it should have something like "if one of the mounts is in the current namespace, return -EBUSY". IOW, the patch-series would make the VFS layer _able_ to remove mount-points, but a normal rmdir() when something is mounted in that namespace would fail in order to give legacy behavior.

Hmm?

I just noticed that Al's latest vfs changes posted yesterday mean I need to rebase and possibly respin these patches, as all of the locking and interesting bits of the dcache have changed. I don't think the conflicts would be fun to resolve during the merge window.

Quoted 1 line · Linus Torvalds

On Fri, Oct 4, 2013 at 3:41 PM, Eric W. Biederman

Quoted 6 lines · <[email protected]> wrote:

Quoted 6 lines

After thinking about it removing the restrictions on mount points appears safe, because it is just plain dumb to have a mount point in a directory that is not restricted to root only modifications.

This is a change in user visible semantics, so I want to be very

careful

Quoted 5 lines

Quoted 1 line

about this. Are there any reasons to not make this change?

At least one worry: people are very used to 'rmdir()' not removing empty directories, and I've written code myself that just does an 'rmdir()' without worrying about it. I think git has code like

"remove

Quoted 2 lines

file, then try to remove directory file is in, and recurse until it fails or you hit the top of tree". And it all depends on knowing

that

Quoted 7 lines

rmdir() is harmless, and returns the appropriate error when the directory isn't empty.

And you're now changing that. If it's a mount-point, the rmdir just succeeds, afaik.

Does anybody care? I dunno. But it looks like a _big_ semantic

change.

Which is definitley why I am asking and being careful.

Quoted 1 line

That said, I like the _concept_ of being able to remove a

mount-point

Quoted 1 line

and the mount just goes away. But I do think that for sanity sake,

it

Quoted 7 lines

should have something like "if one of the mounts is in the current namespace, return -EBUSY". IOW, the patch-series would make the VFS layer _able_ to remove mount-points, but a normal rmdir() when something is mounted in that namespace would fail in order to give legacy behavior.

Hmm?

In principle I have no problems tweaking rmdir to check for that case.

At the same time the real reason that this is safe is that mount points are almost always part of trusted paths to important files and you just don't mess with those paths.

So tweaking rmdir to fail would be more about making stupid mistakes like running "rm -rf /" fail than it would be about security or correctness.

Quoted 9 lines · Eric W. Biederman

In principle I have no problems tweaking rmdir to check for that case.

At the same time the real reason that this is safe is that mount points
are almost always part of trusted paths to important files and you just
don't mess with those paths.

Quoted 3 lines

So tweaking rmdir to fail would be more about making stupid mistakes like running "rm -rf /" fail than it would be about security or correctness.

If you do an rm -rf descending through a mount point, it's going to delete the contents of the mount point before _trying_ to unlink the mount point, which may be bad for the thing you mounted. Then there's the fun corner case of "the directory wasn't empty before it was mounted on, and umounting revealed the overmounted files, so the unlink fails for that reason even after magic umount".

Doing rmdir on a non-empty directory won't delete it if it _isn't_ a mount point, and presumably we require write access to directories to mount on them, so in what ways is this different than another user mucking about with my files asynchronously?

Oh, attached is a dumb "zapchroot" script I've been using for years to unlink all mount points under a given directory, taking advantage of the fact that mount points are appended to the end of the list so if you unlink from the end to the front you should get the sub-mounts before the parent mounts (modulo mount --move not reordering the list, but that's uncommon).

Recently I noticed some kernels where chroot does _not_ trim the paths so that the paths you see in /proc/mounts are relevant to the current chroot but instead have all sorts of crap you can't access with no way to know what it's talking about. That was sad, I need to go figure out if that was distro breakage or vanilla breakage...

Quoted 1 line · Rob Landley

Oh, attached is a dumb "zapchroot" script I've been using for years

to

Quoted 1 line

unlink all mount points under a given directory, taking advantage

of the

Quoted 1 line

fact that mount points are appended to the end of the list so if

you unlink

Quoted 1 line

from the end to the front you should get the sub-mounts before the

parent

Quoted 1 line

mounts (modulo mount --move not reordering the list, but that's

uncommon).

util-linux umount supports --recursive, it uses /proc/self/mountinfo to compose the hierarchy. The important is that the mountinfo file contains Id and Parent_Id relations, so you don't rely on the order only.

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

With a little luck this may prevent commands stupid commands like rm -rf from eating your system.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namei.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

fs/namei.c +21 -0

index b18b017c946b..b9cae480ac27 100644
@@ -3547,6 +3547,20 @@ void dentry_unhash(struct dentry *dentry)
 	spin_unlock(&dentry->d_lock);
 }
 
+static bool covered(struct vfsmount *mnt, struct dentry *dentry)
+{
+	/* test to see if a dentry is covered with a mount in
+	 * the current mount namespace.
+	 */
+	bool is_covered;
+
+	rcu_read_lock();
+	is_covered = d_mountpoint(dentry) && __lookup_mnt(mnt, dentry, 1);
+	rcu_read_unlock();
+
+	return is_covered;
+}
+
 int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int error = may_delete(dir, dentry, 1);

@@ -3619,6 +3633,9 @@ retry:
 		error = -ENOENT;
 		goto exit3;
 	}
+	error = -EBUSY;
+	if (covered(nd.path.mnt, dentry))
+		goto exit3;
 	error = security_path_rmdir(&nd.path, dentry);
 	if (error)
 		goto exit3;

@@ -4155,6 +4172,10 @@ retry:
 	error = -ENOTEMPTY;
 	if (new_dentry == trap)
 		goto exit5;
+	error = -EBUSY;
+	if (new_dentry->d_inode && S_ISDIR(new_dentry->d_inode->i_mode) &&
+	    covered(newnd.path.mnt, new_dentry))
+		goto exit5;
 
 	error = security_path_rename(&oldnd.path, old_dentry,
 				     &newnd.path, new_dentry);

Signature · 1 line

1.7.5.4

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

With a little luck this may prevent commands stupid commands like rm -rf from eating your system.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namei.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

fs/namei.c +21 -0

index b18b017c946b..b9cae480ac27 100644
@@ -3547,6 +3547,20 @@ void dentry_unhash(struct dentry *dentry)
 	spin_unlock(&dentry->d_lock);
 }
 
+static bool covered(struct vfsmount *mnt, struct dentry *dentry)
+{
+	/* test to see if a dentry is covered with a mount in
+	 * the current mount namespace.
+	 */
+	bool is_covered;
+
+	rcu_read_lock();
+	is_covered = d_mountpoint(dentry) && __lookup_mnt(mnt, dentry, 1);
+	rcu_read_unlock();
+
+	return is_covered;
+}
+
 int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int error = may_delete(dir, dentry, 1);

@@ -3619,6 +3633,9 @@ retry:
 		error = -ENOENT;
 		goto exit3;
 	}
+	error = -EBUSY;
+	if (covered(nd.path.mnt, dentry))
+		goto exit3;
 	error = security_path_rmdir(&nd.path, dentry);
 	if (error)
 		goto exit3;

@@ -4155,6 +4172,10 @@ retry:
 	error = -ENOTEMPTY;
 	if (new_dentry == trap)
 		goto exit5;
+	error = -EBUSY;
+	if (new_dentry->d_inode && S_ISDIR(new_dentry->d_inode->i_mode) &&
+	    covered(newnd.path.mnt, new_dentry))
+		goto exit5;
 
 	error = security_path_rename(&oldnd.path, old_dentry,
 				     &newnd.path, new_dentry);

Signature · 1 line

1.7.5.4

Quoted 5 lines · Eric W. Biederman

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

Do you think we should do the same thing for over-mounted file at vfs_unlink()?

Quoted 53 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namei.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

fs/namei.c +21 -0

index b18b017c946b..b9cae480ac27 100644
@@ -3547,6 +3547,20 @@ void dentry_unhash(struct dentry *dentry)
 	spin_unlock(&dentry->d_lock);
 }
 
+static bool covered(struct vfsmount *mnt, struct dentry *dentry)
+{
+	/* test to see if a dentry is covered with a mount in
+	 * the current mount namespace.
+	 */
+	bool is_covered;
+
+	rcu_read_lock();
+	is_covered = d_mountpoint(dentry) && __lookup_mnt(mnt, dentry, 1);
+	rcu_read_unlock();
+
+	return is_covered;
+}
+
 int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int error = may_delete(dir, dentry, 1);

@@ -3619,6 +3633,9 @@ retry:
 		error = -ENOENT;
 		goto exit3;
 	}
+	error = -EBUSY;
+	if (covered(nd.path.mnt, dentry))
+		goto exit3;
 	error = security_path_rmdir(&nd.path, dentry);
 	if (error)
 		goto exit3;

@@ -4155,6 +4172,10 @@ retry:
 	error = -ENOTEMPTY;
 	if (new_dentry == trap)
 		goto exit5;
+	error = -EBUSY;
+	if (new_dentry->d_inode && S_ISDIR(new_dentry->d_inode->i_mode) &&
+	    covered(newnd.path.mnt, new_dentry))
+		goto exit5;
 
 	error = security_path_rename(&oldnd.path, old_dentry,
 				     &newnd.path, new_dentry);

Signature · 1 line

1.7.5.4

Quoted 9 lines · Serge E. Hallyn

Quoted 5 lines · Eric W. Biederman

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

Do you think we should do the same thing for over-mounted file at vfs_unlink()?

We easily could.

The point of the patch is to just preserve the directory is empty don't allow rmdir to succeed semantics, and as typically we can see something in the directory because of the mount it doesn't make sense for rmdir to succeed.

unlink doesn't have any occassions when the permissions are sufficient to remove a directory where it will fail. So I don't see the point of doing this for anything except directories.

Except for possibly the oddball rmdir semantics mentioned I don't think this patch should be part of anyone's correctness analysis.

It is easiest to see that this series of changes is semantically safe if we are safe to run unprivileged code in a mount namespace where root has locally unmounted every mount point.

We do have the restriction that in a user namespace we can't unmount anything root was mounted outside the user namespace. Which combined with the above patch would be roughly equivalent to todays mount restrictions for the common case. Unfortunately being only roughly equivalent the analysis gets very complicated, and complicated reasoning usually means invalid reasoning.

So if we can feel safe just depending on the parent directory permissions (which are not hidden by a mount) protecting our mount points, I feel much better about this patchset.

Quoted 40 lines · Eric W. Biederman

Quoted 9 lines · Serge E. Hallyn

Quoted 5 lines · Eric W. Biederman

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

Do you think we should do the same thing for over-mounted file at vfs_unlink()?

We easily could.

The point of the patch is to just preserve the directory is empty don't allow rmdir to succeed semantics, and as typically we can see something in the directory because of the mount it doesn't make sense for rmdir to succeed.

unlink doesn't have any occassions when the permissions are sufficient to remove a directory where it will fail. So I don't see the point of doing this for anything except directories.

Except for possibly the oddball rmdir semantics mentioned I don't think this patch should be part of anyone's correctness analysis.

It is easiest to see that this series of changes is semantically safe if we are safe to run unprivileged code in a mount namespace where root has locally unmounted every mount point.

We do have the restriction that in a user namespace we can't unmount anything root was mounted outside the user namespace. Which combined with the above patch would be roughly equivalent to todays mount restrictions for the common case. Unfortunately being only roughly equivalent the analysis gets very complicated, and complicated reasoning usually means invalid reasoning.

Quoted 4 lines

So if we can feel safe just depending on the parent directory permissions (which are not hidden by a mount) protecting our mount points, I feel much better about this patchset.

I feel safe about this (in the safe-against-attack sense, not necessary the safe-against-confused-users sense) because the whole point of preventing unmount of things that were mounted by root is to prevent untrusted users from seeing what's under the mount. But rmdir and unlink don't let you see what was under the mount because they remove it.

(Hmm. I wonder if rename on a mountpoint could work. I've wanted this on occasion.)

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines · Eric W. Biederman

Quoted 7 lines · Serge E. Hallyn

Quoted 5 lines · Eric W. Biederman

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Do you think we should do the same thing for over-mounted file at vfs_unlink()?

We easily could.

Quoted 15 lines · Eric W. Biederman

Quoted 13 lines · Andy Lutomirski

Quoted 12 lines · Eric W. Biederman

Quoted 9 lines · Serge E. Hallyn

Quoted 5 lines · Eric W. Biederman

Programs have been known to test for empty directories by attempting to remove them. To keep from violating the principle of least surprise don't let directories the caller can see with someting mounted on them be deleted.

Do you think we should do the same thing for over-mounted file at vfs_unlink()?

Quoted 1 line

Quoted 1 line

Quoted 1 line

We easily could.

And for vfs_rename()?

I think the risks of changing behavior outweigh the benefits. How many times did you have to remove or rename a mounted file or directory? It's very rarely needed.

Thanks,
Miklos

Quoted 3 lines

I think the risks of changing behavior outweigh the benefits. How many times did you have to remove or rename a mounted file or directory? It's very rarely needed.

I do this every time I reinstall a system while running that system. Admittedly, mount --move works, but that's a really unpleasant interface.

When rename2 gets added, there could be a flag RENAME_MOVE_MOUNT to opt in.

Quoted 4 lines · Andy Lutomirski

Quoted 3 lines

I think the risks of changing behavior outweigh the benefits. How many times did you have to remove or rename a mounted file or directory? It's very rarely needed.

Quoted 5 lines

I do this every time I reinstall a system while running that system. Admittedly, mount --move works, but that's a really unpleasant interface.

When rename2 gets added, there could be a flag RENAME_MOVE_MOUNT to opt in.

Good point. Opting in would be the safest for both unlinkat() and renameat2().

If we are going to fix the VFS deficiency we have to let these changes happen in other mount namespaces. To make that safe it has to be sufficient to rely on the directory permissions and the conditions that ensure that the directory permissions are sufficient.

So I find it far safer to allow as much as possible even in the local mount namespace so we can actually see if there are problems with relying on the directory permissions.

Quoted 4 lines · Eric W. Biederman

If we are going to fix the VFS deficiency we have to let these changes happen in other mount namespaces. To make that safe it has to be sufficient to rely on the directory permissions and the conditions that ensure that the directory permissions are sufficient.

Yes.

Quoted 3 lines

So I find it far safer to allow as much as possible even in the local mount namespace so we can actually see if there are problems with relying on the directory permissions.

But it's not safer in terms of breaking legacy environments, which are single namespace, usually.

I see it this way: the current behavior (blocking unlink/rename if mounted) may not be nice, but it's expected. But once we have
multiple namespaces it becomes problematic. Just like network
filesystems where we cannot prevent unlink on another host just because it's mounted on this. So we have to have some way of dealing with this.

Is it worth changing behavor in the "current namespace" case too? It has advantages, but it's not absolutely necessary and there are unknown dangers. But if we disable it for rmdir() then what's the point in enabling it for unlink()? Directory mounts far outnumber non-directory ones, so the additional testing is minimal, but the holes left are probably just as real.

And there's rename(). We have a real security hole in fusermount by allowing it in the local namespace. I don't think changing the behavior of rename() in the local namespace is important enough to risk such problems. And as Andy pointed out, we can just have an option to turn it on and that's that.

I have been weighing the pros and the cons of this.

At this point the most practical path I can see is to block unlink, rename, and rmdir if there is a mount in the local namespace.

At the very least that makes very limited additions to what applications can depend on from the vfs, and even more importantly that can be ready by 3.13.

Furthermore it removes the nasty need for filesystems to lie to the vfs, and removes the nasty DOS that non-local mountpoints are.

Miklos if you as the fuse maintainer aren't worried about network filesystems, and multiple namespaces I won't worry either. Especially since modern versions of fuse aren't affected.

On Thu, Oct 10, 2013 at 1:43 PM, Eric W. Biederman

Quoted 3 lines

Miklos if you as the fuse maintainer aren't worried about network filesystems, and multiple namespaces I won't worry either. Especially since modern versions of fuse aren't affected.

I think the above conditions (local mount blocks unlink/rename) are enough to prevent most of the problems, of which there aren't many in any case.

Quoted 8 lines · Miklos Szeredi

On Thu, Oct 10, 2013 at 1:43 PM, Eric W. Biederman

Quoted 3 lines

Miklos if you as the fuse maintainer aren't worried about network filesystems, and multiple namespaces I won't worry either. Especially since modern versions of fuse aren't affected.

I think the above conditions (local mount blocks unlink/rename) are enough to prevent most of the problems, of which there aren't many in any case.

Dumb question.

What prevents someone setting up a race between the fusermount permission checks and replacing the destination with a symlink, perhaps to /etc/shadow?

Do we need a MS_NOFOLLOW?

In preparation for allowing mountpoints to be renamed and unlinked in remote filesystems and in other mount namespaces test if on a dentry there is a mount in the local mount namespace before allowing it to be renamed or unlinked.

The primary motivation here are old versions of fusermount unmount which is not safe if the a path can be renamed or unlinked while it is verifying the mount is safe to unmount. More recent versions are simpler and safer by simply using UMOUNT_NOFOLLOW when unmounting a mount in a directory owned by an arbitrary user.

Miklos Szeredi <[email protected]> reports this is approach is good enough to remove concerns about new kernels mixed with old versions
of fusermount.

A secondary motivation for restrictions here is that it removing empty directories that have non-empty mount points on them appears to violate the rule that rmdir can not remove empty directories. As Linus Torvalds pointed out this is useful for programs (like git) that test if a directory is empty with rmdir.

Therefore this patch arranges to enforce the existing mount point semantics for local mount namespace.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/namei.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

fs/namei.c +25 -0

index 645268f23eb6..df7bd6e9c7b6 100644
@@ -3547,6 +3547,20 @@ void dentry_unhash(struct dentry *dentry)
 	spin_unlock(&dentry->d_lock);
 }
 
+static bool covered(struct vfsmount *mnt, struct dentry *dentry)
+{
+	/* test to see if a dentry is covered with a mount in
+	 * the current mount namespace.
+	 */
+	bool is_covered;
+
+	rcu_read_lock();
+	is_covered = d_mountpoint(dentry) && __lookup_mnt(mnt, dentry, 1);
+	rcu_read_unlock();
+
+	return is_covered;
+}
+
 int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int error = may_delete(dir, dentry, 1);

@@ -3622,6 +3636,9 @@ retry:
 		error = -ENOENT;
 		goto exit3;
 	}
+	error = -EBUSY;
+	if (covered(nd.path.mnt, dentry))
+		goto exit3;
 	error = security_path_rmdir(&nd.path, dentry);
 	if (error)
 		goto exit3;

@@ -3716,6 +3733,9 @@ retry:
 		inode = dentry->d_inode;
 		if (!inode)
 			goto slashes;
+		error = -EBUSY;
+		if (covered(nd.path.mnt, dentry))
+			goto exit2;
 		ihold(inode);
 		error = security_path_unlink(&nd.path, dentry);
 		if (error)

@@ -4164,6 +4184,11 @@ retry:
 	error = -ENOTEMPTY;
 	if (new_dentry == trap)
 		goto exit5;
+	error = -EBUSY;
+	if (covered(oldnd.path.mnt, old_dentry))
+		goto exit5;
+	if (covered(newnd.path.mnt, new_dentry))
+		goto exit5;
 
 	error = security_path_rename(&oldnd.path, old_dentry,
 				     &newnd.path, new_dentry);

Signature · 1 line

1.7.5.4

fs/namei.c retry: · line 3641

Quoted 10 lines · Eric W. Biederman

int vfs_rmdir(struct inode *dir, struct dentry *dentry) {

fs/namei.c +3 -0

 	int error = may_delete(dir, dentry, 1);

@@ -3622,6 +3636,9 @@ retry:
 		error = -ENOENT;
 		goto exit3;
 	}
+	error = -EBUSY;
+	if (covered(nd.path.mnt, dentry))
+		goto exit3;

Ugh... And it's not racy because of...? IOW, what's to keep the return value of covered() from getting obsolete just as it's being calculated, let alone returned?

The return value of d_mountpoint can be obsolete as soon as it returns as well, so I don't see this as being significantly different.

In 3.12 vfs_rmdir checks d_mountpoint with the
dentry->d_inode->i_mutex and
dentry->d_parent->d_inode->i_mutex held.

In 3.12 vfs_unlink checks d_mountpoint with the
dentry->d_inode->i_mutex and
dentry->d_parent->d_inode->i_mutex hel.d

In 3.12 vfs_rename_dir and vfs_rename_other checks d_mountpint with the target->i_mutex, new_dir->i_mutex, and old_dir->i_mutex held.

Therefore the guarantees in 3.12 are:
- unlink versus mount races are prevented by the dentry->d_inode->i_mutex of the dentry being removed.
- unlink versus umount races are uninteresting.
- mount versus rename races in testing of d_mountpoint are ignored.

So comparing this to how I have implemented covered the test is at a slightly different location in the call path so there may be a slightly larger race in rename.

Quoted 2 lines · Eric W. Biederman

The return value of d_mountpoint can be obsolete as soon as it returns as well, so I don't see this as being significantly different.

Not if the ->i_mutex of that sucker is held. And it *is* held in vfs_unlink/vfs_rmdir/vfs_rename. Note that we only care about a mountpoint being falsely assumed to be a non-mountpoint - in the other direction we can just shrug and say that we'd won the race and got EBUSY for that.

Quoted 12 lines

In 3.12 vfs_rmdir checks d_mountpoint with the
dentry->d_inode->i_mutex and
dentry->d_parent->d_inode->i_mutex held.

In 3.12 vfs_unlink checks d_mountpoint with the
dentry->d_inode->i_mutex and
dentry->d_parent->d_inode->i_mutex hel.d

In 3.12 vfs_rename_dir and vfs_rename_other checks d_mountpint with the target->i_mutex, new_dir->i_mutex, and old_dir->i_mutex held.

Quoted 5 lines

Therefore the guarantees in 3.12 are:
- unlink versus mount races are prevented by the dentry->d_inode->i_mutex of the dentry being removed.
- unlink versus umount races are uninteresting.
- mount versus rename races in testing of d_mountpoint are ignored.

Read what you've written a few lines above. The part about target->i_mutex being held.

Quoted 3 lines

So comparing this to how I have implemented covered the test is at a slightly different location in the call path so there may be a slightly larger race in rename.

You've got a race in unlink. You've got a race in rename. You've got a race in rmdir. And none of those had that race in 3.12 (including rename()).

BTW, could you describe the races with umount in a bit more details? Races with mount are simple - rmdir() sees that victim isn't a mountpoint and proceeds, mount() sees that victim is still alive and proceeds, despite the fact that victim is irretrievably on the way to removal. And that's what ->i_mutex on victim prevents, making "check for d_mountpoint / remove / call dont_mount()" atomic wrt mount(). What is the problem you are seeing with umount()? rmdir() getting EBUSY because it hasn't noticed umount() happening in parallel with it? Legitimate behaviour, as far I can see... Or is it about something different?

Quoted 2 lines

Read what you've written a few lines above. The part about target->i_mutex being held.

That works for the rename as unlink case but we don't hold old_dentry->d_inode->i_mutex which is what is needed to prevent a mount on the dentry we are renaming.

Quoted 6 lines · Eric W. Biederman

Quoted 2 lines

Read what you've written a few lines above. The part about target->i_mutex being held.

That works for the rename as unlink case but we don't hold old_dentry->d_inode->i_mutex which is what is needed to prevent a mount on the dentry we are renaming.

It will be held in 3.13.

The return value of d_mountpoint can be obsolete as soon as it returns as well, so I don't see this as being significantly different.

In 3.12 vfs_rmdir checks d_mountpoint with the
dentry->d_inode->i_mutex and
dentry->d_parent->d_inode->i_mutex held.

In 3.12 vfs_unlink checks d_mountpoint with the
dentry->d_inode->i_mutex and
dentry->d_parent->d_inode->i_mutex hel.d

In 3.12 vfs_rename_dir and vfs_rename_other checks d_mountpint with the target->i_mutex, new_dir->i_mutex, and old_dir->i_mutex held.

Therefore the guarantees in 3.12 are:
- unlink versus mount races are prevented by the dentry->d_inode->i_mutex of the dentry being removed.
- unlink versus umount races are uninteresting.
- mount versus rename races in testing of d_mountpoint are ignored.

So comparing this to how I have implemented covered the test is at a slightly different location in the call path so there may be a slightly larger race in rename.

fs/namei.c retry: · line 3641

Quoted 10 lines · Eric W. Biederman

int vfs_rmdir(struct inode *dir, struct dentry *dentry) {

fs/namei.c +3 -0

 	int error = may_delete(dir, dentry, 1);

@@ -3622,6 +3636,9 @@ retry:
 		error = -ENOENT;
 		goto exit3;
 	}
+	error = -EBUSY;
+	if (covered(nd.path.mnt, dentry))
+		goto exit3;

Ugh... And it's not racy because of...? IOW, what's to keep the return value of covered() from getting obsolete just as it's being calculated, let alone returned?

To spot any possible problems call BUG if a mountpoint is put when it's list of mounts is not empty.

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/mount.h     |    2 ++
 fs/namespace.c |    6 ++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

fs/mount.h +2 -0

index 64a858143ff9..e4342b8dfab1 100644
@@ -21,6 +21,7 @@ struct mnt_pcp {
 struct mountpoint {
 	struct list_head m_hash;
 	struct dentry *m_dentry;
+	struct list_head m_list;
 	int m_count;
 };
 

@@ -47,6 +48,7 @@ struct mount {
 	struct mount *mnt_master;	/* slave is on master->mnt_slave_list */
 	struct mnt_namespace *mnt_ns;	/* containing namespace */
 	struct mountpoint *mnt_mp;	/* where is it mounted */
+	struct list_head mnt_mp_list;	/* list mounts with the same mountpoint */
 #ifdef CONFIG_FSNOTIFY
 	struct hlist_head mnt_fsnotify_marks;
 	__u32 mnt_fsnotify_mask;

fs/namespace.c +6 -0

index da5c49483430..e4fe22c23e00 100644
@@ -197,6 +197,7 @@ static struct mount *alloc_vfsmnt(const char *name)
 		INIT_LIST_HEAD(&mnt->mnt_share);
 		INIT_LIST_HEAD(&mnt->mnt_slave_list);
 		INIT_LIST_HEAD(&mnt->mnt_slave);
+		INIT_LIST_HEAD(&mnt->mnt_mp_list);
 #ifdef CONFIG_FSNOTIFY
 		INIT_HLIST_HEAD(&mnt->mnt_fsnotify_marks);
 #endif

@@ -636,6 +637,7 @@ static struct mountpoint *new_mountpoint(struct dentry *dentry)
 	mp->m_dentry = dentry;
 	mp->m_count = 1;
 	list_add(&mp->m_hash, chain);
+	INIT_LIST_HEAD(&mp->m_list);
 	return mp;
 }
 

@@ -643,6 +645,7 @@ static void put_mountpoint(struct mountpoint *mp)
 {
 	if (!--mp->m_count) {
 		struct dentry *dentry = mp->m_dentry;
+		BUG_ON(!list_empty(&mp->m_list));
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags &= ~DCACHE_MOUNTED;
 		spin_unlock(&dentry->d_lock);

@@ -689,6 +692,7 @@ static void detach_mnt(struct mount *mnt, struct path *old_path)
 	mnt->mnt_mountpoint = mnt->mnt.mnt_root;
 	list_del_init(&mnt->mnt_child);
 	list_del_init(&mnt->mnt_hash);
+	list_del_init(&mnt->mnt_mp_list);
 	put_mountpoint(mnt->mnt_mp);
 	mnt->mnt_mp = NULL;
 }

@@ -705,6 +709,7 @@ void mnt_set_mountpoint(struct mount *mnt,
 	child_mnt->mnt_mountpoint = dget(mp->m_dentry);
 	child_mnt->mnt_parent = mnt;
 	child_mnt->mnt_mp = mp;
+	list_add_tail(&child_mnt->mnt_mp_list, &mp->m_list);
 }
 
 /*

@@ -1191,6 +1196,7 @@ void umount_tree(struct mount *mnt, int propagate)
 		list_del_init(&p->mnt_child);
 		if (mnt_has_parent(p)) {
 			p->mnt_parent->mnt_ghosts++;
+			list_del_init(&p->mnt_mp_list);
 			put_mountpoint(p->mnt_mp);
 			p->mnt_mp = NULL;
 		}

Signature · 1 line

1.7.5.4

v2: Always drop the lock when exiting early.
v3: Make detach_mounts robust about freeing several
mounts on the same mountpoint at one time, and remove the unneeded mnt_list list test.

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/mount.h     |    1 +
 fs/namespace.c |   24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

fs/mount.h +1 -0

index e4342b8dfab1..7a6a2bb3f290 100644
@@ -79,6 +79,7 @@ static inline int is_mounted(struct vfsmount *mnt)
 }
 
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *, int);
+extern void detach_mounts(struct dentry *dentry);
 
 static inline void get_mnt_ns(struct mnt_namespace *ns)
 {

fs/namespace.c +24 -0

index e4fe22c23e00..78f7c5c9e673 100644
@@ -1295,6 +1295,30 @@ static int do_umount(struct mount *mnt, int flags)
 	return retval;
 }
 
+void detach_mounts(struct dentry *dentry)
+{
+	struct mountpoint *mp;
+	struct mount *mnt;
+
+	namespace_lock();
+	if (!d_mountpoint(dentry))
+		goto out_unlock;
+
+	mp = new_mountpoint(dentry);
+	if (IS_ERR(mp))
+		goto out_unlock;
+
+	br_write_lock(&vfsmount_lock);
+	while (!list_empty(&mp->m_list)) {
+		mnt = list_first_entry(&mp->m_list, struct mount, mnt_mp_list);
+		umount_tree(mnt, 1);
+	}
+	br_write_unlock(&vfsmount_lock);
+	put_mountpoint(mp);
+out_unlock:
+	namespace_unlock();
+}
+
 /* 
  * Is the caller allowed to modify his namespace?
  */

Signature · 1 line

1.7.5.4

With the introduction of mount namespaces and bind mounts it became possible to access files and directories that on some paths are mount points but are not mount points on other paths. It is very confusing when rm -rf somedir returns -EBUSY simply because somedir is mounted somewhere else. With the addition of user namespaces allowing unprivileged mounts this condition has gone from annoying to allowing a DOS attack on other users in the system.

The possibility for mischief is removed by updating the vfs to support rename, unlink and rmdir on a dentry that is a mountpoint and by lazily unmounting mountpoints on deleted dentries.

In particular this change allows rename, unlink and rmdir system calls on a dentry without a mountpoint in the current mount namespace to succeed, and it allows rename, unlink, and rmdir performed on a distributed filesystem to update the vfs cache even if when there is a mount in some namespace on the original dentry.

There are two common patterns of maintaining mounts: Mounts on trusted paths with the parent directory of the mount point and all ancestory directories up to / owned by root and modifiable only by root (i.e. /media/xxx, /dev, /dev/pts, /proc, /sys, /sys/fs/cgroup/{cpu, cpuacct, ...}, /usr, /usr/local). Mounts on unprivileged directories maintained by fusermount.

In the case of mounts in trusted directories owned by root and modifiable only by root the current parent directory permissions are sufficient to ensure a mount point on a trusted path is not removed or renamed by anyone other than root, even if there is a context where the there are no mount points to prevent this.

In the case of mounts in directories owned by less privileged users races with users modifying the path of a mount point are already a danger. fusermount already uses a combination of chdir, /proc/<pid>/fd/NNN, and UMOUNT_NOFOLLOW to prevent these races. The removable of global rename, unlink, and rmdir protection really adds nothing new to consider only a widening of the attack window, and fusermount is already safe against unprivileged users modifying the directory simultaneously.

In principle for perfect userspace programs returning -EBUSY for unlink, rmdir, and rename of dentires that have mounts in the local namespace is actually unnecessary. Unfortunately not all userspace programs are perfect so retaining -EBUSY for unlink, rmdir and rename of dentries that have mounts in the current mount namespace plays an important role of maintaining consistency with historical behavior and making imperfect userspace applications hard to exploit.

v2: Remove spurious old_dentry.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/afs/dir.c           |    3 +-
 fs/dcache.c            |   80 ++++++++++++++++++++----------------------------
 fs/fuse/dir.c          |    3 +-
 fs/gfs2/dentry.c       |    4 +--
 fs/namei.c             |   30 ++++++------------
 fs/nfs/dir.c           |    5 +--
 fs/sysfs/dir.c         |    9 +-----
 include/linux/dcache.h |    3 +-
 8 files changed, 50 insertions(+), 87 deletions(-)

fs/afs/dir.c +1 -2

index 646337dc5201..7fb69d45f1b9 100644
@@ -686,8 +686,7 @@ not_found:
 
 out_bad:
 	/* don't unhash if we have submounts */
-	if (check_submounts_and_drop(dentry) != 0)
-		goto out_skip;
+	shrink_submounts_and_drop(dentry);
 
 	_debug("dropping dentry %s/%s",
 	       parent->d_name.name, dentry->d_name.name);

fs/dcache.c +33 -47

index 41000305d716..1e9bf96b0132 100644
@@ -1373,7 +1373,7 @@ int d_set_mounted(struct dentry *dentry)
 	int ret = -ENOENT;
 	write_seqlock(&rename_lock);
 	for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
-		/* Need exclusion wrt. check_submounts_and_drop() */
+		/* Need exclusion wrt. shrink_submounts_and_drop() */
 		spin_lock(&p->d_lock);
 		if (unlikely(d_unhashed(p))) {
 			spin_unlock(&p->d_lock);

@@ -1478,70 +1478,56 @@ void shrink_dcache_parent(struct dentry *parent)
 }
 EXPORT_SYMBOL(shrink_dcache_parent);
 
-static enum d_walk_ret check_and_collect(void *_data, struct dentry *dentry)
+struct detach_data {
+	struct dentry *found;
+};
+static enum d_walk_ret do_detach_submounts(void *ptr, struct dentry *dentry)
 {
-	struct select_data *data = _data;
-
-	if (d_mountpoint(dentry)) {
-		data->found = -EBUSY;
-		return D_WALK_QUIT;
-	}
-
-	return select_collect(_data, dentry);
-}
+	struct detach_data *data = ptr;
 
-static void check_and_drop(void *_data)
-{
-	struct select_data *data = _data;
+	if (d_mountpoint(dentry))
+		data->found = dentry;
 
-	if (d_mountpoint(data->start))
-		data->found = -EBUSY;
-	if (!data->found)
-		__d_drop(data->start);
+	return data->found ? D_WALK_QUIT : D_WALK_CONTINUE;
 }
 
 /**
- * check_submounts_and_drop - prune dcache, check for submounts and drop
+ * detach_submounts - check for submounts and detach them.
  *
- * All done as a single atomic operation relative to has_unlinked_ancestor().
- * Returns 0 if successfully unhashed @parent.  If there were submounts then
- * return -EBUSY.
+ * @dentry: dentry to find mount points under.
  *
- * @dentry: dentry to prune and drop
+ * If dentry or any of it's children is a mount point detach those mounts.
  */
-int check_submounts_and_drop(struct dentry *dentry)
+void detach_submounts(struct dentry *dentry)
 {
-	int ret = 0;
-
-	/* Negative dentries can be dropped without further checks */
-	if (!dentry->d_inode) {
-		d_drop(dentry);
-		goto out;
-	}
-
+	struct detach_data data;
 	for (;;) {
-		struct select_data data;
-
-		INIT_LIST_HEAD(&data.dispose);
-		data.start = dentry;
-		data.found = 0;
+		data.found = NULL;
+		d_walk(dentry, &data, do_detach_submounts, NULL);
 
-		d_walk(dentry, &data, check_and_collect, check_and_drop);
-		ret = data.found;
-
-		if (!list_empty(&data.dispose))
-			shrink_dentry_list(&data.dispose);
-
-		if (ret <= 0)
+		if (!data.found)
 			break;
 
+		detach_mounts(data.found);
 		cond_resched();
 	}
+	detach_mounts(dentry);
+}
 
-out:
-	return ret;
+/**
+ * shrink_submounts_and_drop - detach submounts, prune dcache, and drop
+ *
+ * All done as a single atomic operation reletaive to d_set_mounted().
+ *
+ * @dentry: dentry to detach, prune and drop
+ */
+void shrink_submounts_and_drop(struct dentry *dentry)
+{
+	d_drop(dentry);
+	detach_submounts(dentry);
+	shrink_dcache_parent(dentry);
 }
-EXPORT_SYMBOL(check_submounts_and_drop);
+EXPORT_SYMBOL(shrink_submounts_and_drop);
 
 /**
  * __d_alloc	-	allocate a dcache entry

fs/fuse/dir.c +1 -2

index 62b43b577bfc..b1cd7b79a325 100644
@@ -259,8 +259,7 @@ out:
 
 invalid:
 	ret = 0;
-	if (check_submounts_and_drop(entry) != 0)
-		ret = 1;
+	shrink_submounts_and_drop(entry);
 	goto out;
 }
 

fs/gfs2/dentry.c +1 -3

index d3a5d4e29ba5..2ecc2b873829 100644
@@ -93,9 +93,7 @@ invalid_gunlock:
 	if (!had_lock)
 		gfs2_glock_dq_uninit(&d_gh);
 invalid:
-	if (check_submounts_and_drop(dentry) != 0)
-		goto valid;
-
+	shrink_submounts_and_drop(dentry);
 	dput(parent);
 	return 0;
 

fs/namei.c +10 -20

index df7bd6e9c7b6..a12c1d31d4c8 100644
@@ -3574,10 +3574,6 @@ int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 	dget(dentry);
 	mutex_lock(&dentry->d_inode->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(dentry))
-		goto out;
-
 	error = security_inode_rmdir(dir, dentry);
 	if (error)
 		goto out;

@@ -3589,6 +3585,7 @@ int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 
 	dentry->d_inode->i_flags |= S_DEAD;
 	dont_mount(dentry);
+	detach_mounts(dentry);
 
 out:
 	mutex_unlock(&dentry->d_inode->i_mutex);

@@ -3674,14 +3671,12 @@ int vfs_unlink(struct inode *dir, struct dentry *dentry)
 		return -EPERM;
 
 	mutex_lock(&dentry->d_inode->i_mutex);
-	if (d_mountpoint(dentry))
-		error = -EBUSY;
-	else {
-		error = security_inode_unlink(dir, dentry);
+	error = security_inode_unlink(dir, dentry);
+	if (!error) {
+		error = dir->i_op->unlink(dir, dentry);
 		if (!error) {
-			error = dir->i_op->unlink(dir, dentry);
-			if (!error)
-				dont_mount(dentry);
+			dont_mount(dentry);
+			detach_mounts(dentry);
 		}
 	}
 	mutex_unlock(&dentry->d_inode->i_mutex);

@@ -4008,10 +4003,6 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
 	if (target)
 		mutex_lock(&target->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(old_dentry) || d_mountpoint(new_dentry))
-		goto out;
-
 	error = -EMLINK;
 	if (max_links && !target && new_dir != old_dir &&
 	    new_dir->i_nlink >= max_links)

@@ -4026,6 +4017,7 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
 	if (target) {
 		target->i_flags |= S_DEAD;
 		dont_mount(new_dentry);
+		detach_mounts(new_dentry);
 	}
 out:
 	if (target)

@@ -4051,16 +4043,14 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 	if (target)
 		mutex_lock(&target->i_mutex);
 
-	error = -EBUSY;
-	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
-		goto out;
-
 	error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
 	if (error)
 		goto out;
 
-	if (target)
+	if (target) {
 		dont_mount(new_dentry);
+		detach_mounts(new_dentry);
+	}
 	if (!(old_dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE))
 		d_move(old_dentry, new_dentry);
 out:

fs/nfs/dir.c +1 -4

index 854a8f05a610..e8e35acd8850 100644
@@ -1142,10 +1142,7 @@ out_zap_parent:
 		if (dentry->d_flags & DCACHE_DISCONNECTED)
 			goto out_valid;
 	}
-	/* If we have submounts, don't unhash ! */
-	if (check_submounts_and_drop(dentry) != 0)
-		goto out_valid;
-
+	shrink_submounts_and_drop(dentry);
 	dput(parent);
 	dfprintk(LOOKUPCACHE, "NFS: %s(%s/%s) is invalid\n",
 			__func__, dentry->d_parent->d_name.name,

fs/sysfs/dir.c +1 -8

index 4d83cedb9fcb..477c66d4e2a8 100644
@@ -327,7 +327,6 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, unsigned int flags)
 	}
 
 	mutex_unlock(&sysfs_mutex);
-out_valid:
 	return 1;
 out_bad:
 	/* Remove the dentry from the dcache hashes.

@@ -341,13 +340,7 @@ out_bad:
 	 * to the dcache hashes.
 	 */
 	mutex_unlock(&sysfs_mutex);
-
-	/* If we have submounts we must allow the vfs caches
-	 * to lie about the state of the filesystem to prevent
-	 * leaks and other nasty things.
-	 */
-	if (check_submounts_and_drop(dentry) != 0)
-		goto out_valid;
+	shrink_submounts_and_drop(dentry);
 
 	return 0;
 }

include/linux/dcache.h +2 -1

index 59066e0b4ff1..17948b49f3d5 100644
@@ -254,7 +254,8 @@ extern void d_prune_aliases(struct inode *);
 
 /* test whether we have any submounts in a subdir tree */
 extern int have_submounts(struct dentry *);
-extern int check_submounts_and_drop(struct dentry *);
+extern void detach_submounts(struct dentry *dentry);
+extern void shrink_submounts_and_drop(struct dentry *);
 
 /*
  * This adds the entry to the hash queues.

Signature · 1 line

1.7.5.4

"Serge E. Hallyn" <[email protected]> writes:

So if we can feel safe just depending on the parent directory permissions (which are not hidden by a mount) protecting our mount points, I feel much better about this patchset.

Quoted 5 lines · Eric W. Biederman

"Serge E. Hallyn" <[email protected]> writes:

So if we can feel safe just depending on the parent directory permissions (which are not hidden by a mount) protecting our mount points, I feel much better about this patchset.

As far as i can tell, the permissions of the host-directory of a mount-point are hidden, at least for user-space. (Ignoring (bind-)mounting the parent-mount somewhere else)

As root:

$ mkdir /tmp/test
$ ls -ld /tmp/test
drwxr-xr-x 2 root root 40 Oct  8 12:33 /tmp/test

$ mount tmpfs -t tmpfs /tmp/test
$ ls -ld /tmp/test
drwxrwxrwt 2 root root 40 Oct  8 12:33 /tmp/test

$ chown nobody.users /tmp/test
$ ls -ld /tmp/test
drwxrwxrwt 2 nobody users 40 Oct  8 12:33 /tmp/test

$ umount /tmp/test
$ ls -ld /tmp/test
drwxr-xr-x 2 root root 40 Oct  8 12:33 /tmp/test

So if the kernel would check the host-directory-permissions for allowing umounting by rmdir it follows that a "plain user" doesn't have any possibility to know beforehand if rmdir/umount would be possible.

This patchset is an attempt to address two problems:
1) Not all modifications to the filesystems happen through the vfs and since the vfs can not cope with a mount point being unlinked or renamed filesystems whose modifications that do not come through
the
vfs are required to lie.

2) Through an oversight it is now possible for one unprivileged user to mount something on another unprivileged users dentry and make it impossible for the other user to unlink or rename that dentry.

It is now technically possible to easily lift the restriction on unlinking and renaming files with mount points on them, with a corresponding reduction in complexity of the vfs semantics and a small code side reduction.

A todo item I've had _forever_ is fixing chroot() to not be broken so that you can trivially break out of a chroot via:

  chdir("/");
  mkdir("sub");
  chroot("sub");
  chdir("./../../../../../../../..");

So learn this pattern: every time you use chroot, add a simple

  chdir("/");

immediately after the chroot call.

Quoted 6 lines · Linus Torvalds

So learn this pattern: every time you use chroot, add a simple

  chdir("/");

immediately after the chroot call.

.. btw, also make sure that you close all non-essential file descriptors. Having any open directory file descriptors pointing to outside the chroot is also a classic escape.

Even then, escaping chroot is usually fairly easy. Making a escape-proof chroot is really quite hard. Basically impossible if you allow root.

Which is why containers have all sorts of extra plumbing. But that extra plumbing is currently built on pivot_root(), not on chroot(). And I'd dismissed pivot_root() as residue from initramfs with all that kernel thread reparenting, so it seemed like the wrong tool for the job, but obviously I'll take your and Al's word it's not...

Quoted 3 lines · Rob Landley

A todo item I've had _forever_ is fixing chroot() to not be broken so that you can trivially break out of a chroot via:

What drugs are you on?

Your example is moronic, and against all _documented_ uses of chroot.

Quoted 3 lines · Linus Torvalds

Quoted 2 lines · Rob Landley

A todo item I've had _forever_ is fixing chroot() to not be

broken so that

Quoted 2 lines

Quoted 1 line

you can trivially break out of a chroot via:

Quoted 3 lines

What drugs are you on?

Your example is moronic, and against all _documented_ uses of

chroot.

... and even fixing that example (chroot *can* be escaped, if you can call chroot(2) - mkdir /foo, chdir /, chroot /foo, chroot .. and you are out)

the whole thing is idiocy - chroot() is not and has never been root-proof and anybody expecting it to be has failed to read any number of FAQs out there.

Quoted 2 lines · Rob Landley

A todo item I've had _forever_ is fixing chroot() to not be broken

so that

Quoted 1 line

you can trivially break out of a chroot via:

What drugs are you on?

Your example is moronic, and against all _documented_ uses of chroot.

Quoted 4 lines

  chdir("/");
  mkdir("sub");
  chroot("sub");
  chdir("./../../../../../../../..");

After you do a chroot(), you need to chdir *into* the root. The reason chroot() itself doesn't do that is simple: you may still be doing various setup stuff.

But your example is just stupid. Yes, chroot'ed environments can generally be escaped, but your example escape is simply because you didn't use chroot() correctly.

So learn this pattern: every time you use chroot, add a simple

  chdir("/");

immediately after the chroot call.

A todo item I've had _forever_ is fixing chroot() to not be broken so that you can trivially break out of a chroot via:

  chdir("/");
  mkdir("sub");
  chroot("sub");
  chdir("./../../../../../../../..");

(Because chroot() affects where "/" points but NOT where "." points to, and chdir does an == check with the dentry "/" points at to know when to stop, so if you move "/" under "." you can back up to the actual root of the tree.)

The above is why lxc uses pivot_root() instead of chroot().

These days, we have multiple mount trees so there's no reason chroot() can't trim the process local mount tree (creating a new bind mount if necessary). Except my todo list runneth over and I haven't had a chance to dig in and see what would be involved. (Last time I brought this up people were wondering why chroot() didn't just move "." to the new "/" if it wasn't under it. I had no idea, still don't.)

Quoted 15 lines · Rob Landley

A todo item I've had _forever_ is fixing chroot() to not be broken so that you can trivially break out of a chroot via:

  chdir("/");
  mkdir("sub");
  chroot("sub");
  chdir("./../../../../../../../..");

(Because chroot() affects where "/" points but NOT where "." points to, and chdir does an == check with the dentry "/" points at to know when to stop, so if you move "/" under "." you can back up to the actual root of the tree.)

The above is why lxc uses pivot_root() instead of chroot().

Quoted 7 lines

These days, we have multiple mount trees so there's no reason chroot() can't trim the process local mount tree (creating a new bind mount if necessary). Except my todo list runneth over and I haven't had a chance to dig in and see what would be involved. (Last time I brought this up people were wondering why chroot() didn't just move "." to the new "/" if it wasn't under it. I had no idea, still don't.)

1) RTFUNIXFAQ. chroot() never has been root-proof.

2) your "fix" isn't - it will lead to mounts done by chrooted process not affecting other processes in the same namespace.