Re: [BUG] propagate_umount() breakage

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

On Sun, May 11, 2025 at 11:50:40PM -0500, Eric W. Biederman wrote:

> This is there be dragons talk.
> 
> With out care it is easy to get the code to go non-linear in
> the number of mounts.
> 
> That said I don't see any immediate problem with a first pass
> without my __propgate_umount.
> 
> As I read the current code the __propagate_umount loop is just
> about propagating the information up from the leaves.
> 
> > After the set is collected, we could go through it, doing the
> > something along the lines of
> > 	how = 0
> > 	for each child in children(m)
> > 		if child in set
> > 			continue
> > 		how = 1
> > 		if child is not mounted on root
> > 			how = 2
> > 			break
> > 	if how == 2
> > 		kick_out_ancestors(m)
> > 		remove m itself from set // needs to cooperate with outer loop
> > 	else if how == 1
> > 		for (p = m; p in set && p is mounted on root; p = p->mnt_parent)
> > 			;
> > 		if p in set
> > 			kick_out_ancestors(p)
> > 	else if children(m) is empty && m is not locked	// to optimize things a bit
> > 		commit to unmounting m (again, needs to cooperate with the outer loop)

Misses some valid candidates, unfortunately.  It's not hard to fix -
handle_overmounts(m)
        remove_it = false;
        non_vanishing = NULL; // something non-vanishing there
        for each child in m->mnt_mounts
                if child not in Candidates
                        non_vanishing = child;
                        if (!overmounts_root(child)) {
                                remove_it = true;
                                break;
                        }
        if (non_vanishing) {
                for (p = m; p in Candidates && !marked(p); p = p->mnt_parent) {
                        if (!overmounts_root(non_vanishing) && p != m)
                                Candidates -= {p}
			else
				mark(p);
                        non_vanishing = p;
                }
        } else if m->mnt_mounts is empty && m is not locked { // optimize a bit
                commit_to_unmounting(m);
                remove_it = true;
        }
        res = next(m) // NULL if at the end of Candidates
        if (remove_it) {
		unmark(m);
                Candidates -= {m}
	}
        return res
will do the right thing and it's linear by the number of mounts we must
look at.

And yes, it's going to be posted along with the proof of correctness -
I've had it with the amount of subtle corner cases in that area ;-/