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.
