On Mon, Apr 28, 2025 at 08:03:53AM +0100, Al Viro wrote:
> On Mon, Apr 28, 2025 at 07:30:56AM +0100, Al Viro wrote:
> > have at most one ns so marked, right? And we only care about it in
> > propagate_mnt(), where we are serialized under namespace_lock.
> > So why not simply remember the anon_ns we would've marked and compare
> > ->mnt_ns with it instead of dereferencing and checking for flag?
> >
> > IOW, what's wrong with the following?
>
> Hmm... You also have propagation_would_overmount() (from
> can_move_mount_beneath()) checking it... IDGI.
>
> For that predicate to trigger in there you need source
> anon ns - you won't see NULL ->mnt_ns there. So...
> mnt_from is the absolute root of anon ns, target is
> *not* in that anon ns (either it's in our current namespace,
> or in a different anon ns). IOW, in
> if (propagation_would_overmount(parent_mnt_to, mnt_to, mp))
> return -EINVAL;
> IS_MNT_PROPAGATED() will be false (mnt_to has unmarked namespace)
> and in
> if (propagation_would_overmount(parent_mnt_to, mnt_from, mp))
> return -EINVAL;
> IS_MNT_PROPAGATED() is true. So basically, we can drop that
> check inf propagation_would_overmount() and take it to
> can_move_mount_beneath(), turning the second check into
> if (check_mnt(mnt_from) &&
> propagation_would_overmount(parent_mnt_to, mnt_from, mp))
> return -EINVAL;
> since mnt_from is either ours or root of anon and the check removed
> from propagation_would_overmount() had it return false in "mnt_from
> is root of anon" case.
>
> And we obviously need it cleared at the end of propagate_mnt(),
> yielding the patch below. Do you see any other problems?
>
> diff --git a/fs/mount.h b/fs/mount.h
> index 7aecf2a60472..ad7173037924 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -7,10 +7,6 @@
>
> extern struct list_head notify_list;
>
> -typedef __u32 __bitwise mntns_flags_t;
> -
> -#define MNTNS_PROPAGATING ((__force mntns_flags_t)(1 << 0))
> -
> struct mnt_namespace {
> struct ns_common ns;
> struct mount * root;
> @@ -37,7 +33,6 @@ struct mnt_namespace {
> struct rb_node mnt_ns_tree_node; /* node in the mnt_ns_tree */
> struct list_head mnt_ns_list; /* entry in the sequential list of mounts namespace */
> refcount_t passive; /* number references not pinning @mounts */
> - mntns_flags_t mntns_flags;
> } __randomize_layout;
>
> struct mnt_pcp {
> diff --git a/fs/namespace.c b/fs/namespace.c
> index eba4748388b1..3061f1b04d4c 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3556,7 +3556,8 @@ static int can_move_mount_beneath(const struct path *from,
> * @mnt_from itself. This defeats the whole purpose of mounting
> * @mnt_from beneath @mnt_to.
> */
> - if (propagation_would_overmount(parent_mnt_to, mnt_from, mp))
> + if (check_mnt(mnt_from) &&
> + propagation_would_overmount(parent_mnt_to, mnt_from, mp))
> return -EINVAL;
>
> return 0;
> @@ -3656,14 +3657,6 @@ static int do_move_mount(struct path *old_path,
> */
> if ((is_anon_ns(p->mnt_ns) && ns == p->mnt_ns))
> goto out;
> -
> - /*
> - * If this is an anonymous mount tree ensure that mount
> - * propagation can detect mounts that were just
> - * propagated to the target mount tree so we don't
> - * propagate onto them.
> - */
> - ns->mntns_flags |= MNTNS_PROPAGATING;
> } else if (is_anon_ns(p->mnt_ns)) {
> /*
> * Don't allow moving an attached mount tree to an
> @@ -3714,9 +3707,6 @@ static int do_move_mount(struct path *old_path,
> if (err)
> goto out;
>
> - if (is_anon_ns(ns))
> - ns->mntns_flags &= ~MNTNS_PROPAGATING;
> -
> /* if the mount is moved, it should no longer be expire
> * automatically */
> list_del_init(&old->mnt_expire);
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 7a062a5de10e..26d0482fe017 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -13,6 +13,18 @@
> #include "internal.h"
> #include "pnode.h"
>
> +static struct mnt_namespace *source_anon;
> +static inline bool IS_MNT_PROPAGATED(const struct mount *m)
> +{
> + /*
> + * If this is an anonymous mount tree ensure that mount
> + * propagation can detect mounts that were just
> + * propagated to the target mount tree so we don't
> + * propagate onto them.
> + */
> + return !m->mnt_ns || m->mnt_ns == source_anon;
> +}
> +
> /* return the next shared peer mount of @p */
> static inline struct mount *next_peer(struct mount *p)
> {
> @@ -300,6 +312,9 @@ int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
> last_source = source_mnt;
> list = tree_list;
> dest_master = dest_mnt->mnt_master;
> + source_anon = source_mnt->mnt_ns;
> + if (source_anon && !is_anon_ns(source_anon))
> + source_anon = NULL;
>
> /* all peers of dest_mnt, except dest_mnt itself */
> for (n = next_peer(dest_mnt); n != dest_mnt; n = next_peer(n)) {
> @@ -328,6 +343,7 @@ int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
> CLEAR_MNT_MARK(m->mnt_master);
> }
> read_sequnlock_excl(&mount_lock);
> + source_anon = NULL;
I'm not fond of the global variable. I would generally agree with you if
that were really performance sensitive but this really isn't.
I'll have more uses for the flags member very soon as I will make it
possible to list mounts in anonymous mount namespaces because it
confuses userspace to no end that they can't list detached mount trees.
So anonymous mount namespaces will simply get a mount namespace id just
like any other mount namespace and simply be discerned by a flag.
Thanks for going through this. I appreciate it.
The check_mnt() simplification is good though.
