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@xxxxxxxxxxxxxxxxxx>
> ---
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@xxxxxxxxxx>
> 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;
>
