Call for testing and review of mount detach fixes

Cache the value of __d_entry_type in d_is_dir and test if it equal to DCACHE_DIRECTORY_TYPE or DCACHE_AUTODIR_TYPE.

The generated assembly goes from:

	movl	(%rdi), %eax  # MEM[(volatile __u32 *)dentry_3(D)], tmp73
	andl	$7340032, %eax  #, tmp73
	cmpl	$2097152, %eax  #, tmp73
	je	.L1091	#,
	movl	(%rdi), %eax	# MEM[(volatile __u32 *)dentry_3(D)], tmp74
	andl	$7340032, %eax	#, tmp74
	cmpl	$3145728, %eax	#, tmp74
	je	.L1091	#,
to:
	movl	(%rdi), %eax	# MEM[(volatile __u32 *)dentry_3(D)], tmp71
	andl	$6291456, %eax	#, tmp71
	cmpl	$2097152, %eax	#, tmp71
	jne	.L1091	  #,

Which with only one read of d_flags, one comparison and one jump is dramatically better code.

As __d_entry_type is not written to allow the compiler to optimize away anything that it does, when it is possible and reasonable to optimize things away the optimization needs to be performend manually.

---
 include/linux/dcache.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 5b69856b45a2..82eb50aaf446 100644
@@ -443,7 +443,8 @@ static inline bool d_is_autodir(const struct dentry *dentry)
 
 static inline bool d_is_dir(const struct dentry *dentry)
 {
-	return d_can_lookup(dentry) || d_is_autodir(dentry);
+	unsigned type = __d_entry_type(dentry);
+	return (type == DCACHE_DIRECTORY_TYPE) || (type == DCACHE_AUTODIR_TYPE);
 }
 
 static inline bool d_is_symlink(const struct dentry *dentry)

2.2.1

Quite frankly, I hate this series.

Not all of it. Patches 1,2 and 5 look fine to me. But 3,4,6 and 7 I'm not happy with. Particularly patch 6.

It just smells like a bad hack to me. My gut feel says that it's all wrong. It doesn't feel clean or right.

I'd much rather make ".." handling more expensive than add and maintain that MNT_DIR_ESCAPED flag. My gut feel is that yes, we should look seriously at making ".." much smarter (so I don't object to the concept of patch 7/7 at all), but I think we should strive to look at that ".." handling *without* adding the crufty odd special bind mount crud.

Put another way: the whole "escape bind mount" thing is not at all a new issue, it smells very much like the very traditional Unix "escape chroot" thing. And I detest how this adds magic rules for bind mounts, when it feels like a much more generic issue.

Ok, so I haven't really thought deeply about this, this literally is just a "gut feel" kind of thing.

Can we really not validate ".." some clever way _without_ adding all those "mount escape" flags? And by "clever" I potentially mean "not clever" and in fact just fairly brute force. I'd almost prefer to just walk the parent chains all the way to the root and validate the ".." that way..

Linus

A rename can result in a dentry that by walking up d_parent will never reach it's mnt_root. For lack of a better term I call this an escaped path.

prepend_path is called by four different functions __d_path, d_absolute_path, d_path, and getcwd.

__d_path only wants to see paths are connected to the root it passes in. So __d_path needs prepend_path to return an error.

d_absolute_path similarly wants to see paths that are connected to some root. Escaped paths are not connected to any mnt_root so d_absolute_path needs prepend_path to return an error greater than 1. So escaped paths will be treated like paths on lazily unmounted mounts.

getcwd needs to prepend "(unreachable)" so getcwd also needs prepend_path to return an error.

d_path is the interesting hold out. d_path just wants to print something, and does not care about the weird cases. Which raises the question what should be printed?

Given that <escaped_path>/<anything> should result in -ENOENT I believe it is desirable for escaped paths to be printed as empty paths. As there are not really any meaninful path components when considered from the perspective of a mount tree.

So tweak prepend_path to return an empty path with an new error code of 3 when it encounters an escaped path.

---
 fs/dcache.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 7a3f3e5f9cea..f762e76e85cc 100644
@@ -2923,6 +2923,13 @@ restart:
 
 		if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
 			struct mount *parent = ACCESS_ONCE(mnt->mnt_parent);
+			/* Escaped? */
+			if (dentry != vfsmnt->mnt_root) {
+				bptr = *buffer;
+				blen = *buflen;
+				error = 3;
+				break;
+			}
 			/* Global root? */
 			if (mnt != parent) {
 				dentry = ACCESS_ONCE(mnt->mnt_mountpoint);

2.2.1

i_lock is only needed until __d_find_any_alias calls dget on the alias dentry. After that the reference to new ensures that dentry_kill and d_delete will not remove the inode from the dentry, and remove the dentry from the inode->d_entry list.

The inode i_lock came to be held over the the __d_move calls in d_splice_alias through a series of introduction of locks with increasing smaller scope. First it was the dcache_lock, then it was the dcache_inode_lock, and finally inode->i_lock.

Furthermore inode->i_lock is not held over any other calls to d_move or __d_move so it can not provide any meaningful rename protection.

---
 fs/dcache.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index f762e76e85cc..53b7f1e63beb 100644
@@ -2715,7 +2715,7 @@ struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
  * This helper attempts to cope with remotely renamed directories
  *
  * It assumes that the caller is already holding
- * dentry->d_parent->d_inode->i_mutex, inode->i_lock and rename_lock
+ * dentry->d_parent->d_inode->i_mutex, and rename_lock
  *
  * Note: If ever the locking in lock_rename() changes, then please
  * remember to update this too...

@@ -2741,7 +2741,6 @@ out_unalias:
 	__d_move(alias, dentry, false);
 	ret = 0;
 out_err:
-	spin_unlock(&inode->i_lock);
 	if (m2)
 		mutex_unlock(m2);
 	if (m1)

@@ -2787,10 +2786,11 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 	if (S_ISDIR(inode->i_mode)) {
 		struct dentry *new = __d_find_any_alias(inode);
 		if (unlikely(new)) {
+			/* The reference to new ensures it remains an alias */
+			spin_unlock(&inode->i_lock);
 			write_seqlock(&rename_lock);
 			if (unlikely(d_ancestor(new, dentry))) {
 				write_sequnlock(&rename_lock);
-				spin_unlock(&inode->i_lock);
 				dput(new);
 				new = ERR_PTR(-ELOOP);
 				pr_warn_ratelimited(

@@ -2809,7 +2809,6 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 			} else {
 				__d_move(new, dentry, false);
 				write_sequnlock(&rename_lock);
-				spin_unlock(&inode->i_lock);
 				security_d_instantiate(new, inode);
 			}
 			iput(inode);

2.2.1

If possible find the common ancestor of two dentries.

This is necessary infrastructure for better handling the case when a dentry is moved out from under the root of a bind mount.

---
 fs/dcache.c            | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/dcache.h |  1 +
 2 files changed, 38 insertions(+)

index 53b7f1e63beb..4e66bf92a481 100644
@@ -2469,6 +2469,43 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 }
 EXPORT_SYMBOL(dentry_update_name_case);
 
+static unsigned long d_depth(const struct dentry *dentry)
+{
+	unsigned long depth = 0;
+
+	while (!IS_ROOT(dentry)) {
+		dentry = dentry->d_parent;
+		depth++;
+	}
+	return depth;
+}
+
+const struct dentry *d_common_ancestor(const struct dentry *left,
+				       const struct dentry *right)
+{
+	unsigned long ldepth = d_depth(left);
+	unsigned long rdepth = d_depth(right);
+
+	while (ldepth > rdepth) {
+		left = left->d_parent;
+		ldepth--;
+	}
+
+	while (rdepth > ldepth) {
+		right = right->d_parent;
+		rdepth--;
+	}
+
+	while (left != right) {
+		if (IS_ROOT(left))
+			return NULL;
+		left = left->d_parent;
+		right = right->d_parent;
+	}
+
+	return left;
+}
+
 static void swap_names(struct dentry *dentry, struct dentry *target)
 {
 	if (unlikely(dname_external(target))) {

index 06bed2a1053c..5b69856b45a2 100644
@@ -313,6 +313,7 @@ extern void dentry_update_name_case(struct dentry *, struct qstr *);
 extern void d_move(struct dentry *, struct dentry *);
 extern void d_exchange(struct dentry *, struct dentry *);
 extern struct dentry *d_ancestor(struct dentry *, struct dentry *);
+extern const struct dentry *d_common_ancestor(const struct dentry *, const struct dentry *);
 
 /* appendix may either be NULL or be used for transname suffixes */
 extern struct dentry *d_lookup(const struct dentry *, const struct qstr *);

2.2.1

Quite frankly, I hate this series.

Not all of it. Patches 1,2 and 5 look fine to me. But 3,4,6 and 7 I'm not happy with. Particularly patch 6.

It just smells like a bad hack to me. My gut feel says that it's all wrong. It doesn't feel clean or right.

I'd much rather make ".." handling more expensive than add and maintain that MNT_DIR_ESCAPED flag. My gut feel is that yes, we should look seriously at making ".." much smarter (so I don't object to the concept of patch 7/7 at all), but I think we should strive to look at that ".." handling *without* adding the crufty odd special bind mount crud.

Put another way: the whole "escape bind mount" thing is not at all a new issue, it smells very much like the very traditional Unix "escape chroot" thing. And I detest how this adds magic rules for bind mounts, when it feels like a much more generic issue.

Ok, so I haven't really thought deeply about this, this literally is just a "gut feel" kind of thing.

Can we really not validate ".." some clever way _without_ adding all those "mount escape" flags? And by "clever" I potentially mean "not clever" and in fact just fairly brute force. I'd almost prefer to just walk the parent chains all the way to the root and validate the ".." that way..

Linus

For example: while it's true that walking a logn chain of parents (to validate that we hit root etc) would be expensive, I don't think we'd necessarily need to do it for the common case.

For example, if out current "mnt->mnt_root" is a _real_ root (so IS_ROOT() is true), then we know we're not in some possibly partial bind mount, so we don't need to check anything else, and we can happily move to the parent dentry *without* having to be particularly careful.

Otherwise we might need to walk the dentry parent chain to check that yes, we will hit that mnt->mnt_root" entry, and that we're not possibly escaping the bind mount. But even that walk is "just" following a chain of pointers. It's not *that* expensive.

I'd much rather make ".." more expensive, if it means that we don't have to track the status of whether a mount has a potentially escaped directory in it or not. Because I think we can avoid the costs for traditional non-bind mounts.

No?

Linus

Yes we can compare s_root and mnt_root and only call is_subir if they don't match.

At this point it is a matter of trade offs.

If there is not an escape I do not expect my current implementation will have a measurable cost. And I don't expect there will be any escapes.

That said if you and Al would be happy with what you are proposing I can easily implement it.

My only concern at this point is that I know some containers run with a bind mount for their root directory so it might be a change with a measurable cost. At the same time shallow directory paths are the norm so I don't expect there to be much of a cost.

Eric

Not even "is_subdir()" - for the RCU traversal case, just d_ancestor() should be sufficient since we'd already be in an RCU read-locked region and the RCU lookup checks the rename sequence number around it all.

And d_ancestor() should really be pretty low-cost - even *if* we have to call it, which wouldn't even be the case for the normal situation.

So the cost I worry about is not the CPU cost, but the complexity and correctness. If anything goes subtly wrong, the end result is going to be some very very subtle bugs.

And personally, I'd be much happier with something that is a bit more straightforward, even if it makes ".." lookup slower. Especially since I think we can limit the costs to fairly obvious cases (ie only for partial bind mounts). Keep the code more straightforward, and *if* we ever see the cost of dentry traversal

But it's up to Al, I think.

Al, comments?

Linus

We check the dentry sequence number and the mount sequence number, which may be enough to catch a local rename but is certainly not enough to catch what d_ancestor cares about.

Further we have the partial rcu to non-rcu walk case represented by unlazy_walk that means we can't blithely do something that might be wrong and only check the sequence numbers at each step.

Fair enough. I like simple low complexity code, but I don't want to mess up the pathname lookup fastpath.

At the very beginning of this I got shot down by Al Viro for a simple implementation that essentially had everything except the check for being a bind mount. Knowing what I know now I realize it was a bit buggy, calling d_ancestor in the rcu walk instead of d_subdir, but it was shot down for the cpu cost. Then Al suggested the basic approach I have taken in these patches.

As soon as I am done testing I am going to post the revised version of my final patch that only performs is_subdir checks on bind mounts.

Then we can decide to merge whichever version of the code you and Al are happy with.

Eric

In rare cases a directory can be renamed out from under a bind mount. In those cases without special handling it becomes possible to walk up the directory tree to the root dentry of the filesystem and down from the root dentry to every other file or directory on the filesystem.

Like division by zero .. from an unconnected path can not be given a useful semantic as there is no predicting at which path component the code will realize it is unconnected. We certainly can not match the current behavior as the current behavior is a security hole.

Therefore when encounting .. when following an unconnected path return -ENOENT.

- Add a function path_connected to verify path->dentry is reachable from path->mnt.mnt_root. AKA to validate that rename did not do something nasty to the bind mount.

To avoid races path_connected must be called after following a path component to it's next path component.

---

This is the simple version that needs no extra vfs support.

My availability is likely to be a bit spotty for the next while as I am travelling to and then attending Linux Plumbers Conference.

 fs/namei.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

index ae4e4c18b2ac..5303e994f8d6 100644
@@ -560,6 +560,24 @@ static int __nd_alloc_stack(struct nameidata *nd)
 	return 0;
 }
 
+/**
+ * path_connected - Verify that a path->dentry is below path->mnt.mnt_root
+ * @path: nameidate to verify
+ *
+ * Rename can sometimes move a file or directory outside of a bind
+ * mount, path_connected allows those cases to be detected.
+ */
+static bool path_connected(const struct path *path)
+{
+	struct vfsmount *mnt = path->mnt;
+
+	/* Only bind mounts can have disconnected paths */
+	if (mnt->mnt_root == mnt->mnt_sb->s_root)
+		return true;
+
+	return is_subdir(path->dentry, mnt->mnt_root);
+}
+
 static inline int nd_alloc_stack(struct nameidata *nd)
 {
 	if (likely(nd->depth != EMBEDDED_LEVELS))

@@ -1296,6 +1314,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 				return -ECHILD;
 			nd->path.dentry = parent;
 			nd->seq = seq;
+			if (unlikely(!path_connected(&nd->path)))
+				return -ENOENT;
 			break;
 		} else {
 			struct mount *mnt = real_mount(nd->path.mnt);

@@ -1396,7 +1416,7 @@ static void follow_mount(struct path *path)
 	}
 }
 
-static void follow_dotdot(struct nameidata *nd)
+static int follow_dotdot(struct nameidata *nd)
 {
 	if (!nd->root.mnt)
 		set_root(nd);

@@ -1412,6 +1432,8 @@ static void follow_dotdot(struct nameidata *nd)
 			/* rare case of legitimate dget_parent()... */
 			nd->path.dentry = dget_parent(nd->path.dentry);
 			dput(old);
+			if (unlikely(!path_connected(&nd->path)))
+				return -ENOENT;
 			break;
 		}
 		if (!follow_up(&nd->path))

@@ -1419,6 +1441,7 @@ static void follow_dotdot(struct nameidata *nd)
 	}
 	follow_mount(&nd->path);
 	nd->inode = nd->path.dentry->d_inode;
+	return 0;
 }
 
 /*

@@ -1634,7 +1657,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
 		if (nd->flags & LOOKUP_RCU) {
 			return follow_dotdot_rcu(nd);
 		} else
-			follow_dotdot(nd);
+			return follow_dotdot(nd);
 	}
 	return 0;
 }

2.2.1

I think you are underestimating the frequency of .. traversals. Any build process that creates relative symlinks will be hitting it all the time, for one thing. And less-than-entire-fs mounts are not something pathological - I've got quite a few here, things like container setups often create such beasts, etc. Not to mention that things like NFSv4 will often look like such partial mounts, BTW.

I really don't understand why the hell do we need *anything* complicated around __d_move() callers - just take sodding spinlock of mount_lock in the (few) callers around rename_lock, and that's it. No need for anything subtle and brittle.

Redoing the locking in d_splice_alias() simply doesn't belong anywhere near that work.

Basically, all places where we change tree topology go through __d_move() (and take rename_lock around it). There are very few of those. And we can find and taint affected mounts quite easily - I think we all agree that beginning of that series looks sane (locating mounts by mnt_root), right?

Let's just add "mount_lock should be held read-exclusive by all callers of __d_move()" and do the find-and-taint logics from dentry_lock_for_move(). Move these

        BUG_ON(d_ancestor(dentry, target));
        BUG_ON(d_ancestor(target, dentry));

into dentry_lock_for_move(), while we are at it. And have it begin with finding the last common ancestor of dentry and target. Which turns those checks into ancestor == dentry and ancestor == target... Then we need to taint everything with root being an ancestor of dentry, but not an ancestor of target. Which is trivial with LCA already found. For exchange case we need to do that both for dentry and target, of course. That's it. After that we just use that taint instead of "is it a partial?" in .. handling.

I suspect you're over-estimating how expensive it is to just walk down to the mount-point. It's just a few pointer traversals.

Realistically, we probably do more than that for a *regular* path component lookup, when we follow the hash chains. Following a d_parent chain for ".." isn't that different.

Just looking at the last patch Eric sent, that one looks _trivial_. It didn't need *any* preparation or new rules. Compared to the mess with marking things MNT_DIR_ESCAPED etc, I know which approach I'd prefer.

But hey, if you think you can simplify it... I just don't think that even totally ignoring the d_splice_alias() things, and totally ignoring any locking around __d_move(), the whole "mark things MNT_DIR_ESCAPED" is a lot more complex.

Linus

I suspect you're over-estimating how expensive it is to just walk down to the mount-point. It's just a few pointer traversals.

Realistically, we probably do more than that for a *regular* path component lookup, when we follow the hash chains. Following a d_parent chain for ".." isn't that different.

Just looking at the last patch Eric sent, that one looks _trivial_. It didn't need *any* preparation or new rules. Compared to the mess with marking things MNT_DIR_ESCAPED etc, I know which approach I'd prefer.

But hey, if you think you can simplify it... I just don't think that even totally ignoring the d_splice_alias() things, and totally ignoring any locking around __d_move(), the whole "mark things MNT_DIR_ESCAPED" is a lot more complex.

Linus

Point, but... Keep in mind that there's another PITA in there - unreachable submounts are not as harmless as Eric hopes. umount -l of the entire tainted mount is a very large hammer _and_ userland needs to know when to use it in the first place; otherwise we'll end up with dirty reboots. So slightly longer term I want to have something done to them when they become unreachable. Namely, detach and leave in their place a trap that would give EINVAL on attempt to cross. Details depend on another pile of patches to review and serialize (nfsd-related fs_pin stuff), but catching the moments when they become unreachable is going to be useful (IOW, I don't see how to do it without catching those; there might be an elegant solution I'm missing, of course).

Basically what I have in mind is a few helpers called from
dentry_lock_for_move() with d_move(), d_exchange() and d_splice_alias() doing read_seqlock_excl(&mount_lock); just before grabbing rename_lock and dropping it right after dropping rename_lock.

find_and_taint(dentry, ancestor)

{
	if (!is_dir(dentry))
		return;
	for (p = dentry->d_parent; p != ancestor; p = next) {
		if (unlikely(d_is_someones_root(p)))
			taint_mounts_of_this_subtree(p);
			// ... with dentry passed there as well when we
			// start handling unreachable submounts.
		next = p->d_parent;
		if (p == next)
			break;
	}
}

depth(d)

{
	int n;
	for (n = 0; !IS_ROOT(d); d = d->d_parent, n++)
		;
	return n;
}

/* find the last common ancestor of d1 and d2; NULL if there isn't one */ LCA(d1, d2)

