Re: [RFC][CFT][PATCH] Rewrite of propagate_umount() (was Re: [BUG] propagate_umount() breakage)

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

On Tue, May 20, 2025 at 01:10:24PM +0200, Christian Brauner wrote:
> > +It is convenient to define several properties of sets of mounts:
> > +
> > +1) A set S of mounts is non-shifting if for any mount X belonging
> > +to S all subtrees mounted strictly inside of X (i.e. not overmounting
> > +the root of X) contain only elements of S.
> 
> I think "shifting" is misleading. I would suggest either "isolated" or
> "contained" or ideally "closed" which would mean...

Umm...  I'm not sure.  "Shifting" in a sense that pulling that set out
and reparenting everything that remains to the nearest surviving ancestor
won't change the pathnames.  "Contained" or "isolated"... what would
that be about?

> > +of that set, but only on top of stacks of root-overmounting elements
> > +of set.  They can be reparented to the place where the bottom of
> > +stack is attached to a mount that will survive.  NOTE: doing that
> > +will violate a constraint on having no more than one mount with
> > +the same parent/mountpoint pair; however, the caller (umount_tree())
> 
> I would prefer if this would insert the term "shadow mounts" since
> that's what we've traditionally used for that.

There's a bit of ambiguity - if we have done

mount -t tmpfs none /tmp/foo
touch /tmp/foo/A
mount -t tmpfs none /tmp/foo
touch /tmp/foo/B

we have two mounts, one overmounting the root of another.  Does "shadow"
apply to the lower (with A on it) or the upper (with B on it)?

> > +{
> > +	while (1) {
> > +		struct mount *master = m->mnt_master;
> > +
> > +		if (master == origin->mnt_master) {
> > +			struct mount *next = next_peer(m);
> > +			return (next == origin) ? NULL : next;
> > +		} else if (m->mnt_slave.next != &master->mnt_slave_list)
> > +			return next_slave(m);
> 
> Please add a comment to that helper that explains how it walks the
> propagation tree. I remember having to fix bugs in that code and the
> lack of comments was noticable.

Ugh...  Let's separate that - it's not specific to propagate_umount()
and the helper is the "we hadn't gone into ->mnt_slave_list" half of
propagation_next(), verbatim.

I agree that comments there would be a good thing, but it (and next_group())
belong to different layer - how do we walk the propagation graph.

FWIW, the current variant of that thing (which seems to survive the tests
so far) already has a plenty in it; let's try to keep at least some parts
in separate commits...