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