{
	int n1 = depth(d1), n2 = depth(d2);
	if (n1 > n2)
		do d1 = d1->d_parent; while (--n1 != n2);
	else if (n1 < n2)
		do d2 = d2->d_parent; while (--n2 != n1);
	while (d1 != d2) {
		if (unlikely(IS_ROOT(d1)))
			return NULL;
		d1 = d1->d_parent;
		d2 = d2->d_parent;
	}
	return d1;
}

dentry_lock_for_move(dentry, target, exchange)

{
	ancestor = LCA(dentry, target);
	BUG_ON(ancestor == dentry);	// these BUG_ON are antisocial, BTW
	BUG_ON(ancestor == target);
	find_and_taint(dentry, ancestor);
	if (exchange)
		find_and_taint(target, ancestor);
	// the rest - as we do now
}

*Rolls my eyes*

This has to be one of the most inconsistent lines of reasoning I have ever heard. Either escaping from a bind mount is as rare as hen's teeth and our handling of the case is to keep it that way. Or if they are common we really need to handle people performing lookups on bind mounts people have escaped from.

If escaping from bind mounts is as rare as hen's teeth (which it had better be), then we don't have to be clever, we don't have to be nice, and umount -l is perfectly sufficient. And most of the time it really will be an exit of a mount namespace that blows things away anyway.

Certainly all of the easy to create cases require creating a user namespace and which onws a mount namespace that a less privileged user can manipulate.

If you gave someone you don't trust permission to move a directory under which you have a mount point in the initial mount namespace and that causes problems I am pretty certain that is a PBKAC error.

But even if I accept that the unmount code has to be done you have been objecting to the infrastructure that is needed to make it happen.

That is to unmount something we must take namespace_sem from d_splice_alias. But more specifically we must call namespace_lock() and namespace_unlock() from d_splice_alias. And there are no hacks like trylock for running synchronize_rcu. That is we must sleep in
d_splice_alias to unmount things. Arranging the locks so that it can
happen is what you have rather been strenuously objecting to.

Further if you want to unmount things you can't do it from __d_move inside the rename_lock. The logic must be outside the rename_lock.

So you are arguing for code that has heavier locking than what I have done, and you are conveniently ignoring the cost of reviewing all of the code to see if i_lock is ever taken under mount_lock.

You are of course missing the common case and the obvious optimization here:

	if (d1->d_parent == d2->d_parent)
		return d1->d_parent;

Which begs the question. What is the point of doing this in dentry_lock_for_move. None of that code has any logical connection to the function of dentry_lock_for_move. And if the rest of the logic is untouched it is just stupid to do the work in dentry_lock_for_move.

Al I am sorry. I am having a very hard time taking your comments on code direction seriously at this point. I have tested working code.

You have some sort of half thought out proof of concept code that you have never published. And you are complaining that my working code is not your half thought through code.

In my last round of patches that I sent out today. I did put mount_lock just outside of rename_lock, in d_splice_alias. But apparently you haven't noticed.

Now at this point I have hit the limit of my time available for rewrites before the merge window. We can go with my 7 patch variant I posted today (whose only sin appears not to be your implemenation), it's trivial reduction that Linus likes because it is simple, someone else can write one, or this can all wait until the next development cycle.

Given that I have working code with no known defects I was really hoping I could plug this security hole this development cycle.

Eric

I have. The problem I have with that one is that you end up with duplicated logics rather than taking it to one place.

... or either of us can do merging those checks into a single place, be it as a followup to your 7-patch series, or folded with the fs/dcache.c-affecting patches in there. If you have no time left, I can certainly do that followup myself - not a problem[1]

And umount-related followups are just that - I'm not asking you to do those, especially since as I said this stuff is sensitive to fs_pin details (so far it appears to fold nicely with the __detach_mounts()/umount_tree() stuff, BTW).

[1] with credits for your patches preserved - normally I would assume that this goes without saying, but your reply seems to imply that I'm playing some kind of political BS games, so I'd rather spell that out.

I have. The problem I have with that one is that you end up with duplicated logics rather than taking it to one place.

... or either of us can do merging those checks into a single place, be it as a followup to your 7-patch series, or folded with the fs/dcache.c-affecting patches in there. If you have no time left, I can certainly do that followup myself - not a problem[1]

And umount-related followups are just that - I'm not asking you to do those, especially since as I said this stuff is sensitive to fs_pin details (so far it appears to fold nicely with the __detach_mounts()/umount_tree() stuff, BTW).

[1] with credits for your patches preserved - normally I would assume that this goes without saying, but your reply seems to imply that I'm playing some kind of political BS games, so I'd rather spell that out.

PS: it doesn't need namespace_sem taken inside __d_move() - actual detaching does, of course, but that part gets done via task_work_add(), in a reasonably sane locking environment.

We are talking about a very small duplication of code. But you are also talking about combining logic that can get you in trouble pretty easily.

As I read the code sketch you posted it had the issue that if a disconnected dentry was also a mount point it would mark that mount point as escaped (because there was no common ancestor).

In the split out logic I don't even bother because you trivially can't have escaped from anywhere when IS_ROOT(dentry).

I don't have time. Everytime I have worked with this it has take pretty much full days of staring at the code, and I don't have any more full days left before the merge window.

The part that boggles my mind about that whole approach is that just outside of rename_lock there already is a sane locking environment.

Admittedly it will take a touch of work in d_splice_alias to get it sane, but that isn't particularly hard.

	write_seqlock(&rename_lock);
        if (!IS_ROOT(new))
		__d_unalias(...);

__d_unalias(...)

{	
        /* Parent's don't go away so !IS_ROOT(new) will stay valid */
        write_sequnlock(&rename_lock);
        /* Hooray! The code can be normal and sleep if it needs to,
         * before calling into __d_move.
         */
         ...
         d_move();
}

Similarly for duplicate logic removal. Teaching d_splice_alias when it is performing an ordinarly rename to be able to just call d_move gets you far more from a maintenance perspective than cramming everything into __d_move.

My issue wasn't political games, but rather holding code to an apparently BS standard.

Eric

OK, at that point I've pretty much given up on fs_pin for this cycle. And testing your variant with unconditional checks on .. appears to have fairly low overhead. I still want to deal with catching and unmounting the unreachable suckers, so fs/dcache.c side of things will get used when we get to that stuff, but for now I've taken your 1/7, 2/7 plus the variant of "vfs: Test for and handle paths that are unreachable from their mnt_root" that doesn't care whether anything escaped or not.

