[3/3] mount: cr_pivot_root -- Try temp directory first

Submitted by Cyrill Gorcunov on Dec. 7, 2018, 12:43 p.m.

Details

Message ID 20181207124331.30751-4-gorcunov@gmail.com
State New
Series "mount: Fix the case where tmp directory in already mounted"
Headers show

Commit Message

Cyrill Gorcunov Dec. 7, 2018, 12:43 p.m.
The directory we're pinning old root might be a bind mount
point itself. So we should try with temp location first
and create own fresh new directory and only if it fails
start to use /tmp.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/mount.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index b8a564d2ce92..451d9c965cea 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -2646,7 +2646,8 @@  static int cr_pivot_root(char *root)
 {
 	char tmp_dir_tmpl[] = "crtools-put-root.XXXXXX";
 	bool tmp_dir = false;
-	char *put_root = "tmp";
+	char *put_root_ro = "tmp";
+	char *put_root;
 	int exit_code = -1;
 	struct stat st;
 
@@ -2659,14 +2660,33 @@  static int cr_pivot_root(char *root)
 		}
 	}
 
-	if (stat(put_root, &st) || !S_ISDIR(st.st_mode)) {
-		put_root = mkdtemp(tmp_dir_tmpl);
-		if (put_root == NULL) {
+	/*
+	 * When moving root into a new place we should use
+	 * temporary directory first: in particular we
+	 * already hit a problem where /tmp directory
+	 * was a bind mount itself inside own unshared
+	 * mount namespace. In result once everything is
+	 * finished we didn't see proper content.
+	 *
+	 * Thus try to use temporary generated directory
+	 * first, if it fails (say / is mounted as read
+	 * only) use /tmp. Well, strictly speaking we
+	 * should walk over the root fs and find first
+	 * toplevel dentry which is not mounted and use
+	 * it instead. But for simplicity lets stick this
+	 * way, the code is already too complex.
+	 */
+
+	put_root = mkdtemp(tmp_dir_tmpl);
+	if (!put_root) {
+		if (stat(put_root_ro, &st) || !S_ISDIR(st.st_mode)) {
 			pr_perror("Can't create a temporary directory");
 			return -1;
 		}
+		put_root = put_root_ro;
+	} else
 		tmp_dir = true;
-	}
+	pr_debug("\tPut root in %s\n", put_root);
 
 	if (mount(put_root, put_root, NULL, MS_BIND, NULL)) {
 		pr_perror("Unable to mount tmpfs in %s", put_root);

Comments

Andrei Vagin Dec. 11, 2018, 6:33 a.m.
Where is a test?

On Fri, Dec 07, 2018 at 03:43:31PM +0300, Cyrill Gorcunov wrote:
> The directory we're pinning old root might be a bind mount
> point itself. So we should try with temp location first
> and create own fresh new directory and only if it fails
> start to use /tmp.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/mount.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index b8a564d2ce92..451d9c965cea 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -2646,7 +2646,8 @@ static int cr_pivot_root(char *root)
>  {
>  	char tmp_dir_tmpl[] = "crtools-put-root.XXXXXX";
>  	bool tmp_dir = false;
> -	char *put_root = "tmp";
> +	char *put_root_ro = "tmp";
> +	char *put_root;
>  	int exit_code = -1;
>  	struct stat st;
>  
> @@ -2659,14 +2660,33 @@ static int cr_pivot_root(char *root)
>  		}
>  	}
>  
> -	if (stat(put_root, &st) || !S_ISDIR(st.st_mode)) {
> -		put_root = mkdtemp(tmp_dir_tmpl);
> -		if (put_root == NULL) {
> +	/*
> +	 * When moving root into a new place we should use
> +	 * temporary directory first: in particular we
> +	 * already hit a problem where /tmp directory
> +	 * was a bind mount itself inside own unshared
> +	 * mount namespace. In result once everything is
> +	 * finished we didn't see proper content.
> +	 *
> +	 * Thus try to use temporary generated directory
> +	 * first, if it fails (say / is mounted as read
> +	 * only) use /tmp. Well, strictly speaking we
> +	 * should walk over the root fs and find first
> +	 * toplevel dentry which is not mounted and use
> +	 * it instead. But for simplicity lets stick this
> +	 * way, the code is already too complex.
> +	 */
> +
> +	put_root = mkdtemp(tmp_dir_tmpl);
> +	if (!put_root) {
> +		if (stat(put_root_ro, &st) || !S_ISDIR(st.st_mode)) {
>  			pr_perror("Can't create a temporary directory");
>  			return -1;
>  		}
> +		put_root = put_root_ro;
> +	} else
>  		tmp_dir = true;
> -	}
> +	pr_debug("\tPut root in %s\n", put_root);
>  
>  	if (mount(put_root, put_root, NULL, MS_BIND, NULL)) {
>  		pr_perror("Unable to mount tmpfs in %s", put_root);
> -- 
> 2.17.2
>
Cyrill Gorcunov Dec. 11, 2018, 11:22 a.m.
On Mon, Dec 10, 2018 at 10:33:50PM -0800, Andrey Vagin wrote:
> Where is a test?
> 
> On Fri, Dec 07, 2018 at 03:43:31PM +0300, Cyrill Gorcunov wrote:
> > The directory we're pinning old root might be a bind mount
> > point itself. So we should try with temp location first
> > and create own fresh new directory and only if it fails
> > start to use /tmp.
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Andrew, defer this series. Seems this is not easy to recreate
this issue locally. Once I manage with test I'll resend.