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

[Kirill Tkhai]

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.

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