3--6 are held in a local branch for now; I *am* going to use them come next cycle. And there's another pile of fun around that area, also for the next cycle - kernel-initiated subtree removals on things like sysfs et.al.; handling of the locking in those is inconsistent and tied with the fun we have for d_move()/__d_unalias(). Sigh...

I am sorry to hear about a mess in sysfs.

I am glad to hear that we do not appear to need MNT_DIR_ESCAPED to avoid performance regressions. That should make back porters lives much easier.

As for the future I have a suspicion that we want to look at the rcu readable dynamically sized hash tables the networking guys cooked up.

Al thank you very much for performance testing the simple version.

Eric

I am sorry to hear about a mess in sysfs.

I am glad to hear that we do not appear to need MNT_DIR_ESCAPED to avoid performance regressions. That should make back porters lives much easier.

As for the future I have a suspicion that we want to look at the rcu readable dynamically sized hash tables the networking guys cooked up.

Al thank you very much for performance testing the simple version.

Eric

It occurs to me that there is a fairly simple way we can emperically test to see how expensive calling is_subdir for every .. on a bind mount is in practice.

- Take my last patch
- run a benchmark outside of a bind mount (perhaps a kernel compile).
- run the same benchmark inside of a bind mount.

See if the performance differs.

I am going to try to find time to do this, but I am travelling for the next couple of days.

If someone who has a bit more time wants to try it and beats me to that would be great.

I think having some emperical numbers would be nice in this part of the conversation.

Eric

I took a bit of time to do something simpler though less scientific : I just ran a kernel build under strace -c -f to get a rough idea of the number of syscalls. It takes 2m23 without strace and 3m00 under strace. 7.2M syscalls were seen out of which only 3.2M were related to FS access (mostly open(), fstat() and stat()), the rest is minor. That's 22k syscalls per second, or 45 us between two syscalls. From this point I'm having a hard time imagining that any extra tests in the code path would have a significant impact to cause a measurable difference compared to these 45us, especially since I'm counting all FS accesses (worst case) and not just those referencing "..".

Just my two cents,
Willy

I took a bit of time to do something simpler though less scientific : I just ran a kernel build under strace -c -f to get a rough idea of the number of syscalls. It takes 2m23 without strace and 3m00 under strace. 7.2M syscalls were seen out of which only 3.2M were related to FS access (mostly open(), fstat() and stat()), the rest is minor. That's 22k syscalls per second, or 45 us between two syscalls. From this point I'm having a hard time imagining that any extra tests in the code path would have a significant impact to cause a measurable difference compared to these 45us, especially since I'm counting all FS accesses (worst case) and not just those referencing "..".

Just my two cents,
Willy

When bind mounts are in use, and there is another path to the filesystem it is possible to rename files or directories from a path underneath the root of the bind mount to a path that is not underneath the root of the bind mount.

When a directory is moved out from under the root of a bind mount path name lookups that go up the directory tree potentially allow accessing the entire dentry tree of the filesystem. This is not expected, not what is desired and winds up being a secruity problem for userspace.

Augment d_move, d_exchange, and __d_unalias to call d_common_ancestor and handle_possible_mount_escapes to mark any mount points that directories escape from.

A few notes on the implementation:

- Only directory escapes are recorded as only those are relevant to new pathname lookup. Escaped files are handled in prepend_path.

- A lock either namespace_sem or mount_lock needs to be held across the duration of renames where a directory could be escaping to ensure that a mount is not added, escaped, and missed during the rename.

- The mount_lock is used as it does not sleep. I have audited all of thecallers of d_move and d_exchange and in every instance it appears safe for d_move and d_exchange to start sleeping. But there is no point in adding sleeping behavior if that is unncessary.

- The locking order must be mount_lock outside of rename_lock as prepend_path already takes the locks in this order.

- d_splice_alias (which calls __d_unalias) is a painful when it comes to this kind of locking, as it mostly takes the spinlocks before the sleeping locks. So I have implmented the suboptimal but stupid and correct version of the locking and always take mount_lock.

---
 fs/dcache.c           | 40 ++++++++++++++++++++++++++++++++++++++++
 fs/mount.h            |  2 ++
 fs/namespace.c        | 32 ++++++++++++++++++++++++++++++++
 include/linux/mount.h |  1 +
 4 files changed, 75 insertions(+)

index 4e66bf92a481..ccc7daa0ae71 100644
@@ -2704,9 +2704,23 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
  */
 void d_move(struct dentry *dentry, struct dentry *target)
 {
+	bool unlock = false;
+
+	if (d_is_dir(dentry) && (dentry->d_parent != target->d_parent)) {
+		const struct dentry *ancestor;
+
+		ancestor = d_common_ancestor(dentry, target);
+		read_seqlock_excl(&mount_lock);
+		unlock = true;
+		handle_possible_mount_escapee(ancestor, dentry);
+	}
+
 	write_seqlock(&rename_lock);
 	__d_move(dentry, target, false);
 	write_sequnlock(&rename_lock);
+	if (unlock)
+		read_sequnlock_excl(&mount_lock);
+
 }
 EXPORT_SYMBOL(d_move);
 

@@ -2717,6 +2731,23 @@ EXPORT_SYMBOL(d_move);
  */
 void d_exchange(struct dentry *dentry1, struct dentry *dentry2)
 {
+	bool d1_is_dir = d_is_dir(dentry1);
+	bool d2_is_dir = d_is_dir(dentry2);
+	bool unlock = false;
+
+	if ((d1_is_dir || d2_is_dir) &&
+	    (dentry1->d_parent != dentry2->d_parent)) {
+		const struct dentry *ancestor;
+
+		ancestor = d_common_ancestor(dentry1, dentry2);
+		read_seqlock_excl(&mount_lock);
+		unlock = true;
+		if (d1_is_dir)
+			handle_possible_mount_escapee(ancestor, dentry1);
+		if (d2_is_dir)
+			handle_possible_mount_escapee(ancestor, dentry2);
+	}
+
 	write_seqlock(&rename_lock);
 
 	WARN_ON(!dentry1->d_inode);

@@ -2727,6 +2758,8 @@ void d_exchange(struct dentry *dentry1, struct dentry *dentry2)
 	__d_move(dentry1, dentry2, true);
 
 	write_sequnlock(&rename_lock);
+	if (unlock)
+		read_sequnlock_excl(&mount_lock);
 }
 
 /**

@@ -2761,6 +2794,7 @@ static int __d_unalias(struct inode *inode,
 		struct dentry *dentry, struct dentry *alias)
 {
 	struct mutex *m1 = NULL, *m2 = NULL;
+	const struct dentry *ancestor;
 	int ret = -ESTALE;
 
 	/* If alias and dentry share a parent, then no extra locks required */

@@ -2774,6 +2808,8 @@ static int __d_unalias(struct inode *inode,
 	if (!mutex_trylock(&alias->d_parent->d_inode->i_mutex))
 		goto out_err;
 	m2 = &alias->d_parent->d_inode->i_mutex;
+	ancestor = d_common_ancestor(alias, dentry);
+	handle_possible_mount_escapee(ancestor, alias);
 out_unalias:
 	__d_move(alias, dentry, false);
 	ret = 0;

@@ -2825,9 +2861,11 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 		if (unlikely(new)) {
 			/* The reference to new ensures it remains an alias */
 			spin_unlock(&inode->i_lock);
+			read_seqlock_excl(&mount_lock);
 			write_seqlock(&rename_lock);
 			if (unlikely(d_ancestor(new, dentry))) {
 				write_sequnlock(&rename_lock);
+				read_sequnlock_excl(&mount_lock);
 				dput(new);
 				new = ERR_PTR(-ELOOP);
 				pr_warn_ratelimited(

@@ -2839,6 +2877,7 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 			} else if (!IS_ROOT(new)) {
 				int err = __d_unalias(inode, dentry, new);
 				write_sequnlock(&rename_lock);
+				read_sequnlock_excl(&mount_lock);
 				if (err) {
 					dput(new);
 					new = ERR_PTR(err);

@@ -2846,6 +2885,7 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 			} else {
 				__d_move(new, dentry, false);
 				write_sequnlock(&rename_lock);
+				read_sequnlock_excl(&mount_lock);
 				security_d_instantiate(new, inode);
 			}
 			iput(inode);

index e8f22970fe59..ad91963c83ac 100644
@@ -107,6 +107,8 @@ static inline void detach_mounts(struct dentry *dentry)
 	__detach_mounts(dentry);
 }
 
+extern void handle_possible_mount_escapee(const struct dentry *, struct dentry *);
+
 static inline void get_mnt_ns(struct mnt_namespace *ns)
 {
 	atomic_inc(&ns->count);

index af6abf476394..ddcd0b61a448 100644
@@ -1657,6 +1657,38 @@ out_unlock:
 	namespace_unlock();
 }
 
+static void mark_escaped_mounts(struct dentry *root)
+{
+	/* Must be called with mount_lock held */
+	struct mountroot *mr;
+	struct mount *mnt;
+
+	mr = lookup_mountroot(root);
+	if (mr) {
+		/* Mark each mount from which a directory is escaping.
+		 */
+		hlist_for_each_entry(mnt, &mr->r_list, mnt_mr_list)
+			mnt->mnt.mnt_flags |= MNT_DIR_ESCAPED;
+	}
+}
+
+void handle_possible_mount_escapee(const struct dentry *ancestor,
+				   struct dentry *escapee)
+{
+	struct dentry *dentry;
+
+	for (dentry = escapee->d_parent; dentry != ancestor;
+	     dentry = dentry->d_parent) {
+
+		if (d_mountroot(dentry))
+			mark_escaped_mounts(dentry);
+
+		/* In case there is no common ancestor */
+		if (IS_ROOT(dentry))
+			break;
+	}
+}
+
 /* 
  * Is the caller allowed to modify his namespace?
  */

index f822c3c11377..e58bc12b19aa 100644
@@ -62,6 +62,7 @@ struct mnt_namespace;
 #define MNT_SYNC_UMOUNT		0x2000000
 #define MNT_MARKED		0x4000000
 #define MNT_UMOUNT		0x8000000
+#define MNT_DIR_ESCAPED		0x10000000
 
 struct vfsmount {
 	struct dentry *mnt_root;	/* root of the mounted tree */

2.2.1

In rare cases a directory can be renamed out from under a bind mount. In those cases without special handling it becomes possible to walk up the directory tree to the root dentry of the filesystem and down from the root dentry to every other file or directory on the filesystem.

Like division by zero .. from an unconnected path can not be given a useful semantic as there is no predicting at which path component the code will realize it is unconnected. We certainly can not match the current behavior as the current behavior is a security hole.

Therefore when encounting .. when following an unconnected path return -ENOENT.

- Add a function path_connected to verify path->dentry is reachable from path->mnt.mnt_root. AKA to validate that rename did not do something nasty to the bind mount.

To avoid races path_connected must be called after following a path component to it's next path component.

---
 fs/namei.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

index ae4e4c18b2ac..18d4884c7e85 100644
@@ -560,6 +560,23 @@ static int __nd_alloc_stack(struct nameidata *nd)
 	return 0;
 }
 
+/**
+ * path_connected - Verify that a path->dentry is below path->mnt.mnt_root
+ * @path: nameidate to verify
+ *
+ * Rename can sometimes move a file or directory outside of a bind
+ * mount, path_connected allows those cases to be detected.
+ */
+static bool path_connected(const struct path *path)
+{
+	struct vfsmount *mnt = path->mnt;
+
+	if (likely(!(mnt->mnt_flags & MNT_DIR_ESCAPED)))
+		return true;
+
+	return is_subdir(path->dentry, mnt->mnt_root);
+}
+
 static inline int nd_alloc_stack(struct nameidata *nd)
 {
 	if (likely(nd->depth != EMBEDDED_LEVELS))

@@ -1296,6 +1313,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 				return -ECHILD;
 			nd->path.dentry = parent;
 			nd->seq = seq;
+			if (unlikely(!path_connected(&nd->path)))
+				return -ENOENT;
 			break;
 		} else {
 			struct mount *mnt = real_mount(nd->path.mnt);

@@ -1396,7 +1415,7 @@ static void follow_mount(struct path *path)
 	}
 }
 
-static void follow_dotdot(struct nameidata *nd)
+static int follow_dotdot(struct nameidata *nd)
 {
 	if (!nd->root.mnt)
 		set_root(nd);

@@ -1412,6 +1431,8 @@ static void follow_dotdot(struct nameidata *nd)
 			/* rare case of legitimate dget_parent()... */
 			nd->path.dentry = dget_parent(nd->path.dentry);
 			dput(old);
+			if (unlikely(!path_connected(&nd->path)))
+				return -ENOENT;
 			break;
 		}
 		if (!follow_up(&nd->path))

@@ -1419,6 +1440,7 @@ static void follow_dotdot(struct nameidata *nd)
 	}
 	follow_mount(&nd->path);
 	nd->inode = nd->path.dentry->d_inode;
+	return 0;
 }
 
 /*

@@ -1634,7 +1656,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
 		if (nd->flags & LOOKUP_RCU) {
 			return follow_dotdot_rcu(nd);
 		} else
-			follow_dotdot(nd);
+			return follow_dotdot(nd);
 	}
 	return 0;
 }

2.2.1

In rare cases a directory can be renamed out from under a bind mount. In those cases without special handling it becomes possible to walk up the directory tree to the root dentry of the filesystem and down from the root dentry to every other file or directory on the filesystem.

Like division by zero .. from an unconnected path can not be given a useful semantic as there is no predicting at which path component the code will realize it is unconnected. We certainly can not match the current behavior as the current behavior is a security hole.

Therefore when encounting .. when following an unconnected path return -ENOENT.

- Add a function path_connected to verify path->dentry is reachable from path->mnt.mnt_root. AKA to validate that rename did not do something nasty to the bind mount.

To avoid races path_connected must be called after following a path component to it's next path component.

---
 fs/namei.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

index ae4e4c18b2ac..18d4884c7e85 100644
@@ -560,6 +560,23 @@ static int __nd_alloc_stack(struct nameidata *nd)
 	return 0;
 }
 
+/**
+ * path_connected - Verify that a path->dentry is below path->mnt.mnt_root
+ * @path: nameidate to verify
+ *
+ * Rename can sometimes move a file or directory outside of a bind
+ * mount, path_connected allows those cases to be detected.
+ */
+static bool path_connected(const struct path *path)
+{
+	struct vfsmount *mnt = path->mnt;
+
+	if (likely(!(mnt->mnt_flags & MNT_DIR_ESCAPED)))
+		return true;
+
+	return is_subdir(path->dentry, mnt->mnt_root);
+}
+
 static inline int nd_alloc_stack(struct nameidata *nd)
 {
 	if (likely(nd->depth != EMBEDDED_LEVELS))

@@ -1296,6 +1313,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 				return -ECHILD;
 			nd->path.dentry = parent;
 			nd->seq = seq;
+			if (unlikely(!path_connected(&nd->path)))
+				return -ENOENT;
 			break;
 		} else {
 			struct mount *mnt = real_mount(nd->path.mnt);

@@ -1396,7 +1415,7 @@ static void follow_mount(struct path *path)
 	}
 }
 
