Fix a race in put_mountpoint.

This can cause a panic when simultaneous callers of put_mountpoint attempt to free the same mountpoint. This occurs because some callers hold the mount_hash_lock, while others hold the namespace lock. Some even hold both.

In this submitter's case, the panic manifested itself as a GP fault in put_mountpoint() when it called hlist_del() and attempted to dereference a m_hash.pprev that had been poisioned by another thread.

Instead of trying to force all mountpoint hash users back under the namespace lock, add locks that cover just the mountpoint hash. This uses hlist_bl to protect against simlultaneous additions and removals, and RCU for lookups.

Quoted 5 lines · Krister Johansen

This can cause a panic when simultaneous callers of put_mountpoint attempt to free the same mountpoint. This occurs because some callers hold the mount_hash_lock, while others hold the namespace lock. Some even hold both.

Quoted 8 lines

In this submitter's case, the panic manifested itself as a GP fault in put_mountpoint() when it called hlist_del() and attempted to dereference a m_hash.pprev that had been poisioned by another thread.

Instead of trying to force all mountpoint hash users back under the namespace lock, add locks that cover just the mountpoint hash. This uses hlist_bl to protect against simlultaneous additions and removals, and RCU for lookups.

Too complicated, IMO. Look at the call graph for that sucker:
put_mountpoint
<- unhash_mnt
<- detach_mnt
<- attach_recursive_mnt [under mount_lock] <- pivot_root [under mount_lock]
<- umount_mnt
<- mntput_no_expire [under mount_lock]
<- umount_tree
<- do_umount [under mount_lock]
<- __detach_mounts [under mount_lock] <- copy_tree [under mount_lock]
<- drop_collected_mounts [under mount_lock] <- attach_recursive_mnt [under mount_lock] <- do_loopback [under mount_lock] <- mark_mounts_for_expiry [under mount_lock] <- shrink_submounts
<- do_umount [under mount_lock]
<- __detach_mounts [under mount_lock]
<- __detach_mounts [right after dropping mount_lock]
<- unlock_mount
<- pivot_root
Now, __detach_mounts() thing is trivially fixed - we just move that call one line up. unhash_mnt() is all covered. Which leaves us with unlock_mount() and pivot_root() and both are _not_ hot paths - not by any stretch of imagination. We also have lookup_mountpoint(), which is not hot either. Note that hash insertions and lookups are serialized on namespace lock; it's only removal from final mntput() that can be triggered outside. So playing with bitlocks is absolutely pointless.

Let's do this: make sure that all callers of lookup_mountpoint() have lock_mount (at least read_seqlock_excl), pull the call of put_mountpoint() in __detach_mounts() under mount_lock and slap read_seqlock_excl() around the calls in unlock_mount() and pivot_root(). Note that read_seqlock_excl() *is* exclusive - the "read" part in it is about "don't bump the seqcount part of mount_lock". I.e. the patch below ought to fix that and it's much simpler than your variant. Do you see any holes in the above?

fs/namespace.c +10 -3

index b5b1259e064f..ca98a8ff2732 100644
@@ -1595,11 +1595,11 @@ void __detach_mounts(struct dentry *dentry)
 	struct mount *mnt;
 
 	namespace_lock();
+	lock_mount_hash();
 	mp = lookup_mountpoint(dentry);
 	if (IS_ERR_OR_NULL(mp))
 		goto out_unlock;
 
-	lock_mount_hash();
 	event++;
 	while (!hlist_empty(&mp->m_list)) {
 		mnt = hlist_entry(mp->m_list.first, struct mount, mnt_mp_list);

@@ -1609,9 +1609,9 @@ void __detach_mounts(struct dentry *dentry)
 		}
 		else umount_tree(mnt, UMOUNT_CONNECTED);
 	}
-	unlock_mount_hash();
 	put_mountpoint(mp);
 out_unlock:
+	unlock_mount_hash();
 	namespace_unlock();
 }
 

