[PATCH 0/2] mount: fix detached mount regression

[PATCH 0/2] mount: fix detached mount regression

[Christian Brauner]

When we disabled mount propagation into detached trees again this
accidently broke mounting detached mount trees onto other detached mount
trees. The mount_setattr_tests selftests fail and Allison reported it as
well. Fix the regression.

Also fix the selftests for detached tree propagation now that we
disabled that again.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (2):
      mount: fix detached mount regression
      selftests/mount_setattr: adapt detached mount propagation test

 fs/namespace.c                                     | 28 +++++++++++-----------
 .../selftests/mount_setattr/mount_setattr_test.c   | 17 +------------
 2 files changed, 15 insertions(+), 30 deletions(-)
---
base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282
change-id: 20250605-work-mount-regression-2993df82e99b

[PATCH 1/2] mount: fix detached mount regression

[Christian Brauner]

When we disabled mount propagation into detached trees again this
accidently broke mounting detached mount trees onto other detached mount
trees. The mount_setattr_tests selftests fail and Allison reported it as
well. Fix the regression.

Fixes: 3b5260d12b1f ("Don't propagate mounts into detached trees")
Reported-by: Allison Karlitskaya <lis@redhat.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 2f2e93927f46..cc08eab031db 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3634,22 +3634,22 @@ static int do_move_mount(struct path *old_path,
 	if (!(attached ? check_mnt(old) : is_anon_ns(ns)))
 		goto out;
 
-	if (is_anon_ns(ns) && ns == p->mnt_ns) {
-		/*
-		 * Ending up with two files referring to the root of the
-		 * same anonymous mount namespace would cause an error
-		 * as this would mean trying to move the same mount
-		 * twice into the mount tree which would be rejected
-		 * later. But be explicit about it right here.
-		 */
+	/*
+	 * Ending up with two files referring to the root of the
+	 * same anonymous mount namespace would cause an error
+	 * as this would mean trying to move the same mount
+	 * twice into the mount tree which would be rejected
+	 * later. But be explicit about it right here.
+	 */
+	if (is_anon_ns(ns) && ns == p->mnt_ns)
 		goto out;
-	} else if (is_anon_ns(p->mnt_ns)) {
-		/*
-		 * Don't allow moving an attached mount tree to an
-		 * anonymous mount tree.
-		 */
+
+	/*
+	 * Don't allow moving an attached mount tree to an
+	 * anonymous mount tree.
+	 */
+	if (!is_anon_ns(ns) && is_anon_ns(p->mnt_ns))
 		goto out;
-	}
 
 	if (old->mnt.mnt_flags & MNT_LOCKED)
 		goto out;

-- 
2.47.2

[PATCH 2/2] selftests/mount_setattr: adapt detached mount propagation test

[Christian Brauner]

Make sure that detached trees don't receive mount propagation.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 .../selftests/mount_setattr/mount_setattr_test.c        | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index 8b378c91debf..b1e4618399be 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -2079,24 +2079,9 @@ TEST_F(mount_setattr, detached_tree_propagation)
 	 * means that the device information will be different for any
 	 * statx() that was taken from /mnt/A before the mount compared
 	 * to one after the mount.
-	 *
-	 * Since we already now that the device information between the
-	 * stx1 and stx2 samples are identical we also now that stx2 and
-	 * stx3 device information will necessarily differ.
 	 */
 	ASSERT_NE(stx1.stx_dev_minor, stx3.stx_dev_minor);
-
-	/*
-	 * If mount propagation worked correctly then the tmpfs mount
-	 * that was created after the mount namespace was unshared will
-	 * have propagated onto /mnt/A in the detached mount tree.
-	 *
-	 * Verify that the device information for stx3 and stx4 are
-	 * identical. It is already established that stx3 is different
-	 * from both stx1 and stx2 sampled before the tmpfs mount was
-	 * done so if stx3 and stx4 are identical the proof is done.
-	 */
-	ASSERT_EQ(stx3.stx_dev_minor, stx4.stx_dev_minor);
+	ASSERT_EQ(stx1.stx_dev_minor, stx4.stx_dev_minor);
 
 	EXPECT_EQ(close(fd_tree), 0);
 }

-- 
2.47.2

Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro]

