Re: [REVIEW][PATCH 1/2] mnt: Fix propagate_mount_busy to notice all cases of busy mounts.

[Ram Pai <linuxram@us.ibm.com>]

On Thu, Jan 12, 2017 at 05:18:12AM +1300, Eric W. Biederman wrote:
> 
> When I look at what propagate_mount_busy is trying to do and I look
> at the code closely I discover there is a great disconnect between the
> two.  In the ordinary non-propagation case propagate_mount_busy has
> been verifying that there are no submounts and that there are no
> extraneous references on the mount.
> 
> For mounts that the unmount would propagate to propagate_mount_busy has
> been verifying that there are no extraneous references only if there
> are no submounts.  Which is nonsense.


the reason why we had to do it that way was because there were
situations where it was impossible to umount anything...

take for example.

(1) mount --make-shared A

(2) mount --bind A  A/a    

The tree looks like this

 	A
	|
        B

(3) mount --bind A  B/a    
The tree looks like this
 	A
	|
 	B B'   (B' becomes a shadow mount)
	|
        C


(4) mount --make-slave A
	At this point B and C are peers and A is a slave.

(5) umount B' 
	NOTE: This used to be possible a decade ago if the process doing
	the umount had access to its dentry.
    The tree looks like this
 	A
	|
 	B
	|
        C

Now if you try to unmount C,  it becomes impossible, reason being...

B is the parent of C.
So the umount propagates to A.  But A has B mounted at the same
location.  But B is busy since it has got a child C.
So the entire umount has to fail.  There is no way to umount it all.
Kind of stuck for ever.  That is the reason; in those days a decade ago,
we relaxed the rule to let go propagated mounts that had children.

The above example is a simplest case that demonstrates the phenomenon.

Given that, the current code does not allow any process to reach shadow
mount B' and given that we are getting rid of shadow mounts, I think we
should allow the code changes you propose in this patch.

RP

	
> 
> Thefore rework the logic in propgate_mount_busy so that for each
> mount it examines it considers that mount busy if that mount has
> children or if there are extraneous references to that mount.
> 
> While this check was incorrect we could leak mounts instead of simply
> failing umount.
> 
> Cc: stable@vger.kernel.org
> Fixes: a05964f3917c ("[PATCH] shared mounts handling: umount")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> 
> If you don't figure this fix is worth it after all of this time please
> let me know.  This feels like the proper thing to do, and I don't expect
> it will break anyone to fix this.
> 
>  fs/pnode.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 06a793f4ae38..12fafa711114 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -344,7 +344,6 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
>  {
>  	struct mount *m, *child;
>  	struct mount *parent = mnt->mnt_parent;
> -	int ret = 0;
> 
>  	if (mnt == parent)
>  		return do_refcount_check(mnt, refcnt);
> @@ -360,11 +359,13 @@ 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);
> -		if (child && list_empty(&child->mnt_mounts) &&
> -		    (ret = do_refcount_check(child, 1)))
> -			break;
> +		if (!child)
> +			continue;
> +		if (!list_empty(&child->mnt_mounts) ||
> +		    do_refcount_check(child, 1))
> +			return 1;
>  	}
> -	return ret;
> +	return 0;
>  }
> 
>  /*
> -- 
> 2.10.1

-- 
Ram Pai