[CRIU] [PATCH] mount: Umount nested temporary mount pathes in do_bind_mount()

[Andrew Vagin]

On Fri, Feb 26, 2016 at 01:43:13PM +0300, Kirill Tkhai wrote:
> Hi, Andrew,
> 
> On 26.02.2016 03:47, Andrew Vagin wrote:
> > On Thu, Feb 25, 2016 at 04:54:27PM +0300, Kirill Tkhai wrote:
> >> When a parent directory of restored mount is bound to itself,
> >> a temporary mount is cloned, so we need to umount it twice:
> >>
> >> # mkdir -p parent/child /tmp/cr-XXX
> >> # mount --bind parent parent
> >> # mount --bind parent/child /tmp/cr-XXX/
> >> # mount --bind /tmp/cr-XXX/ parent/child
> >> # umount /tmp/cr-XXX/
> >> # mount | grep "/tmp/cr-XXX"
> >> /dev/sda3 on /tmp/cr-XXX type ext4 (rw,noatime,nodiratime,data=ordered)
> >>
> >> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
> >> ---
> >>  criu/mount.c |   27 ++++++++++++++++++++++++++-
> >>  1 file changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/criu/mount.c b/criu/mount.c
> >> index 1a6b034..bce2465 100644
> >> --- a/criu/mount.c
> >> +++ b/criu/mount.c
> >> @@ -301,6 +301,26 @@ static bool mounts_equal(struct mount_info *a, struct mount_info *b)
> >>  }
> >>  
> >>  /*
> >> + * Successively umount all file systems mounted on @path
> >> + */
> >> +static int umount2_nested(const char *path, int flags)
> >> +{
> >> +	bool once = true;
> >> +	int ret;
> >> +
> >> +	while (1) {
> >> +		ret = umount2(path, flags);
> >> +		if (ret)
> >> +			break;
> >> +		once = false;
> >> +	}
> >> +
> >> +	if (once || errno != EINVAL)
> >> +		return -1;
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >>   * mnt_roots is a temporary directory for restoring sub-trees of
> >>   * non-root namespaces.
> >>   */
> >> @@ -2397,7 +2417,12 @@ static int do_bind_mount(struct mount_info *mi)
> >  if (mount(NULL, mnt_path, NULL, MS_PRIVATE, NULL)) {
> > 
> > I think we don't need to make it private ^^^^, because it may be
> > propagated to somewhere. Kirll, could you try to remove your changes and
> > this mount and check that everythink works as expected in this case.
> 
> No, it does not help. MS_PRIVATE would affect on child mounts, while the problem
> is with parent. Try this example
> 
> # mkdir -p parent/child /tmp/cr-XXX
> # mount --bind parent parent
> # mount --bind parent/child /tmp/cr-XXX/
> # mount --bind /tmp/cr-XXX/ parent/child
> # mount
> /dev/sda2 on /tmp/cr-XXX type ext4 (rw,relatime,data=ordered)
> /dev/sda2 on /root/parent/child type ext4 (rw,relatime,data=ordered)
> /dev/sda2 on /tmp/cr-XXX type ext4 (rw,relatime,data=ordered)
> /dev/sda2 on /root/parent/child type ext4 (rw,relatime,data=ordered)
> 
> Nothing changes if you modify /tmp/cr-XXX or /root/parent/child
> subtree type now.
> 
> The only different solution in our example is to make ./parent MS_PRIVATE
> right after it's bound to itself. Common solution will require to traverse
> to top of the tree and check every parent subtree type, and do it private,
> if it's need, and after all to return everything back. But it's too complex.

You can't recreate shared dependences for mounts.

The patch is incorrect, because "umount /tmp/cr-XXX" will umount
/tmp/cr-XXX and parent/child

[root at fc22-vm tmp]# bash -x test.sh 
+ mkdir -p parent/child /tmp/cr-XXX
+ mount --bind parent parent
+ mount --bind parent/child /tmp/cr-XXX/
+ mount --bind /tmp/cr-XXX/ parent/child
+ mount --make-private /tmp/cr-XXX/
+ umount /tmp/cr-XXX/
+ umount /tmp/cr-XXX/
+ mount
+ grep parent
/dev/vda2 on /root/tmp/parent type ext4 (rw,relatime,data=ordered)

[root at fc22-vm tmp]# cat test.sh 
mkdir -p parent/child /tmp/cr-XXX
mount --bind parent parent
mount --bind parent/child /tmp/cr-XXX/
mount --bind /tmp/cr-XXX/ parent/child
mount --make-private /tmp/cr-XXX/
umount /tmp/cr-XXX/
umount /tmp/cr-XXX/
mount | grep parent

I really recomend to write tests for such sort of problems.

I don't know how to fix this problem. Will think...

Let me know if you have any ideas.
> 
> Regards,
> Kirill
> >>  			pr_perror("Unable to make %s private", mnt_path);
> >>  			return -1;
> >>  		}
> >> -		if (umount2(mnt_path, MNT_DETACH)) {
> >> +		/*
> >> +		 * If mi and mi->bind have the same mountpoint, and
> >> +		 * a parent dir of the mountpoint is bound to itself,
> >> +		 * then we have several mounts on mnt_path.
> >> +		 */
> >> +		if (umount2_nested(mnt_path, MNT_DETACH)) {
> >>  			pr_perror("Unable to umount %s", mnt_path);
> >>  			return -1;
> >>  		}
> >>
> >> _______________________________________________
> >> CRIU mailing list
> >> CRIU at openvz.org
> >> https://lists.openvz.org/mailman/listinfo/criu