-static void follow_dotdot(struct nameidata *nd)
+static int follow_dotdot(struct nameidata *nd)
 {
 	if (!nd->root.mnt)
 		set_root(nd);

@@ -1412,6 +1431,8 @@ static void follow_dotdot(struct nameidata *nd)
 			/* rare case of legitimate dget_parent()... */
 			nd->path.dentry = dget_parent(nd->path.dentry);
 			dput(old);
+			if (unlikely(!path_connected(&nd->path)))
+				return -ENOENT;
 			break;
 		}
 		if (!follow_up(&nd->path))

@@ -1419,6 +1440,7 @@ static void follow_dotdot(struct nameidata *nd)
 	}
 	follow_mount(&nd->path);
 	nd->inode = nd->path.dentry->d_inode;
+	return 0;
 }
 
 /*

@@ -1634,7 +1656,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
 		if (nd->flags & LOOKUP_RCU) {
 			return follow_dotdot_rcu(nd);
 		} else
-			follow_dotdot(nd);
+			return follow_dotdot(nd);
 	}
 	return 0;
 }

2.2.1

In rare cases a directory can be renamed out from under a bind mount. In those cases without special handling it becomes possible to walk up the directory tree to the root dentry of the filesystem and down from the root dentry to every other file or directory on the filesystem.

Like division by zero .. from an unconnected path can not be given a useful semantic as there is no predicting at which path component the code will realize it is unconnected. We certainly can not match the current behavior as the current behavior is a security hole.

Therefore when encounting .. when following an unconnected path return -ENOENT.

- Add a function path_connected to verify nd->path.dentry is reachable from nd->path.mnt.mnt_root. AKA to validate that rename did not do something nasty to the bind mount.

To avoid races path_connected must be called after following a path component to it's next path component.

---
 fs/namei.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

index ae4e4c18b2ac..bccd3810ff60 100644
@@ -560,6 +560,27 @@ static int __nd_alloc_stack(struct nameidata *nd)
 	return 0;
 }
 
+/**
+ * path_connected - Verify that a nd->path.dentry is below nd->path.mnt->mnt.mnt_root
+ * @nd: nameidate to verify
+ *
+ * Rename can sometimes move a file or directory outside of a bind
+ * mount, path_connected allows those cases to be detected.
+ */
+static bool path_connected(struct nameidata *nd)
+{
+	struct vfsmount *mnt = nd->path.mnt;
+	unsigned escape_count = read_mnt_escape_count(mnt);
+
+	if (likely(escape_count == 0))
+		return true;
+
+	if (!is_subdir(nd->path.dentry, mnt->mnt_root))
+		return false;
+
+	return true;
+}
+
 static inline int nd_alloc_stack(struct nameidata *nd)
 {
 	if (likely(nd->depth != EMBEDDED_LEVELS))

@@ -1294,6 +1315,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 			seq = read_seqcount_begin(&parent->d_seq);
 			if (unlikely(read_seqcount_retry(&old->d_seq, nd->seq)))
 				return -ECHILD;
+			if (unlikely(!path_connected(nd)))
+				return -ENOENT;
 			nd->path.dentry = parent;
 			nd->seq = seq;
 			break;

@@ -1396,7 +1419,7 @@ static void follow_mount(struct path *path)
 	}
 }
 
-static void follow_dotdot(struct nameidata *nd)
+static int follow_dotdot(struct nameidata *nd)
 {
 	if (!nd->root.mnt)
 		set_root(nd);

@@ -1410,7 +1433,12 @@ static void follow_dotdot(struct nameidata *nd)
 		}
 		if (nd->path.dentry != nd->path.mnt->mnt_root) {
 			/* rare case of legitimate dget_parent()... */
-			nd->path.dentry = dget_parent(nd->path.dentry);
+			struct dentry *parent = dget_parent(nd->path.dentry);
+			if (unlikely(!path_connected(nd))) {
+				dput(parent);
+				return -ENOENT;
+			}
+			nd->path.dentry = parent;
 			dput(old);
 			break;
 		}

@@ -1419,6 +1447,7 @@ static void follow_dotdot(struct nameidata *nd)
 	}
 	follow_mount(&nd->path);
 	nd->inode = nd->path.dentry->d_inode;
+	return 0;
 }
 
 /*

@@ -1634,7 +1663,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
 		if (nd->flags & LOOKUP_RCU) {
 			return follow_dotdot_rcu(nd);
 		} else
-			follow_dotdot(nd);
+			return follow_dotdot(nd);
 	}
 	return 0;
 }

2.2.1

is_subdir does the read_seqretry on rename_lock. Which is enough to ensure connectivity exists at a single moment in time.

Beyond that the entire path lookup races with d_move, and the code calls path_connected just after finding the parent directory, which ensures that in the moment that follow_dotdot is setting nd->dentry that the original nd->dentry is connected, and by extension the new as the new one is an ancestor.

Or are you thinking of a different race?

Eric

Add a new field mnt_escape_count in nameidata, initialize it to 0 and cache the value of read_mnt_escape_count in nd->mnt_escape_count.

This allows a single check in path_connected in the common case where either the mount has had no escapes (mnt_escape_count == 0) or there has been an escape and it has been validated that the current path does not escape.

To keep the cache valid nd->mnt_escape_count must be set to 0 whenever the nd->path.mnt changes or when nd->path.dentry changes such that the connectedness of the previous value of nd->path.dentry does not imply the connected of the new value of nd->path.dentry.

Various locations in fs/namei.c are updated to set nd->mnt_escape_count to 0 as necessary.

---
 fs/namei.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

index bccd3810ff60..79a5dca073f5 100644
@@ -514,6 +514,7 @@ struct nameidata {
 	struct nameidata *saved;
 	unsigned	root_seq;
 	int		dfd;
+	unsigned	mnt_escape_count;
 };
 
 static void set_nameidata(struct nameidata *p, int dfd, struct filename *name)

@@ -572,12 +573,13 @@ static bool path_connected(struct nameidata *nd)
 	struct vfsmount *mnt = nd->path.mnt;
 	unsigned escape_count = read_mnt_escape_count(mnt);
 
-	if (likely(escape_count == 0))
+	if (likely(escape_count == nd->mnt_escape_count))
 		return true;
 
 	if (!is_subdir(nd->path.dentry, mnt->mnt_root))
 		return false;
 
+	cache_mnt_escape_count(&nd->mnt_escape_count, escape_count);
 	return true;
 }
 

@@ -840,6 +842,9 @@ static inline void path_to_nameidata(const struct path *path,
 		if (nd->path.mnt != path->mnt)
 			mntput(nd->path.mnt);
 	}
+	if (unlikely((nd->path.mnt != path->mnt) ||
+		     (nd->path.dentry != path->dentry->d_parent)))
+		nd->mnt_escape_count = 0;
 	nd->path.mnt = path->mnt;
 	nd->path.dentry = path->dentry;
 }

@@ -856,6 +861,7 @@ void nd_jump_link(struct path *path)
 	nd->path = *path;
 	nd->inode = nd->path.dentry->d_inode;
 	nd->flags |= LOOKUP_JUMPED;
+	nd->mnt_escape_count = 0;
 }
 
 static inline void put_link(struct nameidata *nd)

@@ -1040,6 +1046,7 @@ const char *get_link(struct nameidata *nd)
 			nd->inode = nd->path.dentry->d_inode;
 		}
 		nd->flags |= LOOKUP_JUMPED;
+		nd->mnt_escape_count = 0;
 		while (unlikely(*++res == '/'))
 			;
 	}

@@ -1335,6 +1342,7 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 			nd->path.mnt = &mparent->mnt;
 			inode = inode2;
 			nd->seq = seq;
+			nd->mnt_escape_count = 0;
 		}
 	}
 	while (unlikely(d_mountpoint(nd->path.dentry))) {

@@ -1348,6 +1356,7 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 		nd->path.dentry = mounted->mnt.mnt_root;
 		inode = nd->path.dentry->d_inode;
 		nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
+		nd->mnt_escape_count = 0;
 	}
 	nd->inode = inode;
 	return 0;

@@ -1406,8 +1415,9 @@ EXPORT_SYMBOL(follow_down);
 /*
  * Skip to top of mountpoint pile in refwalk mode for follow_dotdot()
  */
-static void follow_mount(struct path *path)
+static bool follow_mount(struct path *path)
 {
+	bool followed = false;
 	while (d_mountpoint(path->dentry)) {
 		struct vfsmount *mounted = lookup_mnt(path);
 		if (!mounted)

@@ -1416,7 +1426,9 @@ static void follow_mount(struct path *path)
 		mntput(path->mnt);
 		path->mnt = mounted;
 		path->dentry = dget(mounted->mnt_root);
+		followed = true;
 	}
+	return followed;
 }
 
 static int follow_dotdot(struct nameidata *nd)

@@ -1444,8 +1456,10 @@ static int follow_dotdot(struct nameidata *nd)
 		}
 		if (!follow_up(&nd->path))
 			break;
+		nd->mnt_escape_count = 0;
 	}
-	follow_mount(&nd->path);
+	if (follow_mount(&nd->path))
+		nd->mnt_escape_count = 0;
 	nd->inode = nd->path.dentry->d_inode;
 	return 0;
 }

@@ -1997,6 +2011,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 	nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
 	nd->depth = 0;
 	nd->total_link_count = 0;
