On Mon, Apr 28, 2025 at 07:53:18PM +0100, Al Viro wrote: > On Mon, Apr 28, 2025 at 10:50:53AM +0200, Christian Brauner wrote: > > > 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. > > Up to you; propagation calculations *are* hard-serialized (on namespace_sem) > and changing that is too much pain to consider, so I have no problem with > globals in that specific case (note several such in propagate_mnt() > machinery; that was a deliberate decision to avoid shitloads of arguments > that would have to be passed around otherwise), but... I know, I know. I've seen Ram's code and touched it enough. The globals have been very confusing at times though especially because they change across multiple function calls. But anyway. > > Anyway, minimal fix is to shift clearing the flag, as below. > Longer term I'd rather shift setting and clearing it down into > propagate_mnt() (and dropped check from propagation_would_overmount(), > with corresponding change to can_move_mount_beneath()). > > It's really "for the purposes of this mount propagation event treat all > mounts in that namespace as 'new'", so the smaller scope that thing has > the easier it is to reason about... I had a similar thought but my reasoning has been that if it's as close to the system call interface as possible then it's very very obvious where this happens and how it's cleared. IOW, the bigger scope felt actually easier in this case. > > > 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. > > FWIW, I've a series of cleanups falling out of audit of struct mount Seems good. > handling; it's still growing, but I'll post the stable parts for review > tonight or tomorrow... > > -------- > [PATCH] do_move_mount(): don't leak MNTNS_PROPAGATING on failures > > as it is, a failed move_mount(2) from anon namespace breaks > all further propagation into that namespace, including normal > mounts in non-anon namespaces that would otherwise propagate > there. > > Fixes: 064fe6e233e8 "mount: handle mount propagation for detached mount trees" v6.15+ > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > --- > diff --git a/fs/namespace.c b/fs/namespace.c > index eba4748388b1..8b8348ee5a55 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -3714,15 +3714,14 @@ 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); > if (attached) > put_mountpoint(old_mp); > out: > + if (is_anon_ns(ns)) > + ns->mntns_flags &= ~MNTNS_PROPAGATING; Thanks!
