Re: [RFC] move_mount(2): still breakage around new mount detection

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

On Tue, Apr 29, 2025 at 06:10:54AM +0100, Al Viro wrote:
> On Tue, Apr 29, 2025 at 05:03:58AM +0100, Al Viro wrote:
> > On Mon, Apr 28, 2025 at 07:53:18PM +0100, Al Viro wrote:
> > 
> > > FWIW, I've a series of cleanups falling out of audit of struct mount
> > > handling; it's still growing, but I'll post the stable parts for review
> > > tonight or tomorrow...
> > 
> > _Another_ fun one, this time around do_umount().
> 
> ... and more, from 620c266f3949 "fhandle: relax open_by_handle_at()
> permission checks" - just what is protecting has_locked_children()
> use there?  We are, after all, iterating through ->mnt_mounts -
> with no locks whatsoever.  Not to mention the fun question regarding
> the result (including the bits sensitive to is_mounted()) remaining
> valid by the time you get through exportfs_decode_fh_raw() (and no,
> you can't hold any namespace_sem over it - no IO allowed under
> that, so we'll need to recheck after that point)...

FWIW, looking at do_move_mounts(): some of the tests look odd.

        if (is_anon_ns(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.
                 */
                if ((is_anon_ns(p->mnt_ns) && ns == p->mnt_ns))
                        goto out;

Why are we checking is_anon_ns(p->mnt_ns) here?  If ns is equal
to p->mnt_ns, we have just verified that is_anon_ns() is true for it;
if it is not, there's no point bothering with is_anon_ns() since
conjunction is false anyway.  And it's not as if that comparison
had been unsafe to calculate if is_anon_ns(p->mnt_ns) is false...

Looks really really confusing - is there a typo somewhere?  Why
not simply
        if (is_anon_ns(ns)) {
		/*
		 * Can't move the root of namespace into the same
		 * namespace.  Reject that early.
		 */
		if (ns == p->mnt)
			goto out;
What am I missing here?

Another odd thing: what's the point rejecting move of /foo/bar/baz/ beneath
/foo?  What's wrong with doing that?  _IF_ that's really intended, it needs
at least a comment spelling that out.  TBH, for quite a while I'd been
staring at that wondering WTF do you duplicate the common check for target
not being a descendent of source, but with different error value.  Until
spotting that the check is about _source_ being a descendent of target
rather than the other way round...