[criu-stable,1/3] mount: don't lose shared options for bind mounts

Submitted by Pavel Tikhomirov on June 25, 2018, 2:39 p.m.

Details

Message ID 20180625143955.25435-1-ptikhomirov@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Pavel Tikhomirov June 25, 2018, 2:39 p.m.
When we bind-mount something as a child of shared group, child will be
created shared, so we should fix sharing flags after bind but not
before.

Bigger problem we face is: when some mount is wrongly created shared
instead of private, it's children can be duplicated, and migration may 
hit container limit on number of mounts, after couple of iterations.

https://jira.sw.ru/browse/PSBM-85251

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/mount.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index c745932c6..ffe11a22b 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -2093,24 +2093,6 @@  static char *resolve_source(struct mount_info *mi)
 	return NULL;
 }
 
-static int __restore_shared_options(char *mountpoint, bool private, bool shared, bool slave)
-{
-	if (private && mount(NULL, mountpoint, NULL, MS_PRIVATE, NULL)) {
-		pr_perror("Unable to make %s private", mountpoint);
-		return -1;
-	}
-	if (slave && mount(NULL, mountpoint, NULL, MS_SLAVE, NULL)) {
-		pr_perror("Unable to make %s slave", mountpoint);
-		return -1;
-	}
-	if (shared && mount(NULL, mountpoint, NULL, MS_SHARED, NULL)) {
-		pr_perror("Unable to make %s shared", mountpoint);
-		return -1;
-	}
-
-	return 0;
-}
-
 static int restore_shared_options(struct mount_info *mi, bool private, bool shared, bool slave)
 {
 	pr_debug("%d:%s private %d shared %d slave %d\n",
@@ -2123,7 +2105,20 @@  static int restore_shared_options(struct mount_info *mi, bool private, bool shar
 			return mount(NULL, mi->mountpoint, NULL, MS_UNBINDABLE, NULL);
 	}
 
-	return __restore_shared_options(mi->mountpoint, private, shared, slave);
+	if (private && mount(NULL, mi->mountpoint, NULL, MS_PRIVATE, NULL)) {
+		pr_perror("Unable to make %s private", mi->mountpoint);
+		return -1;
+	}
+	if (slave && mount(NULL, mi->mountpoint, NULL, MS_SLAVE, NULL)) {
+		pr_perror("Unable to make %s slave", mi->mountpoint);
+		return -1;
+	}
+	if (shared && mount(NULL, mi->mountpoint, NULL, MS_SHARED, NULL)) {
+		pr_perror("Unable to make %s shared", mi->mountpoint);
+		return -1;
+	}
+
+	return 0;
 }
 
 /*
@@ -2534,10 +2529,6 @@  static int do_bind_mount(struct mount_info *mi)
 		pr_perror("Unable to open %s", mnt_clean_path);
 		return -1;
 	}
-	if (__restore_shared_options(mnt_path, private,
-				   mi->shared_id && !shared,
-				   mi->master_id && !master))
-		return -1;
 
 	if (mnt_path == NULL)
 		return -1;
@@ -2619,6 +2610,11 @@  static int do_bind_mount(struct mount_info *mi)
 		}
 	}
 out:
+	if (restore_shared_options(mi, private,
+				   mi->shared_id && !shared,
+				   mi->master_id && !master))
+		return -1;
+
 	mi->mounted = true;
 	exit_code = 0;
 err:

Comments

Andrei Vagin June 25, 2018, 7:15 p.m.
Do we need these patches for the upstram criu?

On Mon, Jun 25, 2018 at 05:39:53PM +0300, Pavel Tikhomirov wrote:
> When we bind-mount something as a child of shared group, child will be
> created shared, so we should fix sharing flags after bind but not
> before.
> 
> Bigger problem we face is: when some mount is wrongly created shared
> instead of private, it's children can be duplicated, and migration may 
> hit container limit on number of mounts, after couple of iterations.
> 
> https://jira.sw.ru/browse/PSBM-85251
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/mount.c | 42 +++++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index c745932c6..ffe11a22b 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -2093,24 +2093,6 @@ static char *resolve_source(struct mount_info *mi)
>  	return NULL;
>  }
>  
> -static int __restore_shared_options(char *mountpoint, bool private, bool shared, bool slave)
> -{
> -	if (private && mount(NULL, mountpoint, NULL, MS_PRIVATE, NULL)) {
> -		pr_perror("Unable to make %s private", mountpoint);
> -		return -1;
> -	}
> -	if (slave && mount(NULL, mountpoint, NULL, MS_SLAVE, NULL)) {
> -		pr_perror("Unable to make %s slave", mountpoint);
> -		return -1;
> -	}
> -	if (shared && mount(NULL, mountpoint, NULL, MS_SHARED, NULL)) {
> -		pr_perror("Unable to make %s shared", mountpoint);
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
>  static int restore_shared_options(struct mount_info *mi, bool private, bool shared, bool slave)
>  {
>  	pr_debug("%d:%s private %d shared %d slave %d\n",
> @@ -2123,7 +2105,20 @@ static int restore_shared_options(struct mount_info *mi, bool private, bool shar
>  			return mount(NULL, mi->mountpoint, NULL, MS_UNBINDABLE, NULL);
>  	}
>  
> -	return __restore_shared_options(mi->mountpoint, private, shared, slave);
> +	if (private && mount(NULL, mi->mountpoint, NULL, MS_PRIVATE, NULL)) {
> +		pr_perror("Unable to make %s private", mi->mountpoint);
> +		return -1;
> +	}
> +	if (slave && mount(NULL, mi->mountpoint, NULL, MS_SLAVE, NULL)) {
> +		pr_perror("Unable to make %s slave", mi->mountpoint);
> +		return -1;
> +	}
> +	if (shared && mount(NULL, mi->mountpoint, NULL, MS_SHARED, NULL)) {
> +		pr_perror("Unable to make %s shared", mi->mountpoint);
> +		return -1;
> +	}
> +
> +	return 0;
>  }
>  
>  /*
> @@ -2534,10 +2529,6 @@ static int do_bind_mount(struct mount_info *mi)
>  		pr_perror("Unable to open %s", mnt_clean_path);
>  		return -1;
>  	}
> -	if (__restore_shared_options(mnt_path, private,
> -				   mi->shared_id && !shared,
> -				   mi->master_id && !master))
> -		return -1;
>  
>  	if (mnt_path == NULL)
>  		return -1;
> @@ -2619,6 +2610,11 @@ static int do_bind_mount(struct mount_info *mi)
>  		}
>  	}
>  out:
> +	if (restore_shared_options(mi, private,
> +				   mi->shared_id && !shared,
> +				   mi->master_id && !master))
> +		return -1;
> +
>  	mi->mounted = true;
>  	exit_code = 0;
>  err:
> -- 
> 2.17.0
>
Pavel Tikhomirov June 26, 2018, 7:12 a.m.
On 06/25/2018 10:15 PM, Andrew Vagin wrote:
> Do we need these patches for the upstram criu?
> 

I've already sent last two patches with zdtm-test upstream separately, 
we need those two.

Patch 1/3 is only applicable to criu-stable as it has a commit 
b9d19bbead41 ("vz7: mount: create a temporary mount to restore shared 
and master groups") which breaks these case.