+	nd->mnt_escape_count = 0;
 	if (flags & LOOKUP_ROOT) {
 		struct dentry *root = nd->root.dentry;
 		struct inode *inode = root->d_inode;

@@ -3026,6 +3041,7 @@ static int do_last(struct nameidata *nd,
 	unsigned seq;
 	struct inode *inode;
 	struct path save_parent = { .dentry = NULL, .mnt = NULL };
+	unsigned save_parent_escape_count = 0;
 	struct path path;
 	bool retried = false;
 	int error;

@@ -3155,6 +3171,9 @@ finish_lookup:
 	} else {
 		save_parent.dentry = nd->path.dentry;
 		save_parent.mnt = mntget(path.mnt);
+		save_parent_escape_count = nd->mnt_escape_count;
+		if (nd->path.dentry != path.dentry->d_parent)
+			nd->mnt_escape_count = 0;
 		nd->path.dentry = path.dentry;
 
 	}

@@ -3227,6 +3246,7 @@ stale_open:
 
 	BUG_ON(save_parent.dentry != dir);
 	path_put(&nd->path);
+	nd->mnt_escape_count = save_parent_escape_count;
 	nd->path = save_parent;
 	nd->inode = dir->d_inode;
 	save_parent.mnt = NULL;

2.2.1

It is an interesting question.

For the locking on rename we have something that looks like:
mutex_lock(&...s_vfs_mutex); mutex_lock(&p2->d_inode->i_mutex); mutex_lock(&p1->d_inode->i_mutex); read_seqlock_excl(&mount_lock);
escape_count++;
write_seqlock(&rename_lock); write_seqcount_begin(&dentry->d_seq); write_seqcount_begin_nested(&target->d_seq); write_seqcount_end_nested(&target->d_seq); write_seqcount_end(&dentry->d_seq); write_sequnlock(&rename_lock);
escape_count++;
read_sequnlock_excl(&mount_lock); mutex_unlock(&p1->d_inode->i_mutex); mutex_unlock(&p2->d_inode->i_mutex); mutex_unlock(&...s_vfs_mutex);

Which is at least 16 serialized cpu operations. To reach overflow then it would take at least 16 * 2**32 operations cpu operations. On a 4Ghz 16 * 2**32 operations would take roughly 16 seconds. In practice I think it would take noticably more than 16 seconds to perform that many renames as there is a lot more going on than just the locking I signled out.

A pathname lookup taking 16 seconds seems absurd. But perhaps in the worst case.

The maximum length of a path that can be passed into path_lookup is 4096. For a lookup to be problematic there must be at least as many instances of .. as there are of any other path component. So each pair of a minium length path element and a .. element must take at least 5 bytes. Which in 4096 bytes leaves room for 819 path elements. If every one of those 819 path components triggered a disk seek at 100 seeks per second I could see a path name lookup potentially taking 8 seconds.

MAXSYMLINKS is 40. So you could perhaps if you have a truly pathlogical set of file names might make that 320 seconds. Ick.

To get some real figures I have performed a few renames on my 2.5Ghz laptop with ext4 on an ssd. Performing a simple rename in the same directory (which involves much less than in required to rename a file out of a mount point) I get the following timings:

renames time
200,000 1.2s
2,000,000 17.2s
20,000,000 205.6s
40,000,000 418.5s

At that kind of speed I would expect 4,000,000,000 renames to take 41850 seconds or 11.625 hours.

Going the other way on an old system with spinning rust for a hard drive I created 1000 nested directories all named a and times how long it would take to stat a pathlogical pathname. With a simple pathname without symlinks involved the worst case I have been able to trigger
is a 0.3 second path name lookup time. Not being satisified with
that I managed to create a file about 83,968 directories deep and a set of 40 symlinks setup up to get me there in one stat call the first symlink being 8192 directories deep. The worst case time I have been able to measure from stat -L on the symlink that leads me to that file is 14.5 seconds.

If my numbers are at all representative I don't see a realistic possibility of being able to perform enough renames to roll over a 32bit mnt_escape_count during a path name lookup. Even my most optimistic estimate required 16 seconds to perform the renames and I have not been able to get any pathname lookup to run that long.

At the same time I do think it is probably worth it to make escape count an unsigned long, because that is practically free and it removes the any theoretical concern on 64bit.

Am I missing anything?

Eric

A lookup on NFS while a server's rebooting or the network's flaky could take arbitrary long. Other network filesystems and fuse can have similar problems. Depending on threat model an attacker might have quite precise control over that timing. Disk filesystems could have all the same problems since there's no guarantee the underlying block device is really local. Even ignoring that, hardware can be slow or flaky. And couldn't an allocation in theory block for an arbitrary long time?

Apologies for just dropping into the middle here! I haven't read the rest and don't have the context to know whether any of that's relevant.

--b.

A lookup on NFS while a server's rebooting or the network's flaky could take arbitrary long. Other network filesystems and fuse can have similar problems. Depending on threat model an attacker might have quite precise control over that timing. Disk filesystems could have all the same problems since there's no guarantee the underlying block device is really local. Even ignoring that, hardware can be slow or flaky. And couldn't an allocation in theory block for an arbitrary long time?

Apologies for just dropping into the middle here! I haven't read the rest and don't have the context to know whether any of that's relevant.

--b.

No problem. The basic question is: can 2Billion renames be performed on the same filesystem in less time than a single path lookup? Allowing the use of a 32bit counter.

Most of the issues that slow up lookup also slow up rename so I have not been focusing on them.

If you could look up thread and tell me what you think of the issue with file handle to dentry conversion and bind mounts I would be appreciate.

I have been testing a little more on my system and it appears that it takes an 60minutes give or take to perform 2 Billino renames on ramfs. A faster cpu (5Ghz?) could perhaps get that down to 30 minutes.

With no slow downs and no weirdness I have been able to get a single pathname lookup to take just over 2 minutes, and I expect I could get that to take another minute more.

Those numbers are within a factor of 10 of each other, and I expect someone clever could finagle something to overcome the rest. So sigh.

There just is not enough margin in there to be certain of things. Now with the small change to make that counter 64bit and that 30 minutes to wrap the counter becomes 240,000+ years. I think I can safely not worry about the issue. I just need to come up with a good 32bit implemenation.

Eric

No problem. The basic question is: can 2Billion renames be performed on the same filesystem in less time than a single path lookup? Allowing the use of a 32bit counter.

Most of the issues that slow up lookup also slow up rename so I have not been focusing on them.

If you could look up thread and tell me what you think of the issue with file handle to dentry conversion and bind mounts I would be appreciate.

I have been testing a little more on my system and it appears that it takes an 60minutes give or take to perform 2 Billino renames on ramfs. A faster cpu (5Ghz?) could perhaps get that down to 30 minutes.

With no slow downs and no weirdness I have been able to get a single pathname lookup to take just over 2 minutes, and I expect I could get that to take another minute more.

Those numbers are within a factor of 10 of each other, and I expect someone clever could finagle something to overcome the rest. So sigh.

There just is not enough margin in there to be certain of things. Now with the small change to make that counter 64bit and that 30 minutes to wrap the counter becomes 240,000+ years. I think I can safely not worry about the issue. I just need to come up with a good 32bit implemenation.

Eric

Certainly if you have control over an NFS or FUSE server then you can arrange for that to happen--just delay the lookup until you've processed enough renames. I don't know if that's interesting....

OK, I see your comments in "[PATCH review 0/6] Bind mount escape fixes", I'm not sure I understand yet, I'll take a closer look.

--b.

Not particularly when the whole point is to start with a bind mount, do something trick and then have access to the whole filesystem instead of just the part of the filesystem exposed by the bind mount.

If you control the filesystem you already have access to the entire filesystem, so you don't need to do something trick.

That something tricky is a well placed rename that borks the tree structure and causes .. to never see the subdirectory that is the root of the bind mount.

Thanks.

The file handle reconstitution code can certainly be affected by all of this. Given that it is an failure if reconnect_path can't reconnect the path of a file handle. I think it can reasonably considered an error in all cases if that path is outside of an exported bind mount, but I don't know that area particularly well. The solution might just be don't export file handles from bind mounts.

Eric

This is response is kind of ridiculously delayed; vacation and a couple other things interfered!:

I thought there was also a concern about impact on the sanity of the system as a whole beyond just the contents of one filesystem: e.g. you don't want an unprivileged user to be able to create an unmountable mountpoint.

I don't think there's any new cause for concern here.

I'd quibble with the language "don't export filehandles", *if* by that you mean "don't tell allow anyone to know filehandles". They're guessable, so keeping them secret doesn't guarantee much security.

The dangerous operation is open_by_handle, and people need to understand that if you allow someone to call that then you're effectively giving access to the whole filesystem. That's always been true. (We don't really have an efficient way to determine if a non-directory is in a given subtree anyway.)

Such filehandle-guessing attacks on NFS have long been well-understood. NOSUBTREECHECK can prevent them but causes other problems, so isn't the default.

So the basic rule I think is "don't allow lookup-by-filehandle (or NFS export) on part of a filesystem unless you'd be willing to allow it on the whole thing".

--b.

(So in case it wasn't clear: ACK to just ignoring this, I don't think your (otherwise interesting) observations point to anything that needs fixing in the lookup-by-filehandle case.)

--b.

Interesting. I guess it makes sense that filehandles can be guessed. I wonder if a crypto variant that is resistant to plain text attacks would be a practical defense.

We do have d_ancestor/is_subdir that is a compartively efficient way to see if a dentry is in a given subtree. As it does not need to perform the permission checks I believe it would be some cheaper than the current nfs subtree check code. I don't know if that would avoid the known problem with the subtree check code. Nor do I know if it would be cheap enough to use for every nfsd operation when a file handle is received.

Thanks for looking into this. It helps to know that someone who knows the history of what happens with filehandles has looked at this.

Eric

People have considered it. I don't think it would be hard: just generate some permanent server secret and use it to encrypt all filehandles (and decrypt them again when looking them up).

Some of the reasons I don't think it's been done:

- Well, it's work, and nobody's really felt that strongly about it.

- It's usually not that hard to create another filesystem when you need a real security boundary.

- Filehandles are forever. But it's hard to keep secrets forever, especially when they have to be transmitted over the network a lot. (In more detail: client applications can hold long-lived references to filesystem objects through open file descriptors or current working directories. They expect those to keep working even over server reboots. We don't even know about those references. So any filehandle we give out could be looked up at any time in the future. The only case where we consider it acceptable to return ESTALE is when the object's actually gone.)

That would solve the problem for directories, but not for files. For non-directories we'd need special support from the filesystem (since at the time we look up the filehandle the parent(s) may not be in the dcache yet). Steps to check subtree membership would be roughly (number of hardlinks) * (average depth). I think it's probably not worth it.

Anyway, forgive the digressions....

--b.

If viewed that way you are probably right.

I was simply thinking about using the existing subtree check without the inode_permission test. Which works at the cost of a readdir to reconnect an inode to a directory. I had not noticed the readdir before. So I admit it does not sound like something that is a particularly speedy way to go.

No problem. Thank you for the discussion. This has if nothing else allowed me to understand this from a real world perspective, and in particular allows me to understand which permission checks would be necessary to safely allow file handles in a user namespace (if we ever decide it is safe to allow that).

In short if you did not mount the filesystem you better not be nfs exporting the filesystem, or parts of the filesystem, or be allowed to use file handle access to the filesystem.

Eric

If viewed that way you are probably right.

I was simply thinking about using the existing subtree check without the inode_permission test. Which works at the cost of a readdir to reconnect an inode to a directory. I had not noticed the readdir before. So I admit it does not sound like something that is a particularly speedy way to go.

No problem. Thank you for the discussion. This has if nothing else allowed me to understand this from a real world perspective, and in particular allows me to understand which permission checks would be necessary to safely allow file handles in a user namespace (if we ever decide it is safe to allow that).

In short if you did not mount the filesystem you better not be nfs exporting the filesystem, or parts of the filesystem, or be allowed to use file handle access to the filesystem.

Eric

Agreed.

--b.

Agreed.

--b.

(So in case it wasn't clear: ACK to just ignoring this, I don't think your (otherwise interesting) observations point to anything that needs fixing in the lookup-by-filehandle case.)

--b.

- In follup_up and follow_up_rcu don't follow up if the current mount's mountpoint can not reach the parent mount's root.

- In prepend_path and it's callers in the d_path family don't follow to the parent mount if the current mount's mountpoint can not reach the parent mount's root.

---
 fs/dcache.c | 14 ++++++++++----
 fs/namei.c  | 27 +++++++++++++++++----------
 2 files changed, 27 insertions(+), 14 deletions(-)

index e07eb03f6de6..6e68312494ed 100644
@@ -2894,10 +2894,16 @@ restart:
 			struct mount *parent = ACCESS_ONCE(mnt->mnt_parent);
 			/* Global root? */
 			if (mnt != parent) {
-				dentry = ACCESS_ONCE(mnt->mnt_mountpoint);
-				mnt = parent;
-				vfsmnt = &mnt->mnt;
-				continue;
+				struct path new = {
+					.dentry = ACCESS_ONCE(mnt->mnt_mountpoint),
+					.mnt = &parent->mnt,
+				};
+				if (path_connected(&new)) {
+					mnt = parent;
+					dentry = new.dentry;
+					vfsmnt = new.mnt;
+					continue;
+				}
 			}
 			/*
 			 * Filesystems needing to implement special "root names"

index 83cdcdf36eed..40e56d76df34 100644
@@ -935,14 +935,16 @@ static int follow_up_rcu(struct path *path)
 {
 	struct mount *mnt = real_mount(path->mnt);
 	struct mount *parent;
-	struct dentry *mountpoint;
+	struct path new;
 
 	parent = mnt->mnt_parent;
-	if (&parent->mnt == path->mnt)
+	if (parent == mnt)
 		return 0;
-	mountpoint = mnt->mnt_mountpoint;
-	path->dentry = mountpoint;
-	path->mnt = &parent->mnt;
+	new.dentry = mnt->mnt_mountpoint;
+	new.mnt = &parent->mnt;
+	if (!path_connected(&new))
+		return 0;
+	*path = new;
 	return 1;
 }
 

@@ -960,7 +962,7 @@ int follow_up(struct path *path)
 {
 	struct mount *mnt = real_mount(path->mnt);
 	struct mount *parent;
-	struct dentry *mountpoint;
+	struct path new;
 
 	read_seqlock_excl(&mount_lock);
 	parent = mnt->mnt_parent;

@@ -968,13 +970,18 @@ int follow_up(struct path *path)
 		read_sequnlock_excl(&mount_lock);
 		return 0;
 	}
-	mntget(&parent->mnt);
-	mountpoint = dget(mnt->mnt_mountpoint);
+	new.dentry = mnt->mnt_mountpoint;
+	new.mnt = &parent->mnt;
+	if (!path_connected(&new)) {
+		read_sequnlock_excl(&mount_lock);
+		return 0;
+	}
+	mntget(new.mnt);
+	dget(new.dentry);
 	read_sequnlock_excl(&mount_lock);
 	dput(path->dentry);
-	path->dentry = mountpoint;
 	mntput(path->mnt);
-	path->mnt = &parent->mnt;
+	*path = new;
 	return 1;
 }
 EXPORT_SYMBOL(follow_up);

2.2.1

Rename can move a file or directory outside of a bind mount. This has allowed programs with paths below the renamed directory to traverse up their directory tree to the real root of the filesystem instead of just the root of their bind mount.

In the presence of such renames limit applications to what the bind mount intended to reveal by marking mounts that have had dentries renamed out of them with MNT_VIOLATED, marking mounts that can no longer walk up to their parent mounts with MNT_UMOUNT_PENDING and then lazily unmounting such mounts.

All moves go through __d_move so __d_move has been modified to mark all mounts whose dentries have been moved outside of them.

Once the root dentry of a violated mount has been found a new function mnt_set_violated is called to mark all mounts that have that dentry as their root as violated.

The worst case performance of the changes to __d_move is two extra trip from dentry and target to the root of their respective dentry trees.

This closes a hole where it was possible in some circumstances to follow .. past the root of a bind mount.

---
 fs/dcache.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/internal.h  |  1 +
 fs/namespace.c | 17 +++++++++++++++
 3 files changed, 81 insertions(+), 2 deletions(-)

index 6e68312494ed..7baecba354dd 100644
@@ -2535,6 +2535,56 @@ static void dentry_unlock_for_move(struct dentry *dentry, struct dentry *target)
 	spin_unlock(&dentry->d_lock);
 }
 
+static unsigned d_depth(const struct dentry *dentry)
+{
+	unsigned depth = 0;
+
+	while (!IS_ROOT(dentry)) {
+		dentry = dentry->d_parent;
+		depth++;
+	}
+	return depth;
+}
+
+static const struct dentry *d_common_ancestor(const struct dentry *left,
+					      const struct dentry *right)
+{
+	unsigned ldepth = d_depth(left);
+	unsigned rdepth = d_depth(right);
+
+	if (ldepth > rdepth) {
+		swap(left, right);
+		swap(ldepth, rdepth);
+	}
+
+	while (rdepth > ldepth) {
+		right = right->d_parent;
+		rdepth--;
+	}
+
+	while (right != left) {
+		if (IS_ROOT(right))
+			return NULL;
+		right = right->d_parent;
+		left = left->d_parent;
+	}
+
+	return right;
+}
+
+static void mark_violated_mounts(struct dentry *dentry,
+				 const struct dentry *ancestor)
+{
+	/* Mark all mountroots that are children of the common
+	 * ancestor and ancestors of dentry.
+	 */
+	struct dentry *p;
+	for (p = dentry->d_parent; p != ancestor; p = p->d_parent) {
+		if (d_mountroot(p))
+			mnt_set_violated(p);
+	}
+}
+
 /*
  * When switching names, the actual string doesn't strictly have to
  * be preserved in the target - because we're dropping the target

@@ -2563,11 +2613,22 @@ static void dentry_unlock_for_move(struct dentry *dentry, struct dentry *target)
 static void __d_move(struct dentry *dentry, struct dentry *target,
 		     bool exchange)
 {
+	const struct dentry *ancestor = d_common_ancestor(dentry, target);
+
 	if (!dentry->d_inode)
 		printk(KERN_WARNING "VFS: moving negative dcache entry\n");
 
-	BUG_ON(d_ancestor(dentry, target));
-	BUG_ON(d_ancestor(target, dentry));
+	BUG_ON(dentry == ancestor);
+	BUG_ON(target == ancestor);
+
+	/* If there is a common ancestor, mark mounts which may have
+	 * paths that are no longer able to follow d_parent up to
+	 * mnt_root after this move.
+	 */
+	if (ancestor) {
+		mark_violated_mounts(dentry, ancestor);
+		mark_violated_mounts(target, ancestor);
+	}
 
 	dentry_lock_for_move(dentry, target);
 

index 046767f0042e..d6a6cbd1e7a1 100644
@@ -71,6 +71,7 @@ extern int __mnt_want_write_file(struct file *);
 extern void __mnt_drop_write(struct vfsmount *);
 extern void __mnt_drop_write_file(struct file *);
 
+extern void mnt_set_violated(struct dentry *root);
 /*
  * fs_struct.c
  */

index 75abc9fcaafa..083d96bdbd60 100644
@@ -1644,6 +1644,23 @@ out_unlock:
 	namespace_unlock();
 }
 