On Thu, Jun 05, 2025 at 02:50:53PM +0200, Christian Brauner wrote:
> When we disabled mount propagation into detached trees again this
> accidently broke mounting detached mount trees onto other detached mount
> trees. The mount_setattr_tests selftests fail and Allison reported it as
> well. Fix the regression.
> 
> Fixes: 3b5260d12b1f ("Don't propagate mounts into detached trees")
> Reported-by: Allison Karlitskaya <lis@redhat.com>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/namespace.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 2f2e93927f46..cc08eab031db 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3634,22 +3634,22 @@ static int do_move_mount(struct path *old_path,
>  	if (!(attached ? check_mnt(old) : is_anon_ns(ns)))
>  		goto out;
>  
> -	if (is_anon_ns(ns) && ns == p->mnt_ns) {
> -		/*
> -		 * Ending up with two files referring to the root of the
> -		 * same anonymous mount namespace would cause an error
> -		 * as this would mean trying to move the same mount
> -		 * twice into the mount tree which would be rejected
> -		 * later. But be explicit about it right here.
> -		 */
> +	/*
> +	 * Ending up with two files referring to the root of the
> +	 * as this would mean trying to move the same mount
> +	 * twice into the mount tree which would be rejected
> +	 * later. But be explicit about it right here.
> +	 */
> +	if (is_anon_ns(ns) && ns == p->mnt_ns)
>  		goto out;
> -	} else if (is_anon_ns(p->mnt_ns)) {
> -		/*
> -		 * Don't allow moving an attached mount tree to an
> -		 * anonymous mount tree.
> -		 */
> +
> +	/*
> +	 * Don't allow moving an attached mount tree to an
> +	 * anonymous mount tree.
> +	 */
> +	if (!is_anon_ns(ns) && is_anon_ns(p->mnt_ns))
>  		goto out;

UGH...

This is much too convoluted.  How about this:

        /* The thing moved must be mounted... */
	if (!is_mounted(&old->mnt))
		goto out;

	/* ... and either ours or the root of anon namespace */
	if (check_mnt(old)) {
		/* should be detachable from parent */
		if (!mnt_has_parent(old) || IS_MNT_LOCKED(old))
			goto out;
		/* target should be ours */
		if (!check_mount(p))
			goto out;
	} else {
		if (!is_anon_ns(ns) || mnt_has_parent(old))
			goto out;
		/* not into the same anon ns - bail early in that case */
		if (p->mnt_ns == ns)
			goto out;
		/* target should be ours or in an acceptable anon */
		if (!may_use_mount(p))
			goto out;
	}

Would that work for all cases?

Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro]

On Fri, Jun 06, 2025 at 05:54:41AM +0100, Al Viro wrote:
> UGH...
> 
> This is much too convoluted.  How about this:
> 
>         /* The thing moved must be mounted... */
> 	if (!is_mounted(&old->mnt))
> 		goto out;
> 
> 	/* ... and either ours or the root of anon namespace */
> 	if (check_mnt(old)) {
> 		/* should be detachable from parent */
> 		if (!mnt_has_parent(old) || IS_MNT_LOCKED(old))
> 			goto out;
> 		/* target should be ours */
> 		if (!check_mount(p))

		     check_mnt, obviously

> 			goto out;
> 	} else {
> 		if (!is_anon_ns(ns) || mnt_has_parent(old))
> 			goto out;
> 		/* not into the same anon ns - bail early in that case */
> 		if (p->mnt_ns == ns)
> 			goto out;
> 		/* target should be ours or in an acceptable anon */
> 		if (!may_use_mount(p))
> 			goto out;
> 	}
> 
> Would that work for all cases?

The thing is, the real split is not on "has parent"; it's "is the
source in our namespace".

	If it is, we must
* have 'attached' to be true (check_mnt() is incompatible with is_anon_ns())
IOW, mnt_has_parent(old) should be true.
* have no MNT_LOCKED on the source (we check it later, and it really belongs
here - we do *not* want to check it in move-from-anon case, since there we
accept only root and we never have MNT_LOCKED on the roots of anon namespaces
* have the target to be not in anon namespace.  Which, combined with may_use_mount(p)
means that it should be in *our* namespace, so simply check_mnt(p).  And that
subsumes may_use_mount(p), so no need to check it earlier.
* check for is_anon_ns(ns) && ns == p->mnt_ns obviously does not apply.

	Otherwise, we know that 'attached' must be false and is_anon_ns(ns) - true.
Which means that we want the root of anon namespace.
* check for is_anon_ns(ns) && ns == p->mnt_ns turns into simply p->mnt_ns == ns
* check for target not being in anon namespace should be removed in this case -
that's the fix we are after.
* check for may_use_mount() is needed in this case.
* check for MNT_LOCKED is not needed afterwards - again, it's never set on the
root of anon

So everything prior to checking path_mounted() should be covered by the variant
above, and IMO it's easier to follow in that form - restrictions in 'move a
subtree of our namespace' and 'move the entire contents of anon namespace' are
rather different.  Better keep them textually separate...

Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro]

	Folks, could you check the following, on top of viro/vfs.git#fixes?

do_move_mount(): split the checks in subtree-of-our-ns and entire-anon cases

... and fix the breakage in anon-to-anon case.  There are two cases
acceptable for do_move_mount() and mixing checks for those is making
things hard to follow.

One case is move of a subtree in caller's namespace.
	* source and destination must be in caller's namespace
	* source must be detachable from parent
Another is moving the entire anon namespace elsewhere
	* source must be the root of anon namespace
	* target must either in caller's namespace or in a suitable
	  anon namespace (see may_use_mount() for details).
	* target must not be in the same namespace as source.

It's really easier to follow if tests are *not* mixed together...

[[
This should be equivalent to what Christian has posted last night;
please test on whatever tests you've got.
]]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 854099aafed5..59197109e6b9 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3656,37 +3656,29 @@ static int do_move_mount(struct path *old_path,
 	ns = old->mnt_ns;
 
 	err = -EINVAL;
-	if (!may_use_mount(p))
-		goto out;
-
 	/* The thing moved must be mounted... */
 	if (!is_mounted(&old->mnt))
 		goto out;
 
 	/* ... and either ours or the root of anon namespace */
-	if (!(attached ? check_mnt(old) : is_anon_ns(ns)))
-		goto out;
-
-	if (is_anon_ns(ns) && ns == p->mnt_ns) {
-		/*
-		 * Ending up with two files referring to the root of the
-		 * same anonymous mount namespace would cause an error
-		 * as this would mean trying to move the same mount
-		 * twice into the mount tree which would be rejected
-		 * later. But be explicit about it right here.
-		 */
-		goto out;
-	} else if (is_anon_ns(p->mnt_ns)) {
-		/*
-		 * Don't allow moving an attached mount tree to an
-		 * anonymous mount tree.
-		 */
-		goto out;
+	if (check_mnt(old)) {
+		/* should be detachable from parent */
+		if (!mnt_has_parent(old) || IS_MNT_LOCKED(old))
+			goto out;
+		/* target should be ours */
+		if (!check_mnt(p))
+			goto out;
+	} else {
+		if (!is_anon_ns(ns) || mnt_has_parent(old))
+			goto out;
+		/* not into the same anon ns - bail early in that case */
+		if (ns == p->mnt_ns)
+			goto out;
+		/* target should be ours or in an acceptable anon ns */
+		if (!may_use_mount(p))
+			goto out;
 	}
 
-	if (old->mnt.mnt_flags & MNT_LOCKED)
-		goto out;
-
 	if (!path_mounted(old_path))
 		goto out;
 

Re: [PATCH 1/2] mount: fix detached mount regression

[Christian Brauner]

On Fri, Jun 06, 2025 at 08:01:27AM +0100, Al Viro wrote:
> 	Folks, could you check the following, on top of viro/vfs.git#fixes?
> 
> do_move_mount(): split the checks in subtree-of-our-ns and entire-anon cases
> 
> ... and fix the breakage in anon-to-anon case.  There are two cases
> acceptable for do_move_mount() and mixing checks for those is making
> things hard to follow.
> 
> One case is move of a subtree in caller's namespace.
> 	* source and destination must be in caller's namespace
> 	* source must be detachable from parent
> Another is moving the entire anon namespace elsewhere
> 	* source must be the root of anon namespace
> 	* target must either in caller's namespace or in a suitable
> 	  anon namespace (see may_use_mount() for details).
> 	* target must not be in the same namespace as source.
> 
> It's really easier to follow if tests are *not* mixed together...
> 
> [[
> This should be equivalent to what Christian has posted last night;
> please test on whatever tests you've got.
> ]]
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Fwiw, check_mnt() is a useless name for this function that's been
bothering me forever. A few minor comments below. With those addressed:

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

> diff --git a/fs/namespace.c b/fs/namespace.c
> index 854099aafed5..59197109e6b9 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3656,37 +3656,29 @@ static int do_move_mount(struct path *old_path,
>  	ns = old->mnt_ns;
>  
>  	err = -EINVAL;
> -	if (!may_use_mount(p))
> -		goto out;
> -
>  	/* The thing moved must be mounted... */
>  	if (!is_mounted(&old->mnt))
>  		goto out;
>  
>  	/* ... and either ours or the root of anon namespace */
> -	if (!(attached ? check_mnt(old) : is_anon_ns(ns)))
> -		goto out;
> -
> -	if (is_anon_ns(ns) && ns == p->mnt_ns) {
> -		/*
> -		 * Ending up with two files referring to the root of the
> -		 * same anonymous mount namespace would cause an error
> -		 * as this would mean trying to move the same mount
> -		 * twice into the mount tree which would be rejected
> -		 * later. But be explicit about it right here.
> -		 */
> -		goto out;
> -	} else if (is_anon_ns(p->mnt_ns)) {
> -		/*
> -		 * Don't allow moving an attached mount tree to an
> -		 * anonymous mount tree.
> -		 */
> -		goto out;
> +	if (check_mnt(old)) {
> +		/* should be detachable from parent */
> +		if (!mnt_has_parent(old) || IS_MNT_LOCKED(old))
> +			goto out;
> +		/* target should be ours */
> +		if (!check_mnt(p))
> +			goto out;
> +	} else {

This code has gotten more powerful with my recent changes to allow
assembling private mount trees so I think we should be more liberal with
comments to be kind to our future selves. Also I would really prefer if
we could move the checks to separate lines:

/* Only allow moving anonymous mounts... */
if (!is_anon_ns(ns))
	goto out;

/* ... that are the root of their anonymous mount namespace. */
if (mnt_has_parent(old))
	goto out;

if (ns == p->mnt_ns)
	goto out;

/*
 * Finally, make sure that the target is either a mount in our mount
 * namespace or in an acceptable anonymous mount namespace.
 */
target should be ours or in an acceptable anon ns */
if (!may_use_mount(p))
	goto out;


Other than that this looks good to me.

/* 
> +		if (!is_anon_ns(ns) || mnt_has_parent(old))
> +			goto out;
> +		/* not into the same anon ns - bail early in that case */

		/* Don't allow moving into the same mount namespace
> +		if (ns == p->mnt_ns)
> +			goto out;
> +		/* target should be ours or in an acceptable anon ns */
> +		if (!may_use_mount(p))
> +			goto out;
>  	}
>  
> -	if (old->mnt.mnt_flags & MNT_LOCKED)
> -		goto out;
> -
>  	if (!path_mounted(old_path))
>  		goto out;
>  

Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro]

On Fri, Jun 06, 2025 at 09:58:26AM +0200, Christian Brauner wrote:

> Fwiw, check_mnt() is a useless name for this function that's been
> bothering me forever.

Point, but let's keep the renaming (s/check_mnt/our_mount/, for
example) separate.

> > +	if (check_mnt(old)) {
> > +		/* should be detachable from parent */
> > +		if (!mnt_has_parent(old) || IS_MNT_LOCKED(old))
> > +			goto out;
> > +		/* target should be ours */
> > +		if (!check_mnt(p))
> > +			goto out;
> > +	} else {
> 
> This code has gotten more powerful with my recent changes to allow
> assembling private mount trees so I think we should be more liberal with
> comments to be kind to our future selves. Also I would really prefer if
> we could move the checks to separate lines:
> 
> /* Only allow moving anonymous mounts... */
> if (!is_anon_ns(ns))
> 	goto out;
> 
> /* ... that are the root of their anonymous mount namespace. */
> if (mnt_has_parent(old))
> 	goto out;

Maybe, but I wonder if we should add
static inline bool anon_ns_root(struct mount *m)
{
	struct namespace *ns = m->mnt_ns;

	return !IS_ERR_OR_NULL(ns) && is_anon_ns(ns) && m == ns->root;
}
for that kind of stuff...  And I'd prefer direct comparison with root
instead of going for "well, if it's mounted in a namespace, it either
is its root or mnt_has_parent() will return true" kind of logics.

We have checks for that predicate open-coded elsewhere...

<looks>

in addition to do_move_mount() we have clone_private_mount() where
                if (!is_mounted(&old_mnt->mnt) ||
                        !is_anon_ns(old_mnt->mnt_ns) ||
                        mnt_has_parent(old_mnt))
would be replacable with
		if (!anon_ns_root(old_mnt))
and in do_mount_setattr()
        if (!is_mounted(&mnt->mnt))
                goto out;
        if ((mnt_has_parent(mnt) || !is_anon_ns(mnt->mnt_ns)) && !check_mnt(mnt))
                goto out;
with
	if (!check_mnt(mnt) && !anon_ns_root(mnt))
		goto out;

and while we are at it, dissolve_on_fput() becomes
        scoped_guard(rcu) {
                if (!anon_ns_root(m))
                        return;
        }
        scoped_guard(namespace_lock, &namespace_sem) {
                ns = m->mnt_ns;
                if (!anon_ns_root(m))
                        return;
                lock_mount_hash();
                umount_tree(m, UMOUNT_CONNECTED);
                unlock_mount_hash();
	}
        free_mnt_ns(ns);
Incidentally, location of free_mnt_ns() is one aspect of that thing
that *does* need a comment - at the first glance it looks like it
would be better off under namespace_sem.  The reason why that must
not be moved there is well-buried - it's the call of notify_mnt_list()
in namespace_unlock(), where we have calls of mnt_notify(), which
looks at m->prev_ns->n_fsnotify_marks.  IMO that's way too subtle
to be left without an explanation - that, or having something like
bury_anon_ns(ns) called under namespace_sem and triggering
free_mnt_ns() from namespace_unlock(), after the notify_mnt_list()
call there.  do_move_mount() might also benefit from the same...
Anyway, that's a separate story.

Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro]

On Fri, Jun 06, 2025 at 06:45:02PM +0100, Al Viro wrote:
> On Fri, Jun 06, 2025 at 09:58:26AM +0200, Christian Brauner wrote:
> 
> > Fwiw, check_mnt() is a useless name for this function that's been
> > bothering me forever.
> 
> Point, but let's keep the renaming (s/check_mnt/our_mount/, for
> example) separate.

Modified and force-pushed.

It does pass xfstests without regressions.  kselftests... AFAICS,
no regressions either, but the damn thing is a mess.  Example:

# # set_layers_via_fds.c:711:set_layers_via_detached_mount_fds:Expected layers_found[i] (0) == true (1)
# # set_layers_via_fds.c:39:set_layers_via_detached_mount_fds:Expected rmdir("/set_layers_via_fds") (-1) == 0 (0)
# # set_layers_via_detached_mount_fds: Test terminated by assertion
# #          FAIL  set_layers_via_fds.set_layers_via_detached_mount_fds

Not a regression, AFAICT; the underlying problem is that mount options
are shown incorrectly in the tested case.  Still present after overlayfs
merge.  mount does succeed, but... in options we see this:
rw,relatime,lowerdir+=/,lowerdir+=/,lowerdir+=/,lowerdir+=/,datadir+=/,datadir+=/,datadir+=/,upperdir=/upper,workdir=/work,redirect_dir=on,uuid=on,metacopy=on

And it's a perfectly expected result - you are giving fsconfig(2) empty
path on a detached tree, created with OPEN_TREE_CLONE.  I.e. it *is*
an empty path in the mount tree the sucker's in.  What could d_path()
produce other than "/"?

Note, BTW that it really does create set_layers_via_fds in root (WTF?) and
running that sucker again yields a predictable fun result - mkdir() failing
with EEXIST...

IMO that kind of stuff should be dealt with by creating a temporary directory
somewhere in /tmp, mounting tmpfs on it, then doing all creations, etc.
inside that.  Then umount -l /tmp/<whatever>; rmdir /tmp/<whatever> will
clean the things up.

Re: [PATCH 1/2] mount: fix detached mount regression

[Christian Brauner]

On Sat, Jun 07, 2025 at 06:20:48AM +0100, Al Viro wrote:
> On Fri, Jun 06, 2025 at 06:45:02PM +0100, Al Viro wrote:
> > On Fri, Jun 06, 2025 at 09:58:26AM +0200, Christian Brauner wrote:
> > 
> > > Fwiw, check_mnt() is a useless name for this function that's been
> > > bothering me forever.
> > 
> > Point, but let's keep the renaming (s/check_mnt/our_mount/, for
> > example) separate.
> 
> Modified and force-pushed.
> 
> It does pass xfstests without regressions.  kselftests... AFAICS,
> no regressions either, but the damn thing is a mess.  Example:
> 
> # # set_layers_via_fds.c:711:set_layers_via_detached_mount_fds:Expected layers_found[i] (0) == true (1)
> # # set_layers_via_fds.c:39:set_layers_via_detached_mount_fds:Expected rmdir("/set_layers_via_fds") (-1) == 0 (0)
> # # set_layers_via_detached_mount_fds: Test terminated by assertion
> # #          FAIL  set_layers_via_fds.set_layers_via_detached_mount_fds
> 
> Not a regression, AFAICT; the underlying problem is that mount options
> are shown incorrectly in the tested case.  Still present after overlayfs
> merge.  mount does succeed, but... in options we see this:
> rw,relatime,lowerdir+=/,lowerdir+=/,lowerdir+=/,lowerdir+=/,datadir+=/,datadir+=/,datadir+=/,upperdir=/upper,workdir=/work,redirect_dir=on,uuid=on,metacopy=on
> 
> And it's a perfectly expected result - you are giving fsconfig(2) empty
> path on a detached tree, created with OPEN_TREE_CLONE.  I.e. it *is*
> an empty path in the mount tree the sucker's in.  What could d_path()
> produce other than "/"?

Sigh. There's no need to get all high and mighty about this. For once I
actually do write extensive selftests and they do actually catch a lot
of bugs. It's a joke how little selftests we have given the importance
of our apis. Nobody ever gives a flying fsck to review selftests when
they're posted because nobody seems to actually care.

The simple thing here to do is to point out that there's an issue in the
tests and that this should be fixed and maybe ask why.

The answer to that is that the getline checked in

        TEST_F(set_layers_via_fds, set_layers_via_detached_mount_fds)

is a simple copy-and-paste error from

        TEST_F(set_layers_via_fds, set_layers_via_fds)

that should just be removed and that's the end of that problem.

What sort of odd assumption is it that I'm not aware that a detached
tree doesn't resolve to /tmp. It's clearly a simple bug.

I actually had a patch to fix this I probably just forgot to paste it
during the merge window that added support for this.

> Note, BTW that it really does create set_layers_via_fds in root (WTF?) and
> running that sucker again yields a predictable fun result - mkdir() failing
> with EEXIST...

It's clearly a cleanup issue in FIXTURE_TEARDOWN(). Either fix it or ask
me to fix it.

> IMO that kind of stuff should be dealt with by creating a temporary directory
> somewhere in /tmp, mounting tmpfs on it, then doing all creations, etc.
> inside that.  Then umount -l /tmp/<whatever>; rmdir /tmp/<whatever> will
> clean the things up.

Sorry, that's just wishful thinking at best and out of touch with how
these apis are used. The fact that you need a private assembly in some
hidden away directory followed by a umount is a complete waste of system
calls for a start. It's also inherently unclean unless you also bring
mount namespaces into the mix. Being able to use detached mount trees is
simple and clean especially for the overlayfs layer case.

There's enough cases where you don't ever want to leak the mounts into
an actually accessible mount namespace.

Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro]

