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@xxxxxxxxxx>
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> ---
> 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?
