Re: [PATCH 1/2] mount: fix detached mount regression

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Jun 06, 2025 at 09:58:26AM +0200, Christian Brauner wrote:

> Fwiw, check_mnt() is a useless name for this function that's been
> bothering me forever.

Point, but let's keep the renaming (s/check_mnt/our_mount/, for
example) separate.

> > +	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;

Maybe, but I wonder if we should add
static inline bool anon_ns_root(struct mount *m)
{
	struct namespace *ns = m->mnt_ns;

	return !IS_ERR_OR_NULL(ns) && is_anon_ns(ns) && m == ns->root;
}
for that kind of stuff...  And I'd prefer direct comparison with root
instead of going for "well, if it's mounted in a namespace, it either
is its root or mnt_has_parent() will return true" kind of logics.

We have checks for that predicate open-coded elsewhere...

<looks>

in addition to do_move_mount() we have clone_private_mount() where
                if (!is_mounted(&old_mnt->mnt) ||
                        !is_anon_ns(old_mnt->mnt_ns) ||
                        mnt_has_parent(old_mnt))
would be replacable with
		if (!anon_ns_root(old_mnt))
and in do_mount_setattr()
        if (!is_mounted(&mnt->mnt))
                goto out;
        if ((mnt_has_parent(mnt) || !is_anon_ns(mnt->mnt_ns)) && !check_mnt(mnt))
                goto out;
with
	if (!check_mnt(mnt) && !anon_ns_root(mnt))
		goto out;

and while we are at it, dissolve_on_fput() becomes
        scoped_guard(rcu) {
                if (!anon_ns_root(m))
                        return;
        }
        scoped_guard(namespace_lock, &namespace_sem) {
                ns = m->mnt_ns;
                if (!anon_ns_root(m))
                        return;
                lock_mount_hash();
                umount_tree(m, UMOUNT_CONNECTED);
                unlock_mount_hash();
	}
        free_mnt_ns(ns);
Incidentally, location of free_mnt_ns() is one aspect of that thing
that *does* need a comment - at the first glance it looks like it
would be better off under namespace_sem.  The reason why that must
not be moved there is well-buried - it's the call of notify_mnt_list()
in namespace_unlock(), where we have calls of mnt_notify(), which
looks at m->prev_ns->n_fsnotify_marks.  IMO that's way too subtle
to be left without an explanation - that, or having something like
bury_anon_ns(ns) called under namespace_sem and triggering
free_mnt_ns() from namespace_unlock(), after the notify_mnt_list()
call there.  do_move_mount() might also benefit from the same...
Anyway, that's a separate story.