+void mnt_set_violated(struct dentry *root)
+{
+	struct mountroot *mr;
+
+	/* Locking the mount guarantees that root is a mountpoint and
+	 * pushes rcu path walkers onto the slow path.
+	 */
+	lock_mount_hash();
+	mr = lookup_mountroot(root);
+	if (mr) {
+		spin_lock(&root->d_lock);
+		root->d_flags |= DCACHE_MOUNT_VIOLATED;
+		spin_unlock(&root->d_lock);
+	}
+	unlock_mount_hash();
+}
+
 /* 
  * Is the caller allowed to modify his namespace?
  */

2.2.1

Well there is the 4K PATH_MAX.

If nothing else your performance will grind to a halt if you attempt to use a path that deeply nested.

That said it doesn't cost us anything to make the variables unsigned long and it avoids having to worry about this piece of code.

I will respin this patch.

Eric

I am pretty certain it can cause d_path to become an information leak if we do not.

If /tmp is a mount point and before the move there was a:
touch /tmp/b/c/x

And a process opened /tmp/c/c/x.
d_path on that file descriptor before __d_move would say:

/tmp/c/c/x

after the __d_move d_path would say:

/tmp/c/a/x

Which is bizareely weird in this example, and could potentially be an expolitable information leak in the hands of someone who knew what they were doing.

I am not clever enough to take that deleted directory and walk up the tree, so the damage may be limited to seeing the true path on the fileystem. But it just may be that I am dense today.

Furthermore all of the relevant changes to the dentry that happen when exchange is true also happen when exchange is false, so I am very reluctant to believe that the non-exchange case is not exploitable by a sufficiently clever individual.

Eric

I am pretty certain it can cause d_path to become an information leak if we do not.

If /tmp is a mount point and before the move there was a:
touch /tmp/b/c/x

And a process opened /tmp/c/c/x.
d_path on that file descriptor before __d_move would say:

/tmp/c/c/x

after the __d_move d_path would say:

/tmp/c/a/x

Which is bizareely weird in this example, and could potentially be an expolitable information leak in the hands of someone who knew what they were doing.

I am not clever enough to take that deleted directory and walk up the tree, so the damage may be limited to seeing the true path on the fileystem. But it just may be that I am dense today.

Furthermore all of the relevant changes to the dentry that happen when exchange is true also happen when exchange is false, so I am very reluctant to believe that the non-exchange case is not exploitable by a sufficiently clever individual.

Eric

So what?

Exploited how? The same assistant might very well have done
echo "/tmp/c/a/x or whatever else I might want to pass to you" >/tmp/c/c/x and pass whatever information they wanted _that_ way.

As it is, you've created one hell of a DoS - *anyone* can poison
any vfsmount covering a subtree if they have access to a containing subtree somewhere and write permissions on a directory inside and directory outside of the victim one.

So what?

Exploited how? The same assistant might very well have done
echo "/tmp/c/a/x or whatever else I might want to pass to you" >/tmp/c/c/x and pass whatever information they wanted _that_ way.

As it is, you've created one hell of a DoS - *anyone* can poison
any vfsmount covering a subtree if they have access to a containing subtree somewhere and write permissions on a directory inside and directory outside of the victim one.

Maybe I'm missing something, but I see a much simpler fix:

- When following ".." first just check against the dentry being equal
to the root dentry.

- If so, then check mount being equal to root mount.

- If so, then we are fine, found the root.

- If mount is not root mount, then we either have a bind mount or the
escape scenario. So have a peek at the mount tree to see if we have a chance of reaching root or not.

- If yes, then we are fine, continue upward.

- Otherwise stop here and act like we found root.

This doesn't have to hook into d_move() and will only trigger the "violated" mode on an very specific and rare case.

I haven't thought about this very hard, but I don't see how the root dentry could be avoided without first having access to something outside the root.

Thanks,
Miklos

In concrete terms I think you are suggesting something like this patch to follow_dot_dot.

index ae4e4c18b2ac..56a8562899a1 100644
@@ -1409,6 +1409,11 @@ static void follow_dotdot(struct nameidata *nd)
                        break;
                }
                if (nd->path.dentry != nd->path.mnt->mnt_root) {
+                       /* Escaped path? */
+                       if ((nd->path.mnt->mnt_root != nd->path.mnt->mnt_sb->s_root) &&
+                           d_ancestor(nd->path.mnt->mnt_root, nd->path.dentry))
+                               break;
+                       }
                        /* rare case of legitimate dget_parent()... */
                        nd->path.dentry = dget_parent(nd->path.dentry);
                        dput(old);

Am I misunderstanding you? I don't think .. on a bind mount is a very specific rare case.

Operations such as following ../../../../../../../../../.. would go from a cost of O(10) to a cost of O((10*(10 + P + 1))/2) aka
from O(N) to O(N^2+N*P). Where P is the depth of the path below 10 directories up.

Given that in cases like containers bind mounts are frequently the root mount point of a filesystem I don't think we want that expense, if we can possibly avoid it. As that is a DOS attack and messes up performance for cases that are not afflicected with an escape.

Eric

In concrete terms I think you are suggesting something like this patch to follow_dot_dot.

index ae4e4c18b2ac..56a8562899a1 100644
@@ -1409,6 +1409,11 @@ static void follow_dotdot(struct nameidata *nd)
                        break;
                }
                if (nd->path.dentry != nd->path.mnt->mnt_root) {
+                       /* Escaped path? */
+                       if ((nd->path.mnt->mnt_root != nd->path.mnt->mnt_sb->s_root) &&
+                           d_ancestor(nd->path.mnt->mnt_root, nd->path.dentry))
+                               break;
+                       }
                        /* rare case of legitimate dget_parent()... */
                        nd->path.dentry = dget_parent(nd->path.dentry);
                        dput(old);

Am I misunderstanding you? I don't think .. on a bind mount is a very specific rare case.

Operations such as following ../../../../../../../../../.. would go from a cost of O(10) to a cost of O((10*(10 + P + 1))/2) aka
from O(N) to O(N^2+N*P). Where P is the depth of the path below 10 directories up.

Given that in cases like containers bind mounts are frequently the root mount point of a filesystem I don't think we want that expense, if we can possibly avoid it. As that is a DOS attack and messes up performance for cases that are not afflicected with an escape.

Eric

Linus,

Please pull the for-linus branch from the git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-linus

HEAD: e0c9c0afd2fc958ffa34b697972721d81df8a56f mnt: Update detach_mounts to leave mounts connected

Way back in October Andrey Vagin reported that umount(MNT_DETACH) could be used to defeat MNT_LOCKED. As I worked to fix this I discovered that combined with mount propagation and an appropriate selection of shared subtrees a reference to a directory on an unmounted filesystem is not necessary.

That MNT_DETACH is allowed in user namespace in a form that can break MNT_LOCKED comes from my early misunderstanding what MNT_DETACH does.

To avoid breaking existing userspace the conflict between MNT_DETACH and MNT_LOCKED is fixed by leaving mounts that are locked to their parents in the mount hash table until the last reference goes away.

While investigating this issue I also found an issue with __detach_mounts. The code was unnecessarily and incorrectly triggering mount propagation. Resulting in too many mounts going away when a directory is deleted, and too many cpu cycles are burned while doing that.

Looking some more I realized that __detach_mounts by only keeping mounts connected that were MNT_LOCKED it had the potential to still leak information so I tweaked the code to keep everything locked together that possibly could be.

This code was almost ready last cycle but Al invented fs_pin which slightly simplifies this code but required rewrites and retesting, and I have not been in top form for a while so it took me a while to get all of that done. Similiarly this pull request is late because I have been feeling absolutely miserable all week.

The issue of being able to escape a bind mount has not yet been addressed, as the fixes are not yet mature.

Eric W. Biederman (15):
mnt: Use hlist_move_list in namespace_unlock
mnt: Improve the umount_tree flags
mnt: Don't propagate umounts in __detach_mounts
mnt: In umount_tree reuse mnt_list instead of mnt_hash
mnt: Add MNT_UMOUNT flag
mnt: Delay removal from the mount hash.
mnt: On an unmount propagate clearing of MNT_LOCKED
mnt: Don't propagate unmounts to locked mounts
mnt: Fail collect_mounts when applied to unmounted mounts
mnt: Factor out unhash_mnt from detach_mnt and umount_tree
mnt: Factor umount_mnt from umount_tree
fs_pin: Allow for the possibility that m_list or s_list go unused.
mnt: Honor MNT_LOCKED when detaching mounts
mnt: Fix the error check in __detach_mounts
mnt: Update detach_mounts to leave mounts connected

 fs/fs_pin.c            |   4 +-
 fs/namespace.c         | 142 +++++++++++++++++++++++++++++++++----------------
 fs/pnode.c             |  60 ++++++++++++++++++---
 fs/pnode.h             |   7 ++-
 include/linux/fs_pin.h |   2 +
 include/linux/mount.h  |   1 +
 6 files changed, 159 insertions(+), 57 deletions(-)

