Re: [REVIEW][PATCH 2/2] 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:19:34AM +1300, Eric W. Biederman wrote:

> +		if (child->mnt.mnt_root == smp->m_dentry) {
> +			struct mount *q;
> +			q = __lookup_mnt(&child->mnt_parent->mnt,
> +					 child->mnt_mountpoint);
> +			if (q)
> +				mnt_change_mountpoint(child, smp, q);
> +		}

This is wrong; condition will be true for *all* mounts seen by that loop.  
Feel free to add else WARN_ON(1); to the line above and try to trigger
it.  You are misinterpreting what propagate_mnt() and commit_tree() are
doing - the loop in commit_tree() goes through the submounts and sets ->mnt_ns
on those.  The of the fields is already set up by that point.  For roots
of those copies we need to set ->mnt_hash/->mnt_child as well, but for
all submounts it's already been done by copy_tree().  Again, commit_tree()
is called once per secondary copy of source tree, not once per created
mount.

> +static struct mount *find_topper(struct mount *mnt)
> +{
> +	/* If there is exactly one mount covering mnt completely return it. */
> +	struct mount *child;
> +
> +	if (!list_is_singular(&mnt->mnt_mounts))
> +		return NULL;
> +
> +	child = list_first_entry(&mnt->mnt_mounts, struct mount, mnt_child);
> +	if (child->mnt_parent != mnt ||

The first part can't happen.  Turn that into WARN_ON(child->mnt_parent != mnt)
if you wish, but that never occurs unless the data structures are corrupted.

> +	    child->mnt_mountpoint != mnt->mnt.mnt_root)
> +		return NULL;

> @@ -342,7 +358,7 @@ static inline int do_refcount_check(struct mount *mnt, int count)
>   */
>  int propagate_mount_busy(struct mount *mnt, int refcnt)
>  {
> -	struct mount *m, *child;
> +	struct mount *m, *child, *topper;
>  	struct mount *parent = mnt->mnt_parent;
>  
>  	if (mnt == parent)
> @@ -358,11 +374,21 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
>  
>  	for (m = propagation_next(parent, parent); m;
>  	     		m = propagation_next(m, parent)) {
> -		child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
> +		int count = 1;
> +		child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
>  		if (!child)
>  			continue;
> -		if (!list_empty(&child->mnt_mounts) ||
> -		    do_refcount_check(child, 1))
> +
> +		/* Is there exactly one mount on the child that covers
> +		 * it completely whose reference should be ignored?
> +		 */
> +		topper = find_topper(child);
> +		if (topper)
> +			count += 1;
> +		else if (!list_empty(&child->mnt_mounts))
> +			return 1;
> +
> +		if (do_refcount_check(child, count))
>  			return 1;

Again, subject to the comments re semantics change (see the reply to previous
patch).

> @@ -431,6 +459,15 @@ static void __propagate_umount(struct mount *mnt)
>  		if (!child || !IS_MNT_MARKED(child))
>  			continue;
>  		CLEAR_MNT_MARK(child);
> +
> +		/* If there is exactly one mount covering all of child
> +		 * replace child with that mount.
> +		 */
> +		topper = find_topper(child);
> +		if (topper)
> +			mnt_change_mountpoint(child->mnt_parent, child->mnt_mp,
> +					      topper);
> +
>  		if (list_empty(&child->mnt_mounts)) {
>  			list_del_init(&child->mnt_child);
>  			child->mnt.mnt_flags |= MNT_UMOUNT;

Umm...  With fallthrough from "is completely overmounted" case?  And
I'm not sure I understand what that list_empty() is doing there after
your previous semantics change - how _can_ we reach that point with
non-empty ->mnt_mounts now?