[PATCH][CFT][RFC] clone_mnt(): simplify the propagation-related logics

[PATCH][CFT][RFC] clone_mnt(): simplify the propagation-related logics

[Al Viro]

[
Help with testing and review would be very welcome; it does survive
xfstests and ltp.  No visible regressions on kselftests either.
IIRC, Christian mentioned some bunch of mount-related regression
tests somewhere; was that a part of kselftests, or is it something
separate?
]

The underlying rules are simple:
	* MNT_SHARED should be set iff ->mnt_group_id of new mount ends up
non-zero.
	* mounts should be on the same ->mnt_share cyclic list iff they have
the same non-zero ->mnt_group_id value.
	* CL_PRIVATE is mutually exclusive with MNT_SHARED, MNT_SLAVE,
MNT_SHARED_TO_SLAVE and MNT_EXPIRE; the whole point of that thing is to
get a clone of old mount that would *not* be on any namespace-related
lists.

The above allows to make the logics more straightforward; what's more,
it makes the proof that invariants are maintained much simpler.
The variant in mainline is safe (aside of a very narrow race with
unsafe modification of mnt_flags right after we had the mount exposed
in superblock's ->s_mounts; theoretically it can race with ro remount
of the original, but it's not easy to hit), but proof of its correctness
is really unpleasant.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 165b3bd26857..4456a6448b45 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1351,6 +1351,9 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	if (!mnt)
 		return ERR_PTR(-ENOMEM);
 
+	mnt->mnt.mnt_flags = READ_ONCE(old->mnt.mnt_flags) &
+			     ~MNT_INTERNAL_FLAGS;
+
 	if (flag & (CL_SLAVE | CL_PRIVATE | CL_SHARED_TO_SLAVE))
 		mnt->mnt_group_id = 0; /* not a peer of original */
 	else
@@ -1362,8 +1365,8 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 			goto out_free;
 	}
 
-	mnt->mnt.mnt_flags = old->mnt.mnt_flags;
-	mnt->mnt.mnt_flags &= ~(MNT_WRITE_HOLD|MNT_MARKED|MNT_INTERNAL|MNT_LOCKED);
+	if (mnt->mnt_group_id)
+		set_mnt_shared(mnt);
 
 	atomic_inc(&sb->s_active);
 	mnt->mnt.mnt_idmap = mnt_idmap_get(mnt_idmap(&old->mnt));
@@ -1376,22 +1379,20 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	list_add_tail(&mnt->mnt_instance, &sb->s_mounts);
 	unlock_mount_hash();
 
+	if (flag & CL_PRIVATE)	// we are done with it
+		return mnt;
+
+	if (mnt->mnt_group_id && mnt->mnt_group_id == old->mnt_group_id)
+		list_add(&mnt->mnt_share, &old->mnt_share);
+
 	if ((flag & CL_SLAVE) ||
 	    ((flag & CL_SHARED_TO_SLAVE) && IS_MNT_SHARED(old))) {
 		list_add(&mnt->mnt_slave, &old->mnt_slave_list);
 		mnt->mnt_master = old;
-		CLEAR_MNT_SHARED(mnt);
-	} else if (!(flag & CL_PRIVATE)) {
-		if ((flag & CL_MAKE_SHARED) || IS_MNT_SHARED(old))
-			list_add(&mnt->mnt_share, &old->mnt_share);
-		if (IS_MNT_SLAVE(old))
-			list_add(&mnt->mnt_slave, &old->mnt_slave);
+	} else if (IS_MNT_SLAVE(old)) {
+		list_add(&mnt->mnt_slave, &old->mnt_slave);
 		mnt->mnt_master = old->mnt_master;
-	} else {
-		CLEAR_MNT_SHARED(mnt);
 	}
-	if (flag & CL_MAKE_SHARED)
-		set_mnt_shared(mnt);
 
 	/* stick the duplicate mount on the same expiry list
 	 * as the original if that was on one */
@@ -1399,7 +1400,6 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 		if (!list_empty(&old->mnt_expire))
 			list_add(&mnt->mnt_expire, &old->mnt_expire);
 	}
-
 	return mnt;
 
  out_free:

Re: [PATCH][CFT][RFC] clone_mnt(): simplify the propagation-related logics

[Christian Brauner]

On Wed, May 07, 2025 at 10:15:23PM +0100, Al Viro wrote:
> [
> Help with testing and review would be very welcome; it does survive
> xfstests and ltp.  No visible regressions on kselftests either.
> IIRC, Christian mentioned some bunch of mount-related regression
> tests somewhere; was that a part of kselftests, or is it something
> separate?

I've made them all part of:

tools/testing/selftests/mount_setattr/

So just run mount_setattr_test that should excercise most of the
functionality.

In addition simple clone LTP, compile and do:

sudo ./kirk -f ltp -r fs_perms_simple fs_bind

The crucial part is "fs_bind" which should excercise a slew of mount
propagation tests I always run.

> ]
> 
> The underlying rules are simple:
> 	* MNT_SHARED should be set iff ->mnt_group_id of new mount ends up
> non-zero.
> 	* mounts should be on the same ->mnt_share cyclic list iff they have
> the same non-zero ->mnt_group_id value.
> 	* CL_PRIVATE is mutually exclusive with MNT_SHARED, MNT_SLAVE,
> MNT_SHARED_TO_SLAVE and MNT_EXPIRE; the whole point of that thing is to
> get a clone of old mount that would *not* be on any namespace-related
> lists.
> 
> The above allows to make the logics more straightforward; what's more,
> it makes the proof that invariants are maintained much simpler.
> The variant in mainline is safe (aside of a very narrow race with
> unsafe modification of mnt_flags right after we had the mount exposed
> in superblock's ->s_mounts; theoretically it can race with ro remount
> of the original, but it's not easy to hit), but proof of its correctness
> is really unpleasant.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Looks good,
Reviewed-by: Christian Brauner <brauner@kernel.org>