Linus,

Please pull the for-linus branch from the git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-linus

HEAD: e0c9c0afd2fc958ffa34b697972721d81df8a56f mnt: Update detach_mounts to leave mounts connected

Way back in October Andrey Vagin reported that umount(MNT_DETACH) could be used to defeat MNT_LOCKED. As I worked to fix this I discovered that combined with mount propagation and an appropriate selection of shared subtrees a reference to a directory on an unmounted filesystem is not necessary.

That MNT_DETACH is allowed in user namespace in a form that can break MNT_LOCKED comes from my early misunderstanding what MNT_DETACH does.

To avoid breaking existing userspace the conflict between MNT_DETACH and MNT_LOCKED is fixed by leaving mounts that are locked to their parents in the mount hash table until the last reference goes away.

While investigating this issue I also found an issue with __detach_mounts. The code was unnecessarily and incorrectly triggering mount propagation. Resulting in too many mounts going away when a directory is deleted, and too many cpu cycles are burned while doing that.

Looking some more I realized that __detach_mounts by only keeping mounts connected that were MNT_LOCKED it had the potential to still leak information so I tweaked the code to keep everything locked together that possibly could be.

This code was almost ready last cycle but Al invented fs_pin which slightly simplifies this code but required rewrites and retesting, and I have not been in top form for a while so it took me a while to get all of that done. Similiarly this pull request is late because I have been feeling absolutely miserable all week.

The issue of being able to escape a bind mount has not yet been addressed, as the fixes are not yet mature.

Eric W. Biederman (15):
mnt: Use hlist_move_list in namespace_unlock
mnt: Improve the umount_tree flags
mnt: Don't propagate umounts in __detach_mounts
mnt: In umount_tree reuse mnt_list instead of mnt_hash
mnt: Add MNT_UMOUNT flag
mnt: Delay removal from the mount hash.
mnt: On an unmount propagate clearing of MNT_LOCKED
mnt: Don't propagate unmounts to locked mounts
mnt: Fail collect_mounts when applied to unmounted mounts
mnt: Factor out unhash_mnt from detach_mnt and umount_tree
mnt: Factor umount_mnt from umount_tree
fs_pin: Allow for the possibility that m_list or s_list go unused.
mnt: Honor MNT_LOCKED when detaching mounts
mnt: Fix the error check in __detach_mounts
mnt: Update detach_mounts to leave mounts connected

 fs/fs_pin.c            |   4 +-
 fs/namespace.c         | 142 +++++++++++++++++++++++++++++++++----------------
 fs/pnode.c             |  60 ++++++++++++++++++---
 fs/pnode.h             |   7 ++-
 include/linux/fs_pin.h |   2 +
 include/linux/mount.h  |   1 +
 6 files changed, 159 insertions(+), 57 deletions(-)

p.s. My apologies for everyone who is seeing this twice I failed to send this to Linus...

Linus,

Please pull the for-linus branch from the git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-linus

HEAD: e0c9c0afd2fc958ffa34b697972721d81df8a56f mnt: Update detach_mounts to leave mounts connected

Way back in October Andrey Vagin reported that umount(MNT_DETACH) could be used to defeat MNT_LOCKED. As I worked to fix this I discovered that combined with mount propagation and an appropriate selection of shared subtrees a reference to a directory on an unmounted filesystem is not necessary.

That MNT_DETACH is allowed in user namespace in a form that can break MNT_LOCKED comes from my early misunderstanding what MNT_DETACH does.

To avoid breaking existing userspace the conflict between MNT_DETACH and MNT_LOCKED is fixed by leaving mounts that are locked to their parents in the mount hash table until the last reference goes away.

While investigating this issue I also found an issue with __detach_mounts. The code was unnecessarily and incorrectly triggering mount propagation. Resulting in too many mounts going away when a directory is deleted, and too many cpu cycles are burned while doing that.

Looking some more I realized that __detach_mounts by only keeping mounts connected that were MNT_LOCKED it had the potential to still leak information so I tweaked the code to keep everything locked together that possibly could be.

This code was almost ready last cycle but Al invented fs_pin which slightly simplifies this code but required rewrites and retesting, and I have not been in top form for a while so it took me a while to get all of that done. Similiarly this pull request is late because I have been feeling absolutely miserable all week.

The issue of being able to escape a bind mount has not yet been addressed, as the fixes are not yet mature.

Eric W. Biederman (15):
mnt: Use hlist_move_list in namespace_unlock
mnt: Improve the umount_tree flags
mnt: Don't propagate umounts in __detach_mounts
mnt: In umount_tree reuse mnt_list instead of mnt_hash
mnt: Add MNT_UMOUNT flag
mnt: Delay removal from the mount hash.
mnt: On an unmount propagate clearing of MNT_LOCKED
mnt: Don't propagate unmounts to locked mounts
mnt: Fail collect_mounts when applied to unmounted mounts
mnt: Factor out unhash_mnt from detach_mnt and umount_tree
mnt: Factor umount_mnt from umount_tree
fs_pin: Allow for the possibility that m_list or s_list go unused.
mnt: Honor MNT_LOCKED when detaching mounts
mnt: Fix the error check in __detach_mounts
mnt: Update detach_mounts to leave mounts connected

 fs/fs_pin.c            |   4 +-
 fs/namespace.c         | 142 +++++++++++++++++++++++++++++++++----------------
 fs/pnode.c             |  60 ++++++++++++++++++---
 fs/pnode.h             |   7 ++-
 include/linux/fs_pin.h |   2 +
 include/linux/mount.h  |   1 +
 6 files changed, 159 insertions(+), 57 deletions(-)

p.s. My apologies for everyone who is seeing this twice I failed to send this to Linus...

Invoking mount propagation from __detach_mounts is inefficient and wrong.

It is inefficient because __detach_mounts already walks the list of mounts that where something needs to be done, and mount propagation walks some subset of those mounts again.

It is actively wrong because if the dentry that is passed to __detach_mounts is not part of the path to a mount that mount should not be affected.

change_mnt_propagation(p,MS_PRIVATE) modifies the mount propagation tree of a master mount so it's slaves are connected to another master if possible. Which means even removing a mount from the middle of a mount tree with __detach_mounts will not deprive any mount propagated mount events.

---
 fs/namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5bb96c440b31..07d0562290a5 100644
@@ -1497,7 +1497,7 @@ void __detach_mounts(struct dentry *dentry)
 	lock_mount_hash();
 	while (!hlist_empty(&mp->m_list)) {
 		mnt = hlist_entry(mp->m_list.first, struct mount, mnt_mp_list);
-		umount_tree(mnt, UMOUNT_PROPAGATE);
+		umount_tree(mnt, 0);
 	}
 	unlock_mount_hash();
 	put_mountpoint(mp);

2.2.1

umount_tree builds a list of mounts that need to be unmounted. Utilize mnt_list for this purpose instead of mnt_hash as mnt_list is an ordianry list_head, allowing the use of list_splice and list_move instead of rolling our own.

This also begins to allow keeping a mount on the mnt_hash after it is unmounted. Which is necessary for a properly functioning MNT_LOCKED implementation.

---
 fs/namespace.c | 45 +++++++++++++++++++--------------------------
 fs/pnode.c     |  6 +++---
 fs/pnode.h     |  2 +-
 3 files changed, 23 insertions(+), 30 deletions(-)

index 07d0562290a5..44478b6e3719 100644
@@ -1285,23 +1285,22 @@ int may_umount(struct vfsmount *mnt)
 
 EXPORT_SYMBOL(may_umount);
 