On Wed, Jun 11, 2025 at 11:36:43AM +0200, Christian Brauner wrote:

> Sigh. There's no need to get all high and mighty about this. For once I
> actually do write extensive selftests and they do actually catch a lot
> of bugs. It's a joke how little selftests we have given the importance
> of our apis. Nobody ever gives a flying fsck to review selftests when
> they're posted because nobody seems to actually care.

Not quite - for me the problem is more on the logistics side; xfstests
is a lot more convenient in that respect.  To be serious, the main
problems are
	1) many selftests have non-trivial dependencies on config
and spew a lot of noise when run on different configs.
	2) very much oriented to case when kernel tree (with build already
done) sitting on the box where they are going to be run.  Sure, I can
tar c .|ssh kvm.virt "mkdir /tmp/linux; cd /tmp/linux; tar x ." and
then make kselftest there, but it's still a headache.
	3) unlike e.g. xfstests and ltp, you don't get a convenient
summary of the entire run.

None of that is fatal, obviously, just bloody annoying to deal with at 4am...
Yes, I know how to use TARGETS, etc., but IME a test in xfstests is less
of a headache on my setup.

> > IMO that kind of stuff should be dealt with by creating a temporary directory
> > somewhere in /tmp, mounting tmpfs on it, then doing all creations, etc.
> > inside that.  Then umount -l /tmp/<whatever>; rmdir /tmp/<whatever> will
> > clean the things up.
> 
> Sorry, that's just wishful thinking at best and out of touch with how
> these apis are used. The fact that you need a private assembly in some
> hidden away directory followed by a umount is a complete waste of system
> calls for a start. It's also inherently unclean unless you also bring
> mount namespaces into the mix. Being able to use detached mount trees is
> simple and clean especially for the overlayfs layer case.

