Re: [REVIEW][PATCH] mnt: Tuck mounts under others instead of creating shadow/side mounts.

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

On Thu, Jan 12, 2017 at 05:03:42AM +1300, Eric W. Biederman wrote:
> Al Viro <viro@ZenIV.linux.org.uk> writes:
> 
> > On Wed, Jan 11, 2017 at 01:10:57PM +1300, Eric W. Biederman wrote:
> >> >> +		if (child->mnt.mnt_root == smp->m_dentry) {
> >> >
> >> > Explain, please.  In which case is that condition _not_ satisfied, and
> >> > what should happen i
> >> 
> >> When a tree is grafted in that condition does not apply to the lower
> >> leaves of the tree.  At the same time nothing needs to be done for those
> >> leaves.  Only the primary mountpoint needs to worry about tucking.
> >
> > 	How in hell would those lower leaves end up on the list in
> > attach_recursive_mnt()?  IDGI...
> 
> The submounts of a mount tree that is being attached need to have
> commit_tree called on them to attach them to a mount namespace.

Huh?  commit_tree() is called once for each copy of the source tree.  This
        list_add_tail(&head, &mnt->mnt_list);
        list_for_each_entry(m, &head, mnt_list)
                m->mnt_ns = n;
is what goes through submounts in each of them, _not_ the loop in the caller.

What we get out of propagate_mnt() is the list of copies of source tree,
one for each of the mountpoints that should get propagation from the
target.
	->mnt_mountpoint/->mnt_parent is fully set for all nodes.
	Everything except the roots of those trees is hashed and
has ->mnt_child set up.
	->mnt_hash of the roots of those copies host the cyclic list,
anchored in tree_list passed to propagate_mnt().
	->mnt_list in each copy forms an unanchored cyclic list
going through all mounts in that copy.

The loop in attach_recursive_mnt() takes the tree_list apart and for each
element (== each copy of source tree) we have commit_tree() called once,
doing the remaining work:
	* splices the ->mnt_list into the namespace's mount list
	* sets ->mnt_ns for all nodes (root and submounts alike)
	* sets ->mnt_child and ->mnt_hash for the root.

Again, the loop in attach_recursive_mnt() is over the set of secondary
copies of the source tree; it goes *only* through their roots.  Submounts
are seen only by commit_tree(), in the list_for_each_entry() loop in
that function.  Hell, try to add else WARN_ON(1); to that if (...) of yours
and see if you can trigger it if you don't believe the above...

> > You have "if mount is not locked - mark it; if mount is already marked -
> > mark it again".  The latter part (|| IS_MNT_MARKED(mnt), that is) looks
> > very odd, won't you agree?  What the hell was that (its counterpart in
> > the earlier code) about?
> 
> Not mark it again.  If the parent is marked mark the child.

*doh*