-static HLIST_HEAD(unmounted);	/* protected by namespace_sem */
+static LIST_HEAD(unmounted);	/* protected by namespace_sem */
 
 static void namespace_unlock(void)
 {
 	struct mount *mnt;
-	struct hlist_head head = unmounted;
+	LIST_HEAD(head);
 
-	if (likely(hlist_empty(&head))) {
+	if (likely(list_empty(&unmounted))) {
 		up_write(&namespace_sem);
 		return;
 	}
 
-	head.first->pprev = &head.first;
-	INIT_HLIST_HEAD(&unmounted);
+	list_splice_init(&unmounted, &head);
 
 	/* undo decrements we'd done in umount_tree() */
-	hlist_for_each_entry(mnt, &head, mnt_hash)
+	list_for_each_entry(mnt, &head, mnt_list)
 		if (mnt->mnt_ex_mountpoint.mnt)
 			mntget(mnt->mnt_ex_mountpoint.mnt);
 

@@ -1309,9 +1308,9 @@ static void namespace_unlock(void)
 
 	synchronize_rcu();
 
-	while (!hlist_empty(&head)) {
-		mnt = hlist_entry(head.first, struct mount, mnt_hash);
-		hlist_del_init(&mnt->mnt_hash);
+	while (!list_empty(&head)) {
+		mnt = list_first_entry(&head, struct mount, mnt_list);
+		list_del_init(&mnt->mnt_list);
 		if (mnt->mnt_ex_mountpoint.mnt)
 			path_put(&mnt->mnt_ex_mountpoint);
 		mntput(&mnt->mnt);

@@ -1333,24 +1332,25 @@ enum umount_tree_flags {
  */
 static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 {
-	HLIST_HEAD(tmp_list);
+	LIST_HEAD(tmp_list);
 	struct mount *p;
-	struct mount *last = NULL;
 
-	for (p = mnt; p; p = next_mnt(p, mnt)) {
-		hlist_del_init_rcu(&p->mnt_hash);
-		hlist_add_head(&p->mnt_hash, &tmp_list);
-	}
+	/* Gather the mounts to umount */
+	for (p = mnt; p; p = next_mnt(p, mnt))
+		list_move(&p->mnt_list, &tmp_list);
 
-	hlist_for_each_entry(p, &tmp_list, mnt_hash)
+	/* Hide the mounts from lookup_mnt and mnt_mounts */
+	list_for_each_entry(p, &tmp_list, mnt_list) {
+		hlist_del_init_rcu(&p->mnt_hash);
 		list_del_init(&p->mnt_child);
+	}
 
+	/* Add propogated mounts to the tmp_list */
 	if (how & UMOUNT_PROPAGATE)
 		propagate_umount(&tmp_list);
 
-	hlist_for_each_entry(p, &tmp_list, mnt_hash) {
+	list_for_each_entry(p, &tmp_list, mnt_list) {
 		list_del_init(&p->mnt_expire);
-		list_del_init(&p->mnt_list);
 		__touch_mnt_namespace(p->mnt_ns);
 		p->mnt_ns = NULL;
 		if (how & UMOUNT_SYNC)

@@ -1367,15 +1367,8 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 			p->mnt_mp = NULL;
 		}
 		change_mnt_propagation(p, MS_PRIVATE);
-		last = p;
-	}
-	if (last) {
-		last->mnt_hash.next = unmounted.first;
-		if (unmounted.first)
-			unmounted.first->pprev = &last->mnt_hash.next;
-		unmounted.first = tmp_list.first;
-		unmounted.first->pprev = &unmounted.first;
 	}
+	list_splice(&tmp_list, &unmounted);
 }
 
 static void shrink_submounts(struct mount *mnt);

index 260ac8f898a4..bf012af709dd 100644
@@ -384,7 +384,7 @@ static void __propagate_umount(struct mount *mnt)
 		if (child && list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
 			hlist_del_init_rcu(&child->mnt_hash);
-			hlist_add_before_rcu(&child->mnt_hash, &mnt->mnt_hash);
+			list_move_tail(&child->mnt_list, &mnt->mnt_list);
 		}
 	}
 }

@@ -396,11 +396,11 @@ static void __propagate_umount(struct mount *mnt)
  *
  * vfsmount lock must be held for write
  */
-int propagate_umount(struct hlist_head *list)
+int propagate_umount(struct list_head *list)
 {
 	struct mount *mnt;
 
-	hlist_for_each_entry(mnt, list, mnt_hash)
+	list_for_each_entry(mnt, list, mnt_list)
 		__propagate_umount(mnt);
 	return 0;
 }

index 16afc3d6d2f2..aa6d65df7204 100644
@@ -40,7 +40,7 @@ static inline void set_mnt_shared(struct mount *mnt)
 void change_mnt_propagation(struct mount *, int);
 int propagate_mnt(struct mount *, struct mountpoint *, struct mount *,
 		struct hlist_head *);
-int propagate_umount(struct hlist_head *);
+int propagate_umount(struct list_head *);
 int propagate_mount_busy(struct mount *, int);
 void mnt_release_group_id(struct mount *);
 int get_dominating_id(struct mount *mnt, const struct path *root);

2.2.1

In some instances it is necessary to know if the the unmounting process has begun on a mount. Add MNT_UMOUNT to make that reliably testable.

This fix gets used in fixing locked mounts in MNT_DETACH

---
 fs/namespace.c        | 4 +++-
 fs/pnode.c            | 1 +
 include/linux/mount.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

index 44478b6e3719..60d4160cd2f4 100644
@@ -1336,8 +1336,10 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 	struct mount *p;
 
 	/* Gather the mounts to umount */
-	for (p = mnt; p; p = next_mnt(p, mnt))
+	for (p = mnt; p; p = next_mnt(p, mnt)) {
+		p->mnt.mnt_flags |= MNT_UMOUNT;
 		list_move(&p->mnt_list, &tmp_list);
+	}
 
 	/* Hide the mounts from lookup_mnt and mnt_mounts */
 	list_for_each_entry(p, &tmp_list, mnt_list) {

index bf012af709dd..ac3aa0d43b90 100644
@@ -384,6 +384,7 @@ static void __propagate_umount(struct mount *mnt)
 		if (child && list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
 			hlist_del_init_rcu(&child->mnt_hash);
+			child->mnt.mnt_flags |= MNT_UMOUNT;
 			list_move_tail(&child->mnt_list, &mnt->mnt_list);
 		}
 	}

index c2c561dc0114..564beeec5d83 100644
@@ -61,6 +61,7 @@ struct mnt_namespace;
 #define MNT_DOOMED		0x1000000
 #define MNT_SYNC_UMOUNT		0x2000000
 #define MNT_MARKED		0x4000000
+#define MNT_UMOUNT		0x8000000
 
 struct vfsmount {
 	struct dentry *mnt_root;	/* root of the mounted tree */

2.2.1

- Modify __lookup_mnt_hash_last to ignore mounts that have MNT_UMOUNTED set.
- Don't remove mounts from the mount hash table in propogate_umount
- Don't remove mounts from the mount hash table in umount_tree before the entire list of mounts to be umounted is selected.
- Remove mounts from the mount hash table as the last thing that happens in the case where a mount has a parent in umount_tree. Mounts without parents are not hashed (by definition).

This paves the way for delaying removal from the mount hash table even farther and fixing the MNT_LOCKED vs MNT_DETACH issue.

---
 fs/namespace.c | 13 ++++++++-----
 fs/pnode.c     |  1 -
 2 files changed, 8 insertions(+), 6 deletions(-)

index 60d4160cd2f4..a8afec9c81b6 100644
@@ -623,14 +623,17 @@ struct mount *__lookup_mnt(struct vfsmount *mnt, struct dentry *dentry)
  */
 struct mount *__lookup_mnt_last(struct vfsmount *mnt, struct dentry *dentry)
 {
-	struct mount *p, *res;
-	res = p = __lookup_mnt(mnt, dentry);
+	struct mount *p, *res = NULL;
+	p = __lookup_mnt(mnt, dentry);
 	if (!p)
 		goto out;
+	if (!(p->mnt.mnt_flags & MNT_UMOUNT))
+		res = p;
 	hlist_for_each_entry_continue(p, mnt_hash) {
 		if (&p->mnt_parent->mnt != mnt || p->mnt_mountpoint != dentry)
 			break;
-		res = p;
+		if (!(p->mnt.mnt_flags & MNT_UMOUNT))
+			res = p;
 	}
 out:
 	return res;

@@ -1341,9 +1344,8 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 		list_move(&p->mnt_list, &tmp_list);
 	}
 
-	/* Hide the mounts from lookup_mnt and mnt_mounts */
+	/* Hide the mounts from mnt_mounts */
 	list_for_each_entry(p, &tmp_list, mnt_list) {
-		hlist_del_init_rcu(&p->mnt_hash);
 		list_del_init(&p->mnt_child);
 	}
 

@@ -1367,6 +1369,7 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 			p->mnt_mountpoint = p->mnt.mnt_root;
 			p->mnt_parent = p;
 			p->mnt_mp = NULL;
+			hlist_del_init_rcu(&p->mnt_hash);
 		}
 		change_mnt_propagation(p, MS_PRIVATE);
 	}

index ac3aa0d43b90..c27ae38ee250 100644
@@ -383,7 +383,6 @@ static void __propagate_umount(struct mount *mnt)
 		 */
 		if (child && list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
-			hlist_del_init_rcu(&child->mnt_hash);
 			child->mnt.mnt_flags |= MNT_UMOUNT;
 			list_move_tail(&child->mnt_list, &mnt->mnt_list);
 		}

2.2.1

An upcoming change is going to need a version of detach_mnt that leaves the mount on the parents mnt_mounts list. Create that version of detach_mnt now and call it __detach_mnt.

---
 fs/namespace.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index a8afec9c81b6..c3f526ce0522 100644
@@ -789,13 +789,12 @@ static void __touch_mnt_namespace(struct mnt_namespace *ns)
 /*
  * vfsmount lock must be held for write
  */
-static void detach_mnt(struct mount *mnt, struct path *old_path)
+static void __detach_mnt(struct mount *mnt, struct path *old_path)
 {
 	old_path->dentry = mnt->mnt_mountpoint;
 	old_path->mnt = &mnt->mnt_parent->mnt;
 	mnt->mnt_parent = mnt;
 	mnt->mnt_mountpoint = mnt->mnt.mnt_root;
-	list_del_init(&mnt->mnt_child);
 	hlist_del_init_rcu(&mnt->mnt_hash);
 	hlist_del_init(&mnt->mnt_mp_list);
 	put_mountpoint(mnt->mnt_mp);

@@ -805,6 +804,15 @@ static void detach_mnt(struct mount *mnt, struct path *old_path)
 /*
  * vfsmount lock must be held for write
  */
+static void detach_mnt(struct mount *mnt, struct path *old_path)
+{
+	__detach_mnt(mnt, old_path);
+	list_del_init(&mnt->mnt_child);
+}
+
+/*
+ * vfsmount lock must be held for write
+ */
 void mnt_set_mountpoint(struct mount *mnt,
 			struct mountpoint *mp,
 			struct mount *child_mnt)

2.2.1

Replace the open coded __detach_mnt with __detach_mnt.

---
 fs/namespace.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

index c3f526ce0522..9fae55f2242e 100644
@@ -1368,16 +1368,8 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 		if (how & UMOUNT_SYNC)
 			p->mnt.mnt_flags |= MNT_SYNC_UMOUNT;
 		if (mnt_has_parent(p)) {
-			hlist_del_init(&p->mnt_mp_list);
-			put_mountpoint(p->mnt_mp);
 			mnt_add_count(p->mnt_parent, -1);
-			/* move the reference to mountpoint into ->mnt_ex_mountpoint */
-			p->mnt_ex_mountpoint.dentry = p->mnt_mountpoint;
-			p->mnt_ex_mountpoint.mnt = &p->mnt_parent->mnt;
-			p->mnt_mountpoint = p->mnt.mnt_root;
-			p->mnt_parent = p;
-			p->mnt_mp = NULL;
-			hlist_del_init_rcu(&p->mnt_hash);
+			__detach_mnt(p, &p->mnt_ex_mountpoint);
 		}
 		change_mnt_propagation(p, MS_PRIVATE);
 	}

2.2.1

mntget, mntput, dput and pathput all test their arguments to see if they are NULL before taking any action, so testing for NULL in namespace_unlock is redundant.

Remove the redundant checks making namespace_unlock a little shorter and easier to read.

This also makes it possible for mnt_ex_mountpoint.mnt to be NULL allowing putting a dentry without a mount. This is will be needed in __detach_mounts when detaching already unmounted children, as part of the fix for MNT_DETACH on MNT_LOCKED mounts.

---
 fs/namespace.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 9fae55f2242e..3769dbd040c1 100644
@@ -1312,8 +1312,7 @@ static void namespace_unlock(void)
 
 	/* undo decrements we'd done in umount_tree() */
 	list_for_each_entry(mnt, &head, mnt_list)
-		if (mnt->mnt_ex_mountpoint.mnt)
-			mntget(mnt->mnt_ex_mountpoint.mnt);
+		mntget(mnt->mnt_ex_mountpoint.mnt);
 
 	up_write(&namespace_sem);
 

@@ -1322,8 +1321,7 @@ static void namespace_unlock(void)
 	while (!list_empty(&head)) {
 		mnt = list_first_entry(&head, struct mount, mnt_list);
 		list_del_init(&mnt->mnt_list);
-		if (mnt->mnt_ex_mountpoint.mnt)
-			path_put(&mnt->mnt_ex_mountpoint);
+		path_put(&mnt->mnt_ex_mountpoint);
 		mntput(&mnt->mnt);
 	}
 }

2.2.1

Modify umount(MNT_DETACH) to keep mounts in the hash table that are locked to their parent mounts, when the parent is lazily unmounted. In doing this invert the reference count so that the parent holds a reference to the children instead of the children holding a reference to the parent.

Then in mntput_no_expire detach the children and in cleanup_mnt mntput the children and dput the dentry they were mounted on.

In __detach_mounts if there are any mounts that have been unmounted but still are on the list of mounts of a mountpoint, detach those mounts and schedule them to be mntput and their reference to the dentry to be put when it becomes safe to sleep.

---
 fs/namespace.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

index 3769dbd040c1..5373343da715 100644
@@ -1017,6 +1017,17 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	return ERR_PTR(err);
 }
 
+static void mntput_children(struct mount *mnt)
+{
+	struct mount *p, *tmp;
+
+	list_for_each_entry_safe(p, tmp, &mnt->mnt_mounts, mnt_child) {
+		list_del_init(&p->mnt_child);
+		path_put(&p->mnt_ex_mountpoint);
+		mntput(&p->mnt);
+	}
+}
+
 static void cleanup_mnt(struct mount *mnt)
 {
 	/*

@@ -1030,6 +1041,8 @@ static void cleanup_mnt(struct mount *mnt)
 	 * so mnt_get_writers() below is safe.
 	 */
 	WARN_ON(mnt_get_writers(mnt));
+	if (unlikely(!list_empty(&mnt->mnt_mounts)))
+		mntput_children(mnt);
 	if (unlikely(mnt->mnt_pins.first))
 		mnt_pin_kill(mnt);
 	fsnotify_vfsmount_delete(&mnt->mnt);

@@ -1080,6 +1093,15 @@ static void mntput_no_expire(struct mount *mnt)
 	rcu_read_unlock();
 
 	list_del(&mnt->mnt_instance);
+
+	if (unlikely(!list_empty(&mnt->mnt_mounts))) {
+		struct mount *p;
+		list_for_each_entry(p, &mnt->mnt_mounts,  mnt_child) {
+			__detach_mnt(p, &p->mnt_ex_mountpoint);
+			/* No need to mntput mnt */
+			p->mnt_ex_mountpoint.mnt = NULL;
+		}
+	}
 	unlock_mount_hash();
 
 	if (likely(!(mnt->mnt.mnt_flags & MNT_INTERNAL))) {

@@ -1342,7 +1364,7 @@ enum umount_tree_flags {
 static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 {
 	LIST_HEAD(tmp_list);
-	struct mount *p;
+	struct mount *tmp, *p;
 
 	/* Gather the mounts to umount */
 	for (p = mnt; p; p = next_mnt(p, mnt)) {

@@ -1359,7 +1381,7 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 	if (how & UMOUNT_PROPAGATE)
 		propagate_umount(&tmp_list);
 
-	list_for_each_entry(p, &tmp_list, mnt_list) {
+	list_for_each_entry_safe(p, tmp, &tmp_list, mnt_list) {
 		list_del_init(&p->mnt_expire);
 		__touch_mnt_namespace(p->mnt_ns);
 		p->mnt_ns = NULL;

@@ -1367,7 +1389,15 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 			p->mnt.mnt_flags |= MNT_SYNC_UMOUNT;
 		if (mnt_has_parent(p)) {
 			mnt_add_count(p->mnt_parent, -1);
-			__detach_mnt(p, &p->mnt_ex_mountpoint);
+			if ((p->mnt_parent->mnt.mnt_flags & MNT_UMOUNT) &&
+			    ((p->mnt.mnt_flags & (MNT_LOCKED|MNT_SYNC_UMOUNT)) == MNT_LOCKED)) {
+				/* Don't mntput p in namespace_unlock */
+				list_del_init(&p->mnt_list);
+				/* Don't forget about p */
+				list_add_tail(&p->mnt_child, &p->mnt_parent->mnt_mounts);
+			} else {
+				__detach_mnt(p, &p->mnt_ex_mountpoint);
+			}
 		}
 		change_mnt_propagation(p, MS_PRIVATE);
 	}

@@ -1493,7 +1523,16 @@ void __detach_mounts(struct dentry *dentry)
 	lock_mount_hash();
 	while (!hlist_empty(&mp->m_list)) {
 		mnt = hlist_entry(mp->m_list.first, struct mount, mnt_mp_list);
-		umount_tree(mnt, 0);
+		if (mnt->mnt.mnt_flags & MNT_UMOUNT) {
+			struct mount *p, *tmp;
+			list_for_each_entry_safe(p, tmp, &mnt->mnt_mounts,  mnt_child) {
+				detach_mnt(p, &p->mnt_ex_mountpoint);
+				/* p->mnt_parent has already been mntput */
+				p->mnt_ex_mountpoint.mnt = NULL;
+				list_add_tail(&p->mnt_list, &unmounted);
+			}
+		}
+		else umount_tree(mnt, 0);
 	}
 	unlock_mount_hash();
 	put_mountpoint(mp);

2.2.1

Arr. This isn't quite enough. It does not properly handle mount propagation MNT_DETACH and MNT_LOCKED.

I am not even certain MNT_DETACH needs to be involved. Back to banging my head against this one. Sigh.

Eric