Huh?  I'm talking about the easy-of-teardown for reproducers, nothing else.
And the way I'd described above _is_ trivial to set up and tear down cleanly...

Re: [PATCH 1/2] mount: fix detached mount regression

[Christian Brauner]

On Wed, Jun 11, 2025 at 06:29:45PM +0100, Al Viro wrote:
> On Wed, Jun 11, 2025 at 11:36:43AM +0200, Christian Brauner wrote:
> 
> > Sigh. There's no need to get all high and mighty about this. For once I
> > actually do write extensive selftests and they do actually catch a lot
> > of bugs. It's a joke how little selftests we have given the importance
> > of our apis. Nobody ever gives a flying fsck to review selftests when
> > they're posted because nobody seems to actually care.
> 
> Not quite - for me the problem is more on the logistics side; xfstests
> is a lot more convenient in that respect.  To be serious, the main
> problems are
> 	1) many selftests have non-trivial dependencies on config
> and spew a lot of noise when run on different configs.
> 	2) very much oriented to case when kernel tree (with build already
> done) sitting on the box where they are going to be run.  Sure, I can
> tar c .|ssh kvm.virt "mkdir /tmp/linux; cd /tmp/linux; tar x ." and
> then make kselftest there, but it's still a headache.
> 	3) unlike e.g. xfstests and ltp, you don't get a convenient
> summary of the entire run.
> 
> None of that is fatal, obviously, just bloody annoying to deal with at 4am...

Al, please don't work until you drop from exhaustion. :D

> Yes, I know how to use TARGETS, etc., but IME a test in xfstests is less
> of a headache on my setup.

A long time ago I had the plan of moving nearly all testing including
the VFS bits into xfstests proper. After all I added the following years
ago:

brauner@so61|~/src/git/linux/fstests/xfstests-dev
> ls -al src/vfs/
total 500
drwxrwxr-x 2 brauner brauner   4096 Aug 14  2024 .
drwxrwxr-x 6 brauner brauner   8192 Nov 11  2024 ..
-rw-rw-r-- 1 brauner brauner  96274 May 27  2022 btrfs-idmapped-mounts.c
-rw-rw-r-- 1 brauner brauner    274 May 27  2022 btrfs-idmapped-mounts.h
-rw-rw-r-- 1 brauner brauner 240071 Aug 14  2024 idmapped-mounts.c
-rw-r--r-- 1 brauner brauner   3211 Feb 22  2024 idmapped-mounts.h
-rw-rw-r-- 1 brauner brauner   1070 Aug 14  2024 Makefile
-rw-rw-r-- 1 brauner brauner   3677 May 27  2022 missing.h
-rw-rw-r-- 1 brauner brauner   5163 May 27  2022 mount-idmapped.c
-rw-r--r-- 1 brauner brauner  13890 Feb 22  2024 tmpfs-idmapped-mounts.c
-rw-r--r-- 1 brauner brauner    273 Feb 22  2024 tmpfs-idmapped-mounts.h
-rw-r--r-- 1 brauner brauner  22310 Feb 22  2024 utils.c
-rw-r--r-- 1 brauner brauner  10243 Feb 22  2024 utils.h
-rw-r--r-- 1 brauner brauner  66253 Feb 22  2024 vfstest.c
-rw-r--r-- 1 brauner brauner    198 Feb 22  2024 vfstest.h

That tests the s**t out of idmapped mounts and setgid stripping for all
filesystems and a bunch of other stuff. And I had always thought that it
might be worth it to add other tests in there. But in reality adding
selftests for stuff like the mount infra is often a lot easier in the
selftests thanks to the FIXTURE_*() infrastructure that we have there.

IOW, if I have a long series and I'm at the part of "Ok, I now need to
do the boring testing part." I want it to be simple and imho that's
currently where the selftests shine.

The other part is that it all goes into the same place whereas the
selftests for xfstests would need to go into a separate project.