@@ -2038,7 +2038,10 @@ static struct mountpoint *lock_mount(struct path *path)
 	namespace_lock();
 	mnt = lookup_mnt(path);
 	if (likely(!mnt)) {
-		struct mountpoint *mp = lookup_mountpoint(dentry);
+		struct mountpoint *mp;
+		read_seqlock_excl(&mount_lock);
+		mp = lookup_mountpoint(dentry);
+		read_sequnlock_excl(&mount_lock);
 		if (!mp)
 			mp = new_mountpoint(dentry);
 		if (IS_ERR(mp)) {

@@ -2059,7 +2062,9 @@ static struct mountpoint *lock_mount(struct path *path)
 static void unlock_mount(struct mountpoint *where)
 {
 	struct dentry *dentry = where->m_dentry;
+	read_seqlock_excl(&mount_lock);
 	put_mountpoint(where);
+	read_sequnlock_excl(&mount_lock);
 	namespace_unlock();
 	inode_unlock(dentry->d_inode);
 }

@@ -3137,7 +3142,9 @@ SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
 	list_del_init(&new_mnt->mnt_expire);
 	unlock_mount_hash();
 	chroot_fs_refs(&root, &new);
+	read_seqlock_excl(&mount_lock);
 	put_mountpoint(root_mp);
+	read_sequnlock_excl(&mount_lock);
 	error = 0;
 out4:
 	unlock_mount(old_mp);

The only significant thing I see is that you have not taken the mount_lock on the path where new_mountpoint adds the new struct mountpoint into the mountpoint hash table.

Quoted 3 lines · Eric W. Biederman

The only significant thing I see is that you have not taken the mount_lock on the path where new_mountpoint adds the new struct mountpoint into the mountpoint hash table.

Umm... Point, but I really don't like that bouncing mount_lock up and down there. It's not going to cause any serious overhead, but it just looks ugly... ;-/

Let me think for a while...

Quoted 10 lines · Al Viro

Quoted 3 lines · Eric W. Biederman

The only significant thing I see is that you have not taken the mount_lock on the path where new_mountpoint adds the new struct mountpoint into the mountpoint hash table.

Umm... Point, but I really don't like that bouncing mount_lock up and down there. It's not going to cause any serious overhead, but it just looks ugly... ;-/

Quoted 1 line

Let me think for a while...

The other possibility is to grab namespace_sem in mntput_no_expire around the call of umount_mnt. That is the only path where put_mountpoint can be called where we are not holding namespace_sem. That works in the small but I haven't traced the callers of mntput and mntput_no_expire yet to see if it works in practice.

Quoted 14 lines · Eric W. Biederman

Quoted 11 lines · Al Viro

Quoted 3 lines · Eric W. Biederman

The only significant thing I see is that you have not taken the mount_lock on the path where new_mountpoint adds the new struct mountpoint into the mountpoint hash table.

Umm... Point, but I really don't like that bouncing mount_lock up and down there. It's not going to cause any serious overhead, but it just looks ugly... ;-/

Let me think for a while...

Quoted 5 lines

The other possibility is to grab namespace_sem in mntput_no_expire around the call of umount_mnt. That is the only path where put_mountpoint can be called where we are not holding namespace_sem. That works in the small but I haven't traced the callers of mntput and mntput_no_expire yet to see if it works in practice.

No, that's a really bad idea. Final mntput should _not_ happen under namespace_lock, but I don't want grabbing it in that place.

How about this instead:

This can cause a panic when simultaneous callers of put_mountpoint attempt to free the same mountpoint. This occurs because some callers hold the mount_hash_lock, while others hold the namespace lock. Some even hold both.

In this submitter's case, the panic manifested itself as a GP fault in put_mountpoint() when it called hlist_del() and attempted to dereference a m_hash.pprev that had been poisioned by another thread.

- attach_shadows to be renamed __attach_mnt and the it's shadow handling to be removed.

v2: Updated to mnt_change_mountpoint to not call dput or mntput and instead to decrement the counts directly. It is guaranteed that there will be other references when mnt_change_mountpoint is called so this is safe.

+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct mount *mnt)
{

fs/namespace.c +8 -0

+	struct mountpoint *old_mp = mnt->mnt_mp;
+	struct dentry *old_mountpoint = mnt->mnt_mountpoint;
+	struct mount *old_parent = mnt->mnt_parent;
+
+	list_del_init(&mnt->mnt_child);
+	hlist_del_init(&mnt->mnt_mp_list);
+	hlist_del_init_rcu(&mnt->mnt_hash);
+

static inline int do_refcount_check(struct mount *mnt, int count) {

fs/pnode.c +3 -0

+	struct mount *topper = __lookup_mnt(&mnt->mnt, mnt->mnt.mnt_root);
+	if (topper)
+		count++;
 	return mnt_get_count(mnt) > count;
 }

fs/pnode.c +2 -2

@@ -420,8 +425,8 @@ static void __propagate_umount(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *topper;
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
 		/*
 		 * umount the child only if the child has no children

@@ -430,6 +435,16 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;

Quoted 2 lines · Eric W. Biederman

- attach_shadows to be renamed __attach_mnt and the it's shadow handling to be removed.

Er... s/the it's/its/, presumably? Or am I misparsing that?

Quoted 7 lines

v2: Updated to mnt_change_mountpoint to not call dput or mntput and instead to decrement the counts directly. It is guaranteed that there will be other references when mnt_change_mountpoint is called so this is safe.

+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct mount *mnt)
{

Too generic name, IMO, and I really wonder if "mount" (== interpose) and "umount" (== excise?) cases would be better off separately.

Quoted 11 lines

fs/namespace.c +11 -0

+	struct mountpoint *old_mp = mnt->mnt_mp;
+	struct dentry *old_mountpoint = mnt->mnt_mountpoint;
+	struct mount *old_parent = mnt->mnt_parent;
+
+	list_del_init(&mnt->mnt_child);
+	hlist_del_init(&mnt->mnt_mp_list);
+	hlist_del_init_rcu(&mnt->mnt_hash);
+
+	attach_mnt(mnt, parent, mp);
+
+	put_mountpoint(old_mp);

fs/namespace.c static void attach_mnt(struct mount *mnt, · line 901

Quoted 6 lines

fs/namespace.c +6 -0

+	 *
+	 * During mounting, another mount will continue to use the old
+	 * mountpoint and during unmounting, the old mountpoint will
+	 * continue to exist until namespace_unlock which happens well
+	 * after mnt_change_mountpoint.
+	 */

Umm... AFAICS, in the former case "another mount" is simply parent, right?

fs/namespace.c static void attach_mnt(struct mount *mnt,

Quoted 7 lines

fs/namespace.c +5 -0

+	spin_lock(&old_mountpoint->d_lock);
+	old_mountpoint->d_lockref.count--;
+	spin_unlock(&old_mountpoint->d_lock);
+	mnt_add_count(old_parent, -1);
 
 
+		if (child->mnt.mnt_root == smp->m_dentry) {

Explain, please. In which case is that condition _not_ satisfied, and what should happen i

fs/namespace.c static int attach_recursive_mnt(struct mount *source_mnt,

Quoted 9 lines

fs/namespace.c +6 -0

+			struct mount *q;
+			q = __lookup_mnt(&child->mnt_parent->mnt,
+					 child->mnt_mountpoint);
+			if (q)
+				mnt_change_mountpoint(child, smp, q);
 
 
 	unlock_mount_hash();
+	put_mountpoint(smp);

Wrong order...

fs/namespace.c static int attach_recursive_mnt(struct mount *source_mnt, · line 2056

Quoted 2 lines

fs/namespace.c +1 -0

 	ns->pending_mounts = 0;
+	put_mountpoint(smp);

... and worse yet here.

Quoted 10 lines

static inline int do_refcount_check(struct mount *mnt, int count) {

fs/pnode.c +3 -0

+	struct mount *topper = __lookup_mnt(&mnt->mnt, mnt->mnt.mnt_root);
+	if (topper)
+		count++;
 	return mnt_get_count(mnt) > count;
 }

@@ -359,7 +362,7 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 

fs/pnode.c int propagate_mount_busy(struct mount *mnt, int refcnt) · line 368

Quoted 7 lines

fs/pnode.c +1 -1

 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
 		if (child && list_empty(&child->mnt_mounts) &&
 		    (ret = do_refcount_check(child, 1)))
 			break;

Er... You do realize that you can end up with more that one such propagation, right? IOW, there might be more than one thing slipped in.

fs/pnode.c static void mark_umount_candidates(struct mount *mnt) · line 410

Quoted 2 lines

fs/pnode.c +1 -0

+		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
 			SET_MNT_MARK(child);

Reread the condition, please... And yes, I realize that original is also rather odd; at a guess it was meant to be !(, not (!, but that's just a guess - it's your code, IIRC.

Quoted 21 lines

fs/pnode.c +8 -2

@@ -420,8 +425,8 @@ static void __propagate_umount(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *topper;
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
 		/*
 		 * umount the child only if the child has no children

@@ -430,6 +435,16 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;
 		CLEAR_MNT_MARK(child);
+
+		/* If there is exactly one mount covering all of child
+		 * replace child with that mount.
+		 */
+		topper = __lookup_mnt(&child->mnt, child->mnt.mnt_root);
+		if (topper &&

fs/pnode.c static void __propagate_umount(struct mount *mnt) · line 445

Quoted 2 lines

fs/pnode.c +2 -0

+		    (child->mnt_mounts.next == &topper->mnt_child) &&
+		    (topper->mnt_child.next == &child->mnt_mounts))

Weird way to spell child->mnt_mounts.next == child->mnt_mounts.prev, that... Or, perhaps, the entire thing ought to be

		if (list_is_singular(&child->mnt_mounts)) {
			topper = list_first_entry(&child->mnt_mounts,
						  struct mount, mnt_child);
			if (topper->mnt_parent == child &&
			    topped->mnt_mountpoint == child->mnt.mnt_root)

to avoid hash lookups.

fs/pnode.c static void __propagate_umount(struct mount *mnt) · line 446

Quoted 1 line

fs/pnode.c +1 -0

+			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp, topper);

FWIW, the my main worry here is your handling of the umount. For example, what happens if
* something is mounted on A (m1)
* something else is mounted on A/bar (m2)
* D is a slave of C
* something has been mounted on D/foo (n)
* you do mount --rbind A C/foo (m1' on C/foo, m2' on m1'/bar, m1'' interposed on D/foo under n, m2'' on m1''/bar,
m1'' slave of m1', m2'' slave of m2)

* you make C/foo and C/foo/bar private (m1'' and m2'' are not getting propagation from m1' and m2' anymore)
* you umount C/foo/bar (m2' is unmounted)
* you umount C/foo
m1' gets unmounted, all right, but what of m1''? D is a slave of C, so we get propagation of umount from C/foo to D/foo; m1'' still has m2'' attached to it. AFAICS, your logics will happily slip m1'' from under n (assuming that n itself is not busy), and leak both m1'' and m2''.

OTOH, the case of multiple slip-under (Z is slave of Y, which is a slave of X, mount on Z, then mount of Y, then mount on X) the check for being busy would do very odd things.

Quoted 4 lines

fs/namespace.c static int attach_recursive_mnt(struct mount *source_mnt, · line 2032

Quoted 1 line

fs/namespace.c +1 -0

+		if (child->mnt.mnt_root == smp->m_dentry) {

Explain, please. In which case is that condition _not_ satisfied, and what should happen i

When a tree is grafted in that condition does not apply to the lower leaves of the tree. At the same time nothing needs to be done for those leaves. Only the primary mountpoint needs to worry about tucking.

Quoted 6 lines

fs/pnode.c static void mark_umount_candidates(struct mount *mnt) · line 410

Quoted 2 lines

fs/pnode.c +1 -0

+		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
 			SET_MNT_MARK(child);

Reread the condition, please... And yes, I realize that original is also rather odd; at a guess it was meant to be !(, not (!, but that's just a guess - it's your code, IIRC.

The intent is to find all trees that we can unmount where the point at which the tree meets the rest of the mounts is not locked.

The mark is later used to see if it ok to unmount a mount or if we will reveal information to userspace (by breaking a lock).

Therefore the mark needs to be set if the mount is unlocked, and recursively the mark needs to be set for every child of that mount where the mark is set (the second condition).

Quoted 10 lines

FWIW, the my main worry here is your handling of the umount. For example, what happens if
* something is mounted on A (m1)
* something else is mounted on A/bar (m2)
* D is a slave of C
* something has been mounted on D/foo (n)
* you do mount --rbind A C/foo (m1' on C/foo, m2' on m1'/bar, m1'' interposed on D/foo under n, m2'' on m1''/bar,
m1'' slave of m1', m2'' slave of m2)

Quoted 8 lines

* you make C/foo and C/foo/bar private (m1'' and m2'' are not getting propagation from m1' and m2' anymore)
* you umount C/foo/bar (m2' is unmounted)
* you umount C/foo
m1' gets unmounted, all right, but what of m1''? D is a slave of C, so we get propagation of umount from C/foo to D/foo; m1'' still has m2'' attached to it. AFAICS, your logics will happily slip m1'' from under n (assuming that n itself is not busy), and leak both m1'' and m2''.

Yes. This is exactly the same behavior we have today without my patch. The only difference is who is the parent mount.

Quoted 5 lines · Eric W. Biederman

Quoted 4 lines

fs/namespace.c static int attach_recursive_mnt(struct mount *source_mnt, · line 2032

Quoted 1 line

fs/namespace.c +1 -0

+		if (child->mnt.mnt_root == smp->m_dentry) {

Explain, please. In which case is that condition _not_ satisfied, and what should happen i

Quoted 3 lines

When a tree is grafted in that condition does not apply to the lower leaves of the tree. At the same time nothing needs to be done for those leaves. Only the primary mountpoint needs to worry about tucking.

How in hell would those lower leaves end up on the list in
attach_recursive_mnt()? IDGI...

Quoted 11 lines

Quoted 7 lines

fs/pnode.c static void mark_umount_candidates(struct mount *mnt) · line 410

Quoted 3 lines

fs/pnode.c +1 -0

+		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
 			SET_MNT_MARK(child);

Reread the condition, please... And yes, I realize that original is also rather odd; at a guess it was meant to be !(, not (!, but that's just a guess - it's your code, IIRC.

The intent is to find all trees that we can unmount where the point at which the tree meets the rest of the mounts is not locked.

Quoted 6 lines

The mark is later used to see if it ok to unmount a mount or if we will reveal information to userspace (by breaking a lock).

Therefore the mark needs to be set if the mount is unlocked, and recursively the mark needs to be set for every child of that mount where the mark is set (the second condition).

*blink*

You have "if mount is not locked - mark it; if mount is already marked - mark it again". The latter part (|| IS_MNT_MARKED(mnt), that is) looks very odd, won't you agree? What the hell was that (its counterpart in the earlier code) about?

I could understand something along the lines "mark it unless it's locked or already marked", but your code is "mark it if it's not locked *or* if it's already marked". Makes no sense in that form.

Quoted 19 lines

Quoted 10 lines

FWIW, the my main worry here is your handling of the umount. For example, what happens if
* something is mounted on A (m1)
* something else is mounted on A/bar (m2)
* D is a slave of C
* something has been mounted on D/foo (n)
* you do mount --rbind A C/foo (m1' on C/foo, m2' on m1'/bar, m1'' interposed on D/foo under n, m2'' on m1''/bar,
m1'' slave of m1', m2'' slave of m2)

Quoted 8 lines

* you make C/foo and C/foo/bar private (m1'' and m2'' are not getting propagation from m1' and m2' anymore)
* you umount C/foo/bar (m2' is unmounted)
* you umount C/foo
m1' gets unmounted, all right, but what of m1''? D is a slave of C, so we get propagation of umount from C/foo to D/foo; m1'' still has m2'' attached to it. AFAICS, your logics will happily slip m1'' from under n (assuming that n itself is not busy), and leak both m1'' and m2''.

Quoted 2 lines

Yes. This is exactly the same behavior we have today without my patch. The only difference is who is the parent mount.

Not quite. In the current tree m1'' should get stuck there (and be exposed when n gets unmounted); AFAICS, your change will have it kicked out, with m2'' still attached and still contributing to refcount of m1''.

I might be missing something (and I hadn't checked your script - right now I'm at 16 hours of uptime after only 4 hours of sleep). Will take a look at that after I grab some sleep...

fs/pnode.c +7 -4

+	    child->mnt_mountpoint != mnt->mnt.mnt_root)
+		return NULL;

@@ -342,7 +358,7 @@ static inline int do_refcount_check(struct mount *mnt, int count)
  */
 int propagate_mount_busy(struct mount *mnt, int refcnt)
 {
-	struct mount *m, *child;
+	struct mount *m, *child, *topper;
 	struct mount *parent = mnt->mnt_parent;
 
 	if (mnt == parent)

@@ -358,11 +374,21 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
 		if (!child)
 			continue;
-		if (!list_empty(&child->mnt_mounts) ||
-		    do_refcount_check(child, 1))
+
+		/* Is there exactly one mount on the child that covers

fs/pnode.c +2 -0

@@ -431,6 +459,15 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;
 		CLEAR_MNT_MARK(child);
+
+		/* If there is exactly one mount covering all of child

fs/namespace.c static int attach_recursive_mnt(struct mount *source_mnt, · line 2038

Quoted 7 lines · Eric W. Biederman

fs/namespace.c +7 -0

+		if (child->mnt.mnt_root == smp->m_dentry) {
+			struct mount *q;
+			q = __lookup_mnt(&child->mnt_parent->mnt,
+					 child->mnt_mountpoint);
+			if (q)
+				mnt_change_mountpoint(child, smp, q);
+		}

This is wrong; condition will be true for *all* mounts seen by that loop.

Feel free to add else WARN_ON(1); to the line above and try to trigger it. You are misinterpreting what propagate_mnt() and commit_tree() are doing - the loop in commit_tree() goes through the submounts and sets ->mnt_ns on those. The of the fields is already set up by that point. For roots of those copies we need to set ->mnt_hash/->mnt_child as well, but for all submounts it's already been done by copy_tree(). Again, commit_tree() is called once per secondary copy of source tree, not once per created mount.

Quoted 6 lines

fs/pnode.c +6 -0

+static struct mount *find_topper(struct mount *mnt)
+{
+	/* If there is exactly one mount covering mnt completely return it. */
+	struct mount *child;
+
+	if (!list_is_singular(&mnt->mnt_mounts))

fs/pnode.c int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp, · line 334

Quoted 4 lines

fs/pnode.c +4 -0

+		return NULL;
+
+	child = list_first_entry(&mnt->mnt_mounts, struct mount, mnt_child);
+	if (child->mnt_parent != mnt ||

The first part can't happen. Turn that into WARN_ON(child->mnt_parent != mnt) if you wish, but that never occurs unless the data structures are corrupted.

Quoted 15 lines

fs/pnode.c +11 -0

+	    child->mnt_mountpoint != mnt->mnt.mnt_root)
+		return NULL;

@@ -431,6 +459,15 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;
 		CLEAR_MNT_MARK(child);
+
+		/* If there is exactly one mount covering all of child
+		 * replace child with that mount.
+		 */
+		topper = find_topper(child);
+		if (topper)
+			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp,
+					      topper);
+

fs/pnode.c static void __propagate_umount(struct mount *mnt) · line 473

Quoted 3 lines

fs/pnode.c +0 -0

 		if (list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
 			child->mnt.mnt_flags |= MNT_UMOUNT;

Umm... With fallthrough from "is completely overmounted" case? And I'm not sure I understand what that list_empty() is doing there after your previous semantics change - how _can_ we reach that point with non-empty ->mnt_mounts now?

Ever since mount propagation was introduced in cases where a mount in propagated to parent mount mountpoint pair that is already in use the code has placed the new mount behind the old mount in the mount hash table.

This implementation detail is problematic as it allows creating arbitrary length mount hash chains.

Furthermore it invalidates the constraint maintained elsewhere in the mount code that a parent mount and a mountpoint pair will have exactly one mount upon them. Making it hard to deal with and to talk about this special case in the mount code.

Modify mount propagation to notice when there is already a mount at the parent mount and mountpoint where a new mount is propagating to and place that preexisting mount on top of the new mount.

Modify unmount propagation to notice when a mount that is being unmounted has another mount on top of it (and no other children), and to replace the unmounted mount with the mount on top of it.

Move the MNT_UMUONT test from __lookup_mnt_last into __propagate_umount as that is the only call of __lookup_mnt_last where MNT_UMOUNT may be set on any mount visible in the mount hash table.

These modifications allow:
- __lookup_mnt_last to be removed.
- attach_shadows to be renamed __attach_mnt and its shadow handling to be removed.
- commit_tree to be simplified
- copy_tree to be simplified

The result is an easier to understand tree of mounts that does not allow creation of arbitrary length hash chains in the mount hash table.

v2: Updated to mnt_change_mountpoint to not call dput or mntput and instead to decrement the counts directly. It is guaranteed that there will be other references when mnt_change_mountpoint is called so this is safe.

v3: Moved put_mountpoint under mount_lock in attach_recursive_mnt
As the locking in fs/namespace.c changed between v2 and v3.

v4: Reworked the logic in propagate_mount_busy and __propagate_umount
that detects when a mount completely covers another mount.

v5: Removed unnecessary tests whose result is always true in
find_topper and attach_recursive_mnt.

Cc [email protected]

Fixes b90fa9ae8f51 ("[PATCH] shared mount handling: bind and rbind")

Tested-by Andrei Vagin <[email protected]>

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

---
 fs/mount.h     |   1 -
 fs/namespace.c | 110 +++++++++++++++++++++++++++++++--------------------------
 fs/pnode.c     |  61 +++++++++++++++++++++++++-------
 fs/pnode.h     |   2 ++
 4 files changed, 111 insertions(+), 63 deletions(-)

fs/mount.h +0 -1

index 2c856fc47ae3..2826543a131d 100644
@@ -89,7 +89,6 @@ static inline int is_mounted(struct vfsmount *mnt)
 }
 
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *);
-extern struct mount *__lookup_mnt_last(struct vfsmount *, struct dentry *);
 
 extern int __legitimize_mnt(struct vfsmount *, unsigned);
 extern bool legitimize_mnt(struct vfsmount *, unsigned);

fs/namespace.c +60 -50

index 487ba30bb5c6..8a3b6c1b16ff 100644
@@ -637,28 +637,6 @@ struct mount *__lookup_mnt(struct vfsmount *mnt, struct dentry *dentry)
 }
 
 /*
- * find the last mount at @dentry on vfsmount @mnt.
- * mount_lock must be held.
- */
-struct mount *__lookup_mnt_last(struct vfsmount *mnt, struct dentry *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;
-		if (!(p->mnt.mnt_flags & MNT_UMOUNT))
-			res = p;
-	}
-out:
-	return res;
-}
-
-/*
  * lookup_mnt - Return the first child mount mounted at path
  *
  * "First" means first mounted chronologically.  If you create the

@@ -878,6 +856,13 @@ void mnt_set_mountpoint(struct mount *mnt,
 	hlist_add_head(&child_mnt->mnt_mp_list, &mp->m_list);
 }
 
+static void __attach_mnt(struct mount *mnt, struct mount *parent)
+{
+	hlist_add_head_rcu(&mnt->mnt_hash,
+			   m_hash(&parent->mnt, mnt->mnt_mountpoint));
+	list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
+}
+
 /*
  * vfsmount lock must be held for write
  */

@@ -886,28 +871,45 @@ static void attach_mnt(struct mount *mnt,
 			struct mountpoint *mp)
 {
 	mnt_set_mountpoint(parent, mp, mnt);
-	hlist_add_head_rcu(&mnt->mnt_hash, m_hash(&parent->mnt, mp->m_dentry));
-	list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
+	__attach_mnt(mnt, parent);
 }
 
-static void attach_shadowed(struct mount *mnt,
-			struct mount *parent,
-			struct mount *shadows)
+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct mount *mnt)
 {
-	if (shadows) {
-		hlist_add_behind_rcu(&mnt->mnt_hash, &shadows->mnt_hash);
-		list_add(&mnt->mnt_child, &shadows->mnt_child);
-	} else {
-		hlist_add_head_rcu(&mnt->mnt_hash,
-				m_hash(&parent->mnt, mnt->mnt_mountpoint));
-		list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
-	}
+	struct mountpoint *old_mp = mnt->mnt_mp;
+	struct dentry *old_mountpoint = mnt->mnt_mountpoint;
+	struct mount *old_parent = mnt->mnt_parent;
+
+	list_del_init(&mnt->mnt_child);
+	hlist_del_init(&mnt->mnt_mp_list);
+	hlist_del_init_rcu(&mnt->mnt_hash);
+
+	attach_mnt(mnt, parent, mp);
+
+	put_mountpoint(old_mp);
+
+	/*
+	 * Safely avoid even the suggestion this code might sleep or
+	 * lock the mount hash by taking advantage of the knowledge that
+	 * mnt_change_mountpoint will not release the final reference
+	 * to a mountpoint.
+	 *
+	 * During mounting, the mount passed in as the parent mount will
+	 * continue to use the old mountpoint and during unmounting, the
+	 * old mountpoint will continue to exist until namespace_unlock,
+	 * which happens well after mnt_change_mountpoint.
+	 */
+	spin_lock(&old_mountpoint->d_lock);
+	old_mountpoint->d_lockref.count--;
+	spin_unlock(&old_mountpoint->d_lock);
+
+	mnt_add_count(old_parent, -1);
 }
 
 /*
  * vfsmount lock must be held for write
  */
-static void commit_tree(struct mount *mnt, struct mount *shadows)
+static void commit_tree(struct mount *mnt)
 {
 	struct mount *parent = mnt->mnt_parent;
 	struct mount *m;

@@ -925,7 +927,7 @@ static void commit_tree(struct mount *mnt, struct mount *shadows)
 	n->mounts += n->pending_mounts;
 	n->pending_mounts = 0;
 
-	attach_shadowed(mnt, parent, shadows);
+	__attach_mnt(mnt, parent);
 	touch_mnt_namespace(n);
 }
 

@@ -1764,7 +1766,6 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 			continue;
 
 		for (s = r; s; s = next_mnt(s, r)) {
-			struct mount *t = NULL;
 			if (!(flag & CL_COPY_UNBINDABLE) &&
 			    IS_MNT_UNBINDABLE(s)) {
 				s = skip_mnt_tree(s);

@@ -1786,14 +1787,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 				goto out;
 			lock_mount_hash();
 			list_add_tail(&q->mnt_list, &res->mnt_list);
-			mnt_set_mountpoint(parent, p->mnt_mp, q);
-			if (!list_empty(&parent->mnt_mounts)) {
-				t = list_last_entry(&parent->mnt_mounts,
-					struct mount, mnt_child);
-				if (t->mnt_mp != p->mnt_mp)
-					t = NULL;
-			}
-			attach_shadowed(q, parent, t);
+			attach_mnt(q, parent, p->mnt_mp);
 			unlock_mount_hash();
 		}
 	}

@@ -1992,10 +1986,18 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 {
 	HLIST_HEAD(tree_list);
 	struct mnt_namespace *ns = dest_mnt->mnt_ns;
+	struct mountpoint *smp;
 	struct mount *child, *p;
 	struct hlist_node *n;
 	int err;
 
+	/* Preallocate a mountpoint in case the new mounts need
+	 * to be tucked under other mounts.
+	 */
+	smp = get_mountpoint(source_mnt->mnt.mnt_root);
+	if (IS_ERR(smp))
+		return PTR_ERR(smp);
+
 	/* Is there space to add these mounts to the mount namespace? */
 	if (!parent_path) {
 		err = count_mounts(ns, source_mnt);

@@ -2022,16 +2024,19 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 		touch_mnt_namespace(source_mnt->mnt_ns);
 	} else {
 		mnt_set_mountpoint(dest_mnt, dest_mp, source_mnt);
-		commit_tree(source_mnt, NULL);
+		commit_tree(source_mnt);
 	}
 
 	hlist_for_each_entry_safe(child, n, &tree_list, mnt_hash) {
 		struct mount *q;
 		hlist_del_init(&child->mnt_hash);
-		q = __lookup_mnt_last(&child->mnt_parent->mnt,
-				      child->mnt_mountpoint);
-		commit_tree(child, q);
+		q = __lookup_mnt(&child->mnt_parent->mnt,
+				 child->mnt_mountpoint);
+		if (q)
+			mnt_change_mountpoint(child, smp, q);
+		commit_tree(child);
 	}
+	put_mountpoint(smp);
 	unlock_mount_hash();
 
 	return 0;

@@ -2046,6 +2051,11 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 	cleanup_group_ids(source_mnt, NULL);
  out:
 	ns->pending_mounts = 0;
+
+	read_seqlock_excl(&mount_lock);
+	put_mountpoint(smp);
+	read_sequnlock_excl(&mount_lock);
+
 	return err;
 }
 

fs/pnode.c +16 -5

index 06a793f4ae38..5bc7896d122a 100644
@@ -322,6 +322,21 @@ int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
 	return ret;
 }
 
+static struct mount *find_topper(struct mount *mnt)
+{
+	/* If there is exactly one mount covering mnt completely return it. */
+	struct mount *child;
+
+	if (!list_is_singular(&mnt->mnt_mounts))
+		return NULL;
+
+	child = list_first_entry(&mnt->mnt_mounts, struct mount, mnt_child);
+	if (child->mnt_mountpoint != mnt->mnt.mnt_root)
+		return NULL;
+
+	return child;
+}
+
 /*
  * return true if the refcount is greater than count
  */

@@ -342,9 +357,8 @@ static inline int do_refcount_check(struct mount *mnt, int count)
  */
 int propagate_mount_busy(struct mount *mnt, int refcnt)
 {
-	struct mount *m, *child;
+	struct mount *m, *child, *topper;
 	struct mount *parent = mnt->mnt_parent;
-	int ret = 0;
 
 	if (mnt == parent)
 		return do_refcount_check(mnt, refcnt);

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

fs/pnode.c +24 -6

+		if (topper)
+			count += 1;
+		else if (!list_empty(&child->mnt_mounts))
+			continue;
+
+		if (do_refcount_check(child, count))
+			return 1;
 	}
-	return ret;
+	return 0;
 }
 
 /*

@@ -381,7 +407,7 @@ void propagate_mount_unlock(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
 		if (child)
 			child->mnt.mnt_flags &= ~MNT_LOCKED;
 	}

@@ -399,9 +425,11 @@ static void mark_umount_candidates(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
-		if (child && (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m))) {
+		if (!child || (child->mnt.mnt_flags & MNT_UMOUNT))
+			continue;
+		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
 			SET_MNT_MARK(child);
 		}
 	}

@@ -420,8 +448,8 @@ static void __propagate_umount(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *topper;
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
 		/*
 		 * umount the child only if the child has no children

@@ -430,6 +458,15 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;
 		CLEAR_MNT_MARK(child);
+
+		/* If there is exactly one mount covering all of child
+		 * replace child with that mount.
+		 */
+		topper = find_topper(child);
+		if (topper)
+			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp,
+					      topper);
+
 		if (list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
 			child->mnt.mnt_flags |= MNT_UMOUNT;

fs/pnode.h +2 -0

index 550f5a8b4fcf..dc87e65becd2 100644
@@ -49,6 +49,8 @@ int get_dominating_id(struct mount *mnt, const struct path *root);
 unsigned int mnt_get_count(struct mount *mnt);
 void mnt_set_mountpoint(struct mount *, struct mountpoint *,
 			struct mount *);
+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp,
+			   struct mount *mnt);
 struct mount *copy_tree(struct mount *, struct dentry *, int);
 bool is_path_reachable(struct mount *, struct dentry *,
 			 const struct path *root);

Signature · 1 line

2.10.1

Quoted 7 lines

fs/pnode.c +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

fs/pnode.c int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

fs/pnode.c +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

maybe we need a flag to identify tucked mounts?

Quoted 74 lines

fs/pnode.c +20 -6

+
+		if (do_refcount_check(child, count))
+			return 1;
 	}
-	return ret;
+	return 0;
 }
 
 /*

@@ -381,7 +407,7 @@ void propagate_mount_unlock(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
 		if (child)
 			child->mnt.mnt_flags &= ~MNT_LOCKED;
 	}

@@ -399,9 +425,11 @@ static void mark_umount_candidates(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
-		if (child && (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m))) {
+		if (!child || (child->mnt.mnt_flags & MNT_UMOUNT))
+			continue;
+		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
 			SET_MNT_MARK(child);
 		}
 	}

@@ -420,8 +448,8 @@ static void __propagate_umount(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *topper;
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
 		/*
 		 * umount the child only if the child has no children

@@ -430,6 +458,15 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;
 		CLEAR_MNT_MARK(child);
+
+		/* If there is exactly one mount covering all of child
+		 * replace child with that mount.
+		 */
+		topper = find_topper(child);
+		if (topper)
+			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp,
+					      topper);
+
 		if (list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
 			child->mnt.mnt_flags |= MNT_UMOUNT;

fs/pnode.h +2 -0

index 550f5a8b4fcf..dc87e65becd2 100644
@@ -49,6 +49,8 @@ int get_dominating_id(struct mount *mnt, const struct path *root);
 unsigned int mnt_get_count(struct mount *mnt);
 void mnt_set_mountpoint(struct mount *, struct mountpoint *,
 			struct mount *);
+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp,
+			   struct mount *mnt);
 struct mount *copy_tree(struct mount *, struct dentry *, int);
 bool is_path_reachable(struct mount *, struct dentry *,
 			 const struct path *root);

Signature · 1 line

2.10.1

Quoted 39 lines · Ram Pai

Quoted 7 lines

fs/pnode.c +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

fs/pnode.c int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

fs/pnode.c +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

Quoted 6 lines

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

fs/pnode.c +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

fs/pnode.c int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

fs/pnode.c +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Quoted 45 lines · Ram Pai

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

fs/pnode.c +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

fs/pnode.c int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

fs/pnode.c +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

I don't see why it should continue to fail.

Quoted 13 lines

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

Quoted 8 lines

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

I am not seeing that.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 4 lines

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Oh definitely. If we have people ready to talk it through I am happy to dot as many i's and cross as many t's as we productively can.

I was just pointing out that I don't have any reason to expect that any one depends on the subtle details of the implementation today so we still have some wiggle room to fix them. Even if they are visible to user space.

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

Quoted 45 lines · Ram Pai

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

fs/pnode.c +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

fs/pnode.c int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

fs/pnode.c +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

I don't see why it should continue to fail.

Quoted 13 lines

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

Quoted 8 lines

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

I am not seeing that.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 4 lines

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Quoted 7 lines

Oh definitely. If we have people ready to talk it through I am happy to dot as many i's and cross as many t's as we productively can.

I was just pointing out that I don't have any reason to expect that any one depends on the subtle details of the implementation today so we still have some wiggle room to fix them. Even if they are visible to user space.

So I haven't seen a reply, and we are getting awfully close to the merge window. Is there anything concrete we can do to ease concerns?

Right now I am thinking my last version of the patch is the likely the best we have time and energy to manage and it would be good to merge it before the code bit rots.

Quoted 110 lines · Eric W. Biederman

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

Quoted 45 lines · Ram Pai

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

(quoted diff) +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

Review int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

(quoted diff) +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

I don't see why it should continue to fail.

Quoted 13 lines

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

Quoted 8 lines

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

I am not seeing that.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 4 lines

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Quoted 7 lines

Oh definitely. If we have people ready to talk it through I am happy to dot as many i's and cross as many t's as we productively can.

I was just pointing out that I don't have any reason to expect that any one depends on the subtle details of the implementation today so we still have some wiggle room to fix them. Even if they are visible to user space.

Quoted 6 lines

So I haven't seen a reply, and we are getting awfully close to the merge window. Is there anything concrete we can do to ease concerns?

Right now I am thinking my last version of the patch is the likely the best we have time and energy to manage and it would be good to merge it before the code bit rots.

I was waiting for some other opinions on the behavior, since I continue to think that 'one should not be able to unmount mounts on which a user has explicitly mounted upon'. I am happy to be overruled, since your patch significantly improves the rest of the semantics.

Viro?

Quoted 118 lines · Ram Pai

Quoted 116 lines · Eric W. Biederman

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

Quoted 45 lines · Ram Pai

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

(quoted diff) +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

Review int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

(quoted diff) +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

I don't see why it should continue to fail.

Quoted 13 lines

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

Quoted 8 lines

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

I am not seeing that.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 4 lines

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Quoted 7 lines

Oh definitely. If we have people ready to talk it through I am happy to dot as many i's and cross as many t's as we productively can.

I was just pointing out that I don't have any reason to expect that any one depends on the subtle details of the implementation today so we still have some wiggle room to fix them. Even if they are visible to user space.

So I haven't seen a reply, and we are getting awfully close to the merge window. Is there anything concrete we can do to ease concerns?

Right now I am thinking my last version of the patch is the likely the best we have time and energy to manage and it would be good to merge it before the code bit rots.

Quoted 6 lines

I was waiting for some other opinions on the behavior, since I continue to think that 'one should not be able to unmount mounts on which a user has explicitly mounted upon'. I am happy to be overruled, since your patch significantly improves the rest of the semantics.

Viro?

Ram Pai, just to be clear you were hoping to add the logic below to my patch?

My objections to the snippet below are:

- It makes it hard for the CRIU folks (yet more state they have to find and restore).

- It feels subjectively worse to me.

- We already have cases where mounts are unmounted transparently (umount on rmdir).

- Al Viro claims that the side/shadow mounts are ordinary mounts and maintaining this extra logic that remembers if we tucked one mount under another seems to make this them less ordinary.

- The symmetry for unmounting exists for a tucked mount. We can unmount it via propagation or we can unmount the mount above it, and then we can unmount the new underlying mount.

So I don't see why we don't
want symmetry in the other case just because we mounted on top of the mount and rather than had the mount tucked under us.

fs/namespace.c +3 -1

index 8bfad42c1ccf..8b00e0548438 100644
@@ -2047,8 +2047,10 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 		hlist_del_init(&child->mnt_hash);
 		q = __lookup_mnt(&child->mnt_parent->mnt,
 				 child->mnt_mountpoint);
-		if (q)
+		if (q) {
 			mnt_change_mountpoint(child, smp, q);
+			child->mnt.mnt_flags |= MNT_TUCKED;
+		}
 		commit_tree(child);
 	}
 	put_mountpoint(smp);

fs/pnode.c +3 -0

index 5bc7896d122a..e2a6ac68feb9 100644
@@ -327,6 +327,9 @@ static struct mount *find_topper(struct mount *mnt)
 	/* If there is exactly one mount covering mnt completely return it. */
 	struct mount *child;
 
+	if (!(mnt->mnt.mnt_flags & MNT_TUCKED))
+		return NULL;
+	
 	if (!list_is_singular(&mnt->mnt_mounts))
 		return NULL;
 

include/linux/mount.h +1 -0

index 8e0352af06b7..25ca398b19b3 100644
@@ -52,6 +52,7 @@ struct mnt_namespace;
 
 #define MNT_INTERNAL	0x4000
 
+#define MNT_TUCKED		0x020000
 #define MNT_LOCK_ATIME		0x040000
 #define MNT_LOCK_NOEXEC		0x080000
 #define MNT_LOCK_NOSUID		0x100000

Eric

Quoted 120 lines · Eric W. Biederman

Quoted 118 lines · Ram Pai

Quoted 116 lines · Eric W. Biederman

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

Quoted 45 lines · Ram Pai

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

(quoted diff) +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

Review int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

(quoted diff) +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

I don't see why it should continue to fail.

Quoted 13 lines

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

Quoted 8 lines

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

I am not seeing that.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 4 lines

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Quoted 7 lines

Oh definitely. If we have people ready to talk it through I am happy to dot as many i's and cross as many t's as we productively can.

I was just pointing out that I don't have any reason to expect that any one depends on the subtle details of the implementation today so we still have some wiggle room to fix them. Even if they are visible to user space.

So I haven't seen a reply, and we are getting awfully close to the merge window. Is there anything concrete we can do to ease concerns?

Right now I am thinking my last version of the patch is the likely the best we have time and energy to manage and it would be good to merge it before the code bit rots.

Quoted 8 lines

Quoted 6 lines

I was waiting for some other opinions on the behavior, since I continue to think that 'one should not be able to unmount mounts on which a user has explicitly mounted upon'. I am happy to be overruled, since your patch significantly improves the rest of the semantics.

Viro?

Ram Pai, just to be clear you were hoping to add the logic below to my patch?

Yes. the behavior of your patch below is what I was proposing.

Quoted 1 line

Quoted 4 lines

My objections to the snippet below are:

- It makes it hard for the CRIU folks (yet more state they have to find and restore).

true. unfortunately one more subtle detail to be aware off.

Quoted 3 lines

- It feels subjectively worse to me.

- We already have cases where mounts are unmounted transparently (umount on rmdir).

sorry. i am not aware of this case. some details will help.

Quoted 3 lines

- Al Viro claims that the side/shadow mounts are ordinary mounts and maintaining this extra logic that remembers if we tucked one mount under another seems to make this them less ordinary.

I tend to argue that they are a bit more than ordinary, for they have the ability to tuck.

Quoted 4 lines

- The symmetry for unmounting exists for a tucked mount. We can unmount it via propagation or we can unmount the mount above it, and then we can unmount the new underlying mount.

this is fine with me.

Quoted 3 lines

So I don't see why we don't
want symmetry in the other case just because we mounted on top of the mount and rather than had the mount tucked under us.

A tucked mount should be un-tuckable. I agree. But a non-tucked mount cannot pretend to be tucked and this is where I disagree.

From a system management point of view one of the largest practical

Quoted 135 lines · Ram Pai

Quoted 120 lines · Eric W. Biederman

Quoted 118 lines · Ram Pai

Quoted 116 lines · Eric W. Biederman

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

Quoted 45 lines · Ram Pai

Quoted 44 lines · Eric W. Biederman

Quoted 42 lines · Ram Pai

Quoted 7 lines

(quoted diff) +0 -3

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))

Review int propagate_mount_busy(struct mount *mnt, int refcnt) · line 384

Quoted 10 lines

(quoted diff) +9 -1

-			break;
+		int count = 1;
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
+		if (!child)
+			continue;
+
+		/* Is there exactly one mount on the child that covers
+		 * it completely whose reference should be ignored?
+		 */
+		topper = find_topper(child);

This is tricky. I understand it is trying to identify the case where a mount got tucked-in because of propagation. But this will not distinguish the case where a mount got over-mounted genuinely, not because of propagation, but because of explicit user action.

example:

case 1: (explicit user action)
B is a slave of A
mount something on A/a , it will propagate to B/a and than mount something on B/a

case 2: (tucked mount)
B is a slave of A
mount something on B/a
and than mount something on A/a

Both case 1 and case 2 lead to the same mount configuration.

however 'umount A/a' in case 1 should fail. and 'umount A/a' in case 2 should pass.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Right? in other words, umounts of 'tucked mounts' should pass(case 2).

	whereas umounts of mounts on which overmounts exist should
		fail.(case 1)

Looking at your example. I agree that case 1 will fail today.

And should continue to fail. right? Your semantics change will pass it.

I don't see why it should continue to fail.

Quoted 13 lines

Quoted 7 lines

However my actual expectation would be for both mount configurations to behave the same. In both cases something has been explicitly mounted on B/a and something has propagated to B/a. In both cases the mount on top is what was explicitly mounted, and the mount below is what was propagated to B/a.

I don't see why the order of operations should matter.

One of the subtle expectation is reversibility.

Mount followed immediately by unmount has always passed and that is the standard expectation always. Your proposed code will ensure that.

Quoted 8 lines

However there is one other subtle expectaton.

A mount cannot disappear if a user has explicitly mounted on top of it.

your proposed code will not meet that expectation.

In other words, these two expectations make it behave differently even when; arguably, they feel like the same configuration.

I am not seeing that.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

maybe we need a flag to identify tucked mounts?

To preserve our exact current semantics yes.

The mount configurations that are delibearately constructed that I am aware of are comparatively simple. I don't think anyone has even taken advantage of the shadow/side mounts at this point. I made a reasonable effort to find out and no one was even aware they existed. Much less what they were. And certainly no one I talked to could find code that used them.

Quoted 4 lines

But someday; even if its after a decade, someone ;) will stumble into this semantics and wonder 'why?'. Its better to get it right sooner. Sorry, I am blaming myself; for keeping some of the problems open thinking no one will bump into them.

Quoted 7 lines

Oh definitely. If we have people ready to talk it through I am happy to dot as many i's and cross as many t's as we productively can.

I was just pointing out that I don't have any reason to expect that any one depends on the subtle details of the implementation today so we still have some wiggle room to fix them. Even if they are visible to user space.

So I haven't seen a reply, and we are getting awfully close to the merge window. Is there anything concrete we can do to ease concerns?

Right now I am thinking my last version of the patch is the likely the best we have time and energy to manage and it would be good to merge it before the code bit rots.

Quoted 8 lines

Quoted 6 lines

I was waiting for some other opinions on the behavior, since I continue to think that 'one should not be able to unmount mounts on which a user has explicitly mounted upon'. I am happy to be overruled, since your patch significantly improves the rest of the semantics.

Viro?

Ram Pai, just to be clear you were hoping to add the logic below to my patch?

Yes. the behavior of your patch below is what I was proposing.

Quoted 3 lines

My objections to the snippet below are:

Quoted 4 lines

Quoted 2 lines

- It makes it hard for the CRIU folks (yet more state they have to find and restore).

true. unfortunately one more subtle detail to be aware off.

A bit more than that, as it means that it requires an almost exact playback of the sequence of mounts in all mount namespaces to get to the point of reproducing a mount namespace.

Quoted 6 lines

Quoted 4 lines

- It feels subjectively worse to me.

- We already have cases where mounts are unmounted transparently (umount on rmdir).

sorry. i am not aware of this case. some details will help.

The question:

What happens when we rmdir a directory that has a mount on it in another mount namespace?

What happens when someone on the nfs server deletes a directory there is a mount on?

It used to be that we returned -EBUSY, and refused the rmdir operation, and we lied in the vfs about the nfs dentry being deleted to preserve the mount.

In recent kernels I have done the work so that we transparently unmount the mounts and allow the rmdir to happen. An unprivileged user mounting over say glibc and blocking the yum update of it is a pretty serious bug.

Quoted 12 lines

Quoted 3 lines

- Al Viro claims that the side/shadow mounts are ordinary mounts and maintaining this extra logic that remembers if we tucked one mount under another seems to make this them less ordinary.

I tend to argue that they are a bit more than ordinary, for they have the ability to tuck.

Quoted 4 lines

- The symmetry for unmounting exists for a tucked mount. We can unmount it via propagation or we can unmount the mount above it, and then we can unmount the new underlying mount.

Quoted 8 lines

this is fine with me.

Quoted 3 lines

So I don't see why we don't
want symmetry in the other case just because we mounted on top of the mount and rather than had the mount tucked under us.

A tucked mount should be un-tuckable. I agree. But a non-tucked mount cannot pretend to be tucked and this is where I disagree.

I have always seen the question as: Should a mount that is propagated be unmountable via umount propagation.

Which leads me to think that allowing the umount propagation when it won't change the applications view of files and filesystems is a good thing. From my perspective it also better preserves the reversability
property that is important. The mount propgated and now the unmount
propagated.

From a system management point of view one of the largest practical problems with mount namespaces and mount propagation is: mounts that propagate into another mount namespaces but don't get unmounted.

Which is to say not unmounting something (especially silently) and leaving the filesystem busy when something could be unmounted is a practical problem for people.

I am going to be out for a week, and I am leaving in a few minutes. So I am going to push my patch to the my for-next branch, so there is a reasonable chance of merging things when the merge window opens.

If the feedback is to add the MNT_TUCKED annotations to make the patch suitable for merging to Linus's tree I will take care of that when I get back.

Eric

Quoted 15 lines · Eric W. Biederman

Quoted 13 lines · Ram Pai

Quoted 12 lines · Eric W. Biederman

Quoted 10 lines · Ram Pai

Quoted 9 lines · Eric W. Biederman

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

Quoted 5 lines · Ram Pai

Quoted 4 lines · Eric W. Biederman

Quoted 2 lines · Ram Pai

Quoted 2 lines

(quoted diff) +0 -0

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 

Quoted 4 lines

A bit more than that, as it means that it requires an almost exact playback of the sequence of mounts in all mount namespaces to get to the point of reproducing a mount namespace.

Currently dump and restore of mount namespaces is the most complicated part of CRIU. The main problem is that we don't know how a tree of mounts was created. Mounts have two types of relationships:
child<->parent and shared groups. Currently both this relationships can't be restored directly. We can not add a mount into an existing group, the group can be only inherited from a source mount. And we can not restore "tucked" mounts, which can be only appeared due to propagation.

Now we don't know an algorithm to dump and restore any set of mount points for a reqsonable time. The problem becomes more complex if we start thinking how to restore mount namespaces which lives in different user namespaces.

This patch from Eric together with my patch https://lkml.org/lkml/2017/1/23/712
can solve the problem of dumping and restoring mount namespaces.

Quoted 16 lines · Andrei Vagin

Quoted 15 lines · Eric W. Biederman

Quoted 13 lines · Ram Pai

Quoted 12 lines · Eric W. Biederman

Quoted 10 lines · Ram Pai

Quoted 9 lines · Eric W. Biederman

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

Quoted 5 lines · Ram Pai

Quoted 4 lines · Eric W. Biederman

Quoted 2 lines · Ram Pai

Quoted 2 lines

(quoted diff) +0 -0

@@ -359,12 +373,24 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 

....snip....

Quoted 6 lines

Quoted 4 lines

A bit more than that, as it means that it requires an almost exact playback of the sequence of mounts in all mount namespaces to get to the point of reproducing a mount namespace.

Quoted 7 lines

Currently dump and restore of mount namespaces is the most complicated part of CRIU. The main problem is that we don't know how a tree of mounts was created. Mounts have two types of relationships:
child<->parent and shared groups. Currently both this relationships can't be restored directly. We can not add a mount into an existing group, the group can be only inherited from a source mount. And we can not restore "tucked" mounts, which can be only appeared due to

Quoted 8 lines

propagation.

Now we don't know an algorithm to dump and restore any set of mount points for a reqsonable time. The problem becomes more complex if we start thinking how to restore mount namespaces which lives in different user namespaces.

Quoted 2 lines

This patch from Eric together with my patch https://lkml.org/lkml/2017/1/23/712
can solve the problem of dumping and restoring mount namespaces.

Lets say we exposed the tucked flag for the mount in the /proc/*/mountinfo, than there will be no need to know the history. Its just a matter or setting that flag as part of
your new MS_SET_GROUP mount() call. right?

Looking at the patch at https://lkml.org/lkml/2017/1/23/712, I am guessing the CRUI dumps information from mountinfo to restore it later.

RP

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

Thefore rework the logic in propgate_mount_busy so that for each mount it examines it considers that mount busy if that mount has children or if there are extraneous references to that mount.

While this check was incorrect we could leak mounts instead of simply failing umount.

Quoted 10 lines · Eric W. Biederman

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

... because?

Quoted 4 lines

Thefore rework the logic in propgate_mount_busy so that for each mount it examines it considers that mount busy if that mount has children or if there are extraneous references to that mount.

Quoted 2 lines

While this check was incorrect we could leak mounts instead of simply failing umount.

What do you mean, leak? We ended up not unmounting them, and they
stayed around until umount of whatever they'd been shadowed by/slipped under had exposed them and they got explicitly unmounted.

This is not a leak in a sense of "data structure is unreachable and
will never be freed", unlike the one your previous version had introduced.

Your change might very well be a nicer behaviour - or a DoS in making. But it really deserves more detailed rationale than that and yes, it is a user-visible change. With rather insane userland setups in that area (*cough* systemd *cough* docker), it's _not_ obviously correct.

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

Thefore rework the logic in propgate_mount_busy so that for each mount it examines it considers that mount busy if that mount has children or if there are extraneous references to that mount.

While this check was incorrect we could leak mounts instead of simply failing umount.

Cc [email protected]

Fixes a05964f3917c ("[PATCH] shared mounts handling: umount")

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

---

If you don't figure this fix is worth it after all of this time please let me know. This feels like the proper thing to do, and I don't expect it will break anyone to fix this.

 fs/pnode.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

fs/pnode.c +6 -5

index 06a793f4ae38..12fafa711114 100644
@@ -344,7 +344,6 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 {
 	struct mount *m, *child;
 	struct mount *parent = mnt->mnt_parent;
-	int ret = 0;
 
 	if (mnt == parent)
 		return do_refcount_check(mnt, refcnt);

@@ -360,11 +359,13 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
 		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))
-			break;
+		if (!child)
+			continue;
+		if (!list_empty(&child->mnt_mounts) ||
+		    do_refcount_check(child, 1))
+			return 1;
 	}
-	return ret;
+	return 0;
 }
 
 /*

Signature · 1 line

2.10.1

Hi Eric,

Something wrong is in this patch. Pls, take a look at this script:

[root@fc24 ~]# unshare -m bash -x xxx.sh
+ set -x -e -m
+ mount --make-rprivate /
+ mount --make-shared /
+ mount -t tmpfs xxx /mnt
+ mount --make-private /mnt
+ mkdir /mnt/yyy
+ mount -t tmpfs xxx /mnt/yyy
+ sleep 1
+ unshare --propagation unchanged -m sleep 1000
+ pid=452
+ umount /mnt/yyy
+ umount /mnt/
umount: /mnt/: target is busy
(In some cases useful info about processes that
use the device is found by lsof(8) or fuser(1).)
+ echo FAIL
FAIL
+ kill 452
+ wait
xxx.sh: line 15: 452 Terminated unshare --propagation
unchanged -m sleep 1000

[root@fc24 ~]# cat xxx.sh
set -x -e -m

mount --make-rprivate /
mount --make-shared /
mount -t tmpfs xxx /mnt
mount --make-private /mnt
mkdir /mnt/yyy
mount -t tmpfs xxx /mnt/yyy
unshare --propagation unchanged -m sleep 1000 &
sleep 1
pid=$!
umount /mnt/yyy
umount /mnt/ || echo FAIL
kill $pid
wait

Quoted 63 lines · Eric W. Biederman

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

Thefore rework the logic in propgate_mount_busy so that for each mount it examines it considers that mount busy if that mount has children or if there are extraneous references to that mount.

While this check was incorrect we could leak mounts instead of simply failing umount.

Cc [email protected]

Fixes a05964f3917c ("[PATCH] shared mounts handling: umount")

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

---

If you don't figure this fix is worth it after all of this time please let me know. This feels like the proper thing to do, and I don't expect it will break anyone to fix this.

 fs/pnode.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

fs/pnode.c +6 -5

index 06a793f4ae38..12fafa711114 100644
@@ -344,7 +344,6 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 {
 	struct mount *m, *child;
 	struct mount *parent = mnt->mnt_parent;
-	int ret = 0;
 
 	if (mnt == parent)
 		return do_refcount_check(mnt, refcnt);

@@ -360,11 +359,13 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
 		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))
-			break;
+		if (!child)
+			continue;
+		if (!list_empty(&child->mnt_mounts) ||
+		    do_refcount_check(child, 1))
+			return 1;
 	}
-	return ret;
+	return 0;
 }
 
 /*

Signature · 1 line

2.10.1

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

Thefore rework the logic in propgate_mount_busy so that for each mount it examines it considers that mount busy if that mount has children or if there are extraneous references to that mount.

While this check was incorrect we could leak mounts instead of simply failing umount.

Cc [email protected]

Fixes a05964f3917c ("[PATCH] shared mounts handling: umount")

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

---

If you don't figure this fix is worth it after all of this time please let me know. This feels like the proper thing to do, and I don't expect it will break anyone to fix this.

 fs/pnode.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

fs/pnode.c +6 -5

index 06a793f4ae38..12fafa711114 100644
@@ -344,7 +344,6 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 {
 	struct mount *m, *child;
 	struct mount *parent = mnt->mnt_parent;
-	int ret = 0;
 
 	if (mnt == parent)
 		return do_refcount_check(mnt, refcnt);

@@ -360,11 +359,13 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
 		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
-		if (child && list_empty(&child->mnt_mounts) &&
-		    (ret = do_refcount_check(child, 1)))
-			break;
+		if (!child)
+			continue;
+		if (!list_empty(&child->mnt_mounts) ||
+		    do_refcount_check(child, 1))
+			return 1;
 	}
-	return ret;
+	return 0;
 }
 
 /*

Signature · 1 line

2.10.1

Quoted 10 lines · Eric W. Biederman

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

the reason why we had to do it that way was because there were situations where it was impossible to umount anything...

take for example.

(1) mount --make-shared A

(2) mount --bind A A/a

The tree looks like this

A

	|
        B

(3) mount --bind A B/a
The tree looks like this
A

	|
 	B B'   (B' becomes a shadow mount)
	|
        C

(4) mount --make-slave A
At this point B and C are peers and A is a slave.

(5) umount B'
NOTE: This used to be possible a decade ago if the process doing the umount had access to its dentry.
The tree looks like this
A

	|
 	B
	|
        C

Now if you try to unmount C, it becomes impossible, reason being...

B is the parent of C.
So the umount propagates to A. But A has B mounted at the same location. But B is busy since it has got a child C.
So the entire umount has to fail. There is no way to umount it all. Kind of stuck for ever. That is the reason; in those days a decade ago, we relaxed the rule to let go propagated mounts that had children.

The above example is a simplest case that demonstrates the phenomenon.

Given that, the current code does not allow any process to reach shadow mount B' and given that we are getting rid of shadow mounts, I think we should allow the code changes you propose in this patch.

Quoted 59 lines · Ram Pai

Quoted 10 lines · Eric W. Biederman

When I look at what propagate_mount_busy is trying to do and I look at the code closely I discover there is a great disconnect between the two. In the ordinary non-propagation case propagate_mount_busy has been verifying that there are no submounts and that there are no extraneous references on the mount.

For mounts that the unmount would propagate to propagate_mount_busy has been verifying that there are no extraneous references only if there are no submounts. Which is nonsense.

the reason why we had to do it that way was because there were situations where it was impossible to umount anything...

take for example.

(1) mount --make-shared A

(2) mount --bind A A/a

The tree looks like this

A

	|
        B

(3) mount --bind A B/a
The tree looks like this
A

	|
 	B B'   (B' becomes a shadow mount)
	|
        C

(4) mount --make-slave A
At this point B and C are peers and A is a slave.

(5) umount B'
NOTE: This used to be possible a decade ago if the process doing the umount had access to its dentry.
The tree looks like this
A

	|
 	B
	|
        C

Now if you try to unmount C, it becomes impossible, reason being...

B is the parent of C.
So the umount propagates to A. But A has B mounted at the same location. But B is busy since it has got a child C.
So the entire umount has to fail. There is no way to umount it all. Kind of stuck for ever. That is the reason; in those days a decade ago, we relaxed the rule to let go propagated mounts that had children.

Quoted 5 lines

The above example is a simplest case that demonstrates the phenomenon.

Given that, the current code does not allow any process to reach shadow mount B' and given that we are getting rid of shadow mounts, I think we should allow the code changes you propose in this patch.

Thank you very much for the good description of why propagate_mount_busy works the way it does.

I just finished taking a hard look at this and in fact the current code does allow reaching B' via umount propagation. My other patch changes exactly how you have to reach it but it is still possible to umount B'

At the same time those mounts have alwasy been unmountable with "umount -l" aka MOUNT_DETACH.

Have you ever encountered a non-contrived situation that leads to this kind of problem?

Quoted 10 lines · Al Viro

Quoted 8 lines · Eric W. Biederman

Quoted 4 lines

fs/namespace.c static int attach_recursive_mnt(struct mount *source_mnt, · line 2032

Quoted 1 line

fs/namespace.c +1 -0

+		if (child->mnt.mnt_root == smp->m_dentry) {

Explain, please. In which case is that condition _not_ satisfied, and what should happen i

When a tree is grafted in that condition does not apply to the lower leaves of the tree. At the same time nothing needs to be done for those leaves. Only the primary mountpoint needs to worry about tucking.

Quoted 2 lines

How in hell would those lower leaves end up on the list in
attach_recursive_mnt()? IDGI...

The submounts of a mount tree that is being attached need to have commit_tree called on them to attach them to a mount namespace.

Quoted 4 lines

You have "if mount is not locked - mark it; if mount is already marked - mark it again". The latter part (|| IS_MNT_MARKED(mnt), that is) looks very odd, won't you agree? What the hell was that (its counterpart in the earlier code) about?

Not mark it again. If the parent is marked mark the child.

Ever since mount propagation was introduced in cases where a mount in propagated to parent mount mountpoint pair that is already in use the code has placed the new mount behind the old mount in the mount hash table.

This implementation detail is problematic as it allows creating arbitrary length mount hash chains.

Furthermore it invalidates the constraint maintained elsewhere in the mount code that a parent mount and a mountpoint pair will have exactly one mount upon them. Making it hard to deal with and to talk about this special case in the mount code.

Modify mount propagation to notice when there is already a mount at the parent mount and mountpoint where a new mount is propagating to and place that preexisting mount on top of the new mount.

Modify unmount propagation to notice when a mount that is being unmounted has another mount on top of it (and no other children), and to replace the unmounted mount with the mount on top of it.

Move the MNT_UMUONT test from __lookup_mnt_last into __propagate_umount as that is the only call of __lookup_mnt_last where MNT_UMOUNT may be set on any mount visible in the mount hash table.

These modifications allow:
- __lookup_mnt_last to be removed.
- attach_shadows to be renamed __attach_mnt and the it's shadow handling to be removed.
- commit_tree to be simplified
- copy_tree to be simplified

The result is an easier to understand tree of mounts that does not allow creation of arbitrary length hash chains in the mount hash table.

v2: Updated to mnt_change_mountpoint to not call dput or mntput and instead to decrement the counts directly. It is guaranteed that there will be other references when mnt_change_mountpoint is called so this is safe.

Cc [email protected]

Fixes b90fa9ae8f51 ("[PATCH] shared mount handling: bind and rbind")

Tested-by Andrei Vagin <[email protected]>

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

---

Since the last version some of you may have seen I have modified my implementation of mnt_change_mountpoint so that it no longer calls mntput or dput but instead relies on the knowledge that it can not possibly have the last reference to the mnt and dentry of interest. This avoids code checking tools from complaining bitterly.

This is on top of my previous patch that sorts out locking of the mountpoint hash table. After time giving ample time for review I intend to push this and the previous bug fix to Linus.

 fs/mount.h     |   1 -
 fs/namespace.c | 110 +++++++++++++++++++++++++++++++--------------------------
 fs/pnode.c     |  27 ++++++++++----
 fs/pnode.h     |   2 ++
 4 files changed, 82 insertions(+), 58 deletions(-)

fs/mount.h +0 -1

index 2c856fc47ae3..2826543a131d 100644
@@ -89,7 +89,6 @@ static inline int is_mounted(struct vfsmount *mnt)
 }
 
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *);
-extern struct mount *__lookup_mnt_last(struct vfsmount *, struct dentry *);
 
 extern int __legitimize_mnt(struct vfsmount *, unsigned);
 extern bool legitimize_mnt(struct vfsmount *, unsigned);

fs/namespace.c +59 -51

index 487ba30bb5c6..91ccfb73f0e0 100644
@@ -637,28 +637,6 @@ struct mount *__lookup_mnt(struct vfsmount *mnt, struct dentry *dentry)
 }
 
 /*
- * find the last mount at @dentry on vfsmount @mnt.
- * mount_lock must be held.
- */
-struct mount *__lookup_mnt_last(struct vfsmount *mnt, struct dentry *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;
-		if (!(p->mnt.mnt_flags & MNT_UMOUNT))
-			res = p;
-	}
-out:
-	return res;
-}
-
-/*
  * lookup_mnt - Return the first child mount mounted at path
  *
  * "First" means first mounted chronologically.  If you create the

@@ -878,6 +856,13 @@ void mnt_set_mountpoint(struct mount *mnt,
 	hlist_add_head(&child_mnt->mnt_mp_list, &mp->m_list);
 }
 
+static void __attach_mnt(struct mount *mnt, struct mount *parent)
+{
+	hlist_add_head_rcu(&mnt->mnt_hash,
+			   m_hash(&parent->mnt, mnt->mnt_mountpoint));
+	list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
+}
+
 /*
  * vfsmount lock must be held for write
  */

@@ -886,28 +871,45 @@ static void attach_mnt(struct mount *mnt,
 			struct mountpoint *mp)
 {
 	mnt_set_mountpoint(parent, mp, mnt);
-	hlist_add_head_rcu(&mnt->mnt_hash, m_hash(&parent->mnt, mp->m_dentry));
-	list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
+	__attach_mnt(mnt, parent);
 }
 
-static void attach_shadowed(struct mount *mnt,
-			struct mount *parent,
-			struct mount *shadows)
+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct mount *mnt)
 {
-	if (shadows) {
-		hlist_add_behind_rcu(&mnt->mnt_hash, &shadows->mnt_hash);
-		list_add(&mnt->mnt_child, &shadows->mnt_child);
-	} else {
-		hlist_add_head_rcu(&mnt->mnt_hash,
-				m_hash(&parent->mnt, mnt->mnt_mountpoint));
-		list_add_tail(&mnt->mnt_child, &parent->mnt_mounts);
-	}
+	struct mountpoint *old_mp = mnt->mnt_mp;
+	struct dentry *old_mountpoint = mnt->mnt_mountpoint;
+	struct mount *old_parent = mnt->mnt_parent;
+
+	list_del_init(&mnt->mnt_child);
+	hlist_del_init(&mnt->mnt_mp_list);
+	hlist_del_init_rcu(&mnt->mnt_hash);
+
+	attach_mnt(mnt, parent, mp);
+
+	put_mountpoint(old_mp);
+
+	/*
+	 * Safely avoid even the suggestion this code might sleep or
+	 * lock the mount hash by taking avantage of the knowlege that
+	 * mnt_change_mounpoint will not release the final reference
+	 * to a mountpoint.
+	 *
+	 * During mounting, another mount will continue to use the old
+	 * mountpoint and during unmounting, the old mountpoint will
+	 * continue to exist until namespace_unlock which happens well
+	 * after mnt_change_mountpoint.
+	 */
+	spin_lock(&old_mountpoint->d_lock);
+	old_mountpoint->d_lockref.count--;
+	spin_unlock(&old_mountpoint->d_lock);
+
+	mnt_add_count(old_parent, -1);
 }
 
 /*
  * vfsmount lock must be held for write
  */
-static void commit_tree(struct mount *mnt, struct mount *shadows)
+static void commit_tree(struct mount *mnt)
 {
 	struct mount *parent = mnt->mnt_parent;
 	struct mount *m;

@@ -925,7 +927,7 @@ static void commit_tree(struct mount *mnt, struct mount *shadows)
 	n->mounts += n->pending_mounts;
 	n->pending_mounts = 0;
 
-	attach_shadowed(mnt, parent, shadows);
+	__attach_mnt(mnt, parent);
 	touch_mnt_namespace(n);
 }
 

@@ -1764,7 +1766,6 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 			continue;
 
 		for (s = r; s; s = next_mnt(s, r)) {
-			struct mount *t = NULL;
 			if (!(flag & CL_COPY_UNBINDABLE) &&
 			    IS_MNT_UNBINDABLE(s)) {
 				s = skip_mnt_tree(s);

@@ -1786,14 +1787,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 				goto out;
 			lock_mount_hash();
 			list_add_tail(&q->mnt_list, &res->mnt_list);
-			mnt_set_mountpoint(parent, p->mnt_mp, q);
-			if (!list_empty(&parent->mnt_mounts)) {
-				t = list_last_entry(&parent->mnt_mounts,
-					struct mount, mnt_child);
-				if (t->mnt_mp != p->mnt_mp)
-					t = NULL;
-			}
-			attach_shadowed(q, parent, t);
+			attach_mnt(q, parent, p->mnt_mp);
 			unlock_mount_hash();
 		}
 	}

@@ -1992,10 +1986,18 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 {
 	HLIST_HEAD(tree_list);
 	struct mnt_namespace *ns = dest_mnt->mnt_ns;
+	struct mountpoint *smp;
 	struct mount *child, *p;
 	struct hlist_node *n;
 	int err;
 
+	/* Preallocate a mountpoint in case the new mounts need
+	 * to be tucked under other mounts.
+	 */
+	smp = get_mountpoint(source_mnt->mnt.mnt_root);
+	if (IS_ERR(smp))
+		return PTR_ERR(smp);
+
 	/* Is there space to add these mounts to the mount namespace? */
 	if (!parent_path) {
 		err = count_mounts(ns, source_mnt);

@@ -2022,17 +2024,22 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 		touch_mnt_namespace(source_mnt->mnt_ns);
 	} else {
 		mnt_set_mountpoint(dest_mnt, dest_mp, source_mnt);
-		commit_tree(source_mnt, NULL);
+		commit_tree(source_mnt);
 	}
 
 	hlist_for_each_entry_safe(child, n, &tree_list, mnt_hash) {
-		struct mount *q;
 		hlist_del_init(&child->mnt_hash);
-		q = __lookup_mnt_last(&child->mnt_parent->mnt,
-				      child->mnt_mountpoint);
-		commit_tree(child, q);
+		if (child->mnt.mnt_root == smp->m_dentry) {
+			struct mount *q;
+			q = __lookup_mnt(&child->mnt_parent->mnt,
+					 child->mnt_mountpoint);
+			if (q)
+				mnt_change_mountpoint(child, smp, q);
+		}
+		commit_tree(child);
 	}
 	unlock_mount_hash();
+	put_mountpoint(smp);
 
 	return 0;
 

@@ -2046,6 +2053,7 @@ static int attach_recursive_mnt(struct mount *source_mnt,
 	cleanup_group_ids(source_mnt, NULL);
  out:
 	ns->pending_mounts = 0;
+	put_mountpoint(smp);
 	return err;
 }
 

fs/pnode.c +21 -6

index 06a793f4ae38..eb4331240fd1 100644
@@ -327,6 +327,9 @@ int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
  */
 static inline int do_refcount_check(struct mount *mnt, int count)
 {
+	struct mount *topper = __lookup_mnt(&mnt->mnt, mnt->mnt.mnt_root);
+	if (topper)
+		count++;
 	return mnt_get_count(mnt) > count;
 }
 

@@ -359,7 +362,7 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
 
 	for (m = propagation_next(parent, parent); m;
 	     		m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
 		if (child && list_empty(&child->mnt_mounts) &&
 		    (ret = do_refcount_check(child, 1)))
 			break;

@@ -381,7 +384,7 @@ void propagate_mount_unlock(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
+		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
 		if (child)
 			child->mnt.mnt_flags &= ~MNT_LOCKED;
 	}

@@ -399,9 +402,11 @@ static void mark_umount_candidates(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
-		if (child && (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m))) {
+		if (!child || (child->mnt.mnt_flags & MNT_UMOUNT))
+			continue;
+		if (!IS_MNT_LOCKED(child) || IS_MNT_MARKED(m)) {
 			SET_MNT_MARK(child);
 		}
 	}

@@ -420,8 +425,8 @@ static void __propagate_umount(struct mount *mnt)
 
 	for (m = propagation_next(parent, parent); m;
 			m = propagation_next(m, parent)) {
-
-		struct mount *child = __lookup_mnt_last(&m->mnt,
+		struct mount *topper;
+		struct mount *child = __lookup_mnt(&m->mnt,
 						mnt->mnt_mountpoint);
 		/*
 		 * umount the child only if the child has no children

@@ -430,6 +435,16 @@ static void __propagate_umount(struct mount *mnt)
 		if (!child || !IS_MNT_MARKED(child))
 			continue;
 		CLEAR_MNT_MARK(child);
+
+		/* If there is exactly one mount covering all of child
+		 * replace child with that mount.
+		 */
+		topper = __lookup_mnt(&child->mnt, child->mnt.mnt_root);
+		if (topper &&
+		    (child->mnt_mounts.next == &topper->mnt_child) &&
+		    (topper->mnt_child.next == &child->mnt_mounts))
+			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp, topper);
+
 		if (list_empty(&child->mnt_mounts)) {
 			list_del_init(&child->mnt_child);
 			child->mnt.mnt_flags |= MNT_UMOUNT;

fs/pnode.h +2 -0

index 550f5a8b4fcf..dc87e65becd2 100644
@@ -49,6 +49,8 @@ int get_dominating_id(struct mount *mnt, const struct path *root);
 unsigned int mnt_get_count(struct mount *mnt);
 void mnt_set_mountpoint(struct mount *, struct mountpoint *,
 			struct mount *);
+void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp,
+			   struct mount *mnt);
 struct mount *copy_tree(struct mount *, struct dentry *, int);
 bool is_path_reachable(struct mount *, struct dentry *,
 			 const struct path *root);

Signature · 1 line

2.10.1