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...
