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

[Al Viro <viro@zeniv.linux.org.uk>]

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?