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

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

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...