[v3] mount: dump a file system only if a mount point isn't overmounted

Submitted by Andrei Vagin on May 11, 2016, 12:37 a.m.

Details

Message ID 1462927050-30912-1-git-send-email-avagin@openvz.org
State Rejected
Series "mount: dump a file system only if a mount point isn't overmounted"
Headers show

Commit Message

Andrei Vagin May 11, 2016, 12:37 a.m.
From: Andrew Vagin <avagin@virtuozzo.com>

Something else may be mounted into the same folder and in this case
we can't get access to the required file system.

$ cat /proc/61693/root/etc/redhat-release
Fedora release 23 (Twenty Three)

$ cat /proc/61692/mountinfo  | grep '\s/tmp'
234 199 0:57 / /tmp rw shared:97 master:76 - tmpfs tmpfs rw,size=131072k,nr_inodes=32768
235 234 0:57 /systemd-private-dd74de99e1104383aa7cd6e27d3d0b8a-httpd.service-uFqNHk/tmp /tmp rw,relatime shared:98 master:76 - tmpfs tmpfs rw,size=131072k,nr_inodes=32768

v2: return an error if we can't dump a file system
v3: try to find a mount point which allows to dump a file system
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
---
 criu/mount.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index fc584bf..71c417a 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -1130,7 +1130,7 @@  static int open_mountpoint(struct mount_info *pm)
 	char mnt_path_tmp[] = "/tmp/cr-tmpfs.XXXXXX";
 	char mnt_path_root[] = "/cr-tmpfs.XXXXXX";
 	char *mnt_path = mnt_path_tmp;
-	int cwd_fd;
+	int cwd_fd, mnt_id;
 
 	/*
 	 * If a mount doesn't have children, we can open a mount point,
@@ -1158,6 +1158,19 @@  static int open_mountpoint(struct mount_info *pm)
 	if (switch_ns(pm->nsid->ns_pid, &mnt_ns_desc, &ns_old) < 0)
 		goto out;
 
+	/* check that a correct mountpoint can be opened */
+	fd = open(pm->mountpoint, O_RDONLY);
+	if (fd < 0) {
+		pr_perror("Unable to open %s", pm->mountpoint);
+		goto out;
+	}
+
+	if (get_fd_mntid(fd, &mnt_id) || mnt_id != pm->mnt_id) {
+		pr_err("Unable to open %d(%d):%s\n", pm->mnt_id, mnt_id, pm->mountpoint);
+		goto out;
+	}
+	close_safe(&fd);
+
 	mnt_path = get_clean_mnt(pm, mnt_path_tmp, mnt_path_root);
 	if (mnt_path == NULL)
 		goto out;
@@ -1223,7 +1236,7 @@  static int tmpfs_dump(struct mount_info *pm)
 
 	fd = open_mountpoint(pm);
 	if (fd < 0)
-		return -1;
+		return 1;
 
 	/* if fd happens to be 0 here, we need to move it to something
 	 * non-zero, because cr_system_userns closes STDIN_FILENO as we are not
@@ -1434,7 +1447,7 @@  static int binfmt_misc_dump(struct mount_info *pm)
 
 	fd = open_mountpoint(pm);
 	if (fd < 0)
-		return -1;
+		return 1;
 
 	fdir = fdopendir(fd);
 	if (fdir == NULL) {
@@ -1605,7 +1618,7 @@  static int fusectl_dump(struct mount_info *pm)
 
 	fd = open_mountpoint(pm);
 	if (fd < 0)
-		return -1;
+		return 1;
 
 	fdir = fdopendir(fd);
 	if (fdir == NULL) {
@@ -1662,7 +1675,7 @@  static int dump_empty_fs(struct mount_info *pm)
 	fd = open_mountpoint(pm);
 
 	if (fd < 0)
-		return -1;
+		return 1;
 
 	ret = is_empty_dir(fd);
 	close(fd);
@@ -1864,6 +1877,40 @@  uns:
 	return &fstypes[0];
 }
 
+static int dump_one_fs(struct mount_info *mi)
+{
+	struct mount_info *pm = mi;
+	struct mount_info *t;
+	bool first = true;
+
+	if (mi->is_ns_root || mi->need_plugin || mi->external || !mi->fstype->dump)
+		return 0;
+
+	for (; &pm->mnt_bind != &mi->mnt_bind || first;
+	     pm = list_entry(pm->mnt_bind.next, typeof(*pm), mnt_bind)) {
+		int ret;
+
+		first = false;
+
+		if (!fsroot_mounted(pm))
+			continue;
+
+		ret = pm->fstype->dump(pm);
+		if (ret == 1)
+			continue;
+		if (ret < 0)
+			return ret;
+
+		list_for_each_entry(t, &pm->mnt_bind, mnt_bind)
+			t->dumped = true;
+		return 0;
+	}
+
+	pr_err("Unable to dump a file system for %d:%s\n",
+				mi->mnt_id, mi->mountpoint);
+	return -1;
+}
+
 static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
 {
 	MntEntry me = MNT_ENTRY__INIT;
@@ -1876,16 +1923,9 @@  static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
 	if (me.fstype == FSTYPE__AUTO)
 		me.fsname = pm->fsname;
 
-	if (pm->parent && !pm->dumped && !pm->need_plugin && !pm->external &&
-	    pm->fstype->dump && fsroot_mounted(pm)) {
-		struct mount_info *t;
-
-		if (pm->fstype->dump(pm))
-			return -1;
 
-		list_for_each_entry(t, &pm->mnt_bind, mnt_bind)
-			t->dumped = true;
-	}
+	if (!pm->dumped && dump_one_fs(pm))
+		return -1;
 
 	me.mnt_id		= pm->mnt_id;
 	me.root_dev		= pm->s_dev;

Comments

Pavel Emelianov May 11, 2016, 12:54 p.m.
On 05/11/2016 03:37 AM, Andrey Vagin wrote:
> From: Andrew Vagin <avagin@virtuozzo.com>
> 
> Something else may be mounted into the same folder and in this case
> we can't get access to the required file system.
> 
> $ cat /proc/61693/root/etc/redhat-release
> Fedora release 23 (Twenty Three)
> 
> $ cat /proc/61692/mountinfo  | grep '\s/tmp'
> 234 199 0:57 / /tmp rw shared:97 master:76 - tmpfs tmpfs rw,size=131072k,nr_inodes=32768
> 235 234 0:57 /systemd-private-dd74de99e1104383aa7cd6e27d3d0b8a-httpd.service-uFqNHk/tmp /tmp rw,relatime shared:98 master:76 - tmpfs tmpfs rw,size=131072k,nr_inodes=32768
> 
> v2: return an error if we can't dump a file system
> v3: try to find a mount point which allows to dump a file system
> Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
> ---
>  criu/mount.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 54 insertions(+), 14 deletions(-)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index fc584bf..71c417a 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -1130,7 +1130,7 @@ static int open_mountpoint(struct mount_info *pm)
>  	char mnt_path_tmp[] = "/tmp/cr-tmpfs.XXXXXX";
>  	char mnt_path_root[] = "/cr-tmpfs.XXXXXX";
>  	char *mnt_path = mnt_path_tmp;
> -	int cwd_fd;
> +	int cwd_fd, mnt_id;
>  
>  	/*
>  	 * If a mount doesn't have children, we can open a mount point,
> @@ -1158,6 +1158,19 @@ static int open_mountpoint(struct mount_info *pm)
>  	if (switch_ns(pm->nsid->ns_pid, &mnt_ns_desc, &ns_old) < 0)
>  		goto out;
>  
> +	/* check that a correct mountpoint can be opened */
> +	fd = open(pm->mountpoint, O_RDONLY);
> +	if (fd < 0) {
> +		pr_perror("Unable to open %s", pm->mountpoint);
> +		goto out;
> +	}
> +
> +	if (get_fd_mntid(fd, &mnt_id) || mnt_id != pm->mnt_id) {
> +		pr_err("Unable to open %d(%d):%s\n", pm->mnt_id, mnt_id, pm->mountpoint);
> +		goto out;
> +	}
> +	close_safe(&fd);

Please, explain this change. The amount of checks around opening a mountpoint
becomes ... too big :)

-- Pavel

> +
>  	mnt_path = get_clean_mnt(pm, mnt_path_tmp, mnt_path_root);
>  	if (mnt_path == NULL)
>  		goto out;
> @@ -1223,7 +1236,7 @@ static int tmpfs_dump(struct mount_info *pm)
>  
>  	fd = open_mountpoint(pm);
>  	if (fd < 0)
> -		return -1;
> +		return 1;
>  
>  	/* if fd happens to be 0 here, we need to move it to something
>  	 * non-zero, because cr_system_userns closes STDIN_FILENO as we are not
> @@ -1434,7 +1447,7 @@ static int binfmt_misc_dump(struct mount_info *pm)
>  
>  	fd = open_mountpoint(pm);
>  	if (fd < 0)
> -		return -1;
> +		return 1;
>  
>  	fdir = fdopendir(fd);
>  	if (fdir == NULL) {
> @@ -1605,7 +1618,7 @@ static int fusectl_dump(struct mount_info *pm)
>  
>  	fd = open_mountpoint(pm);
>  	if (fd < 0)
> -		return -1;
> +		return 1;
>  
>  	fdir = fdopendir(fd);
>  	if (fdir == NULL) {
> @@ -1662,7 +1675,7 @@ static int dump_empty_fs(struct mount_info *pm)
>  	fd = open_mountpoint(pm);
>  
>  	if (fd < 0)
> -		return -1;
> +		return 1;
>  
>  	ret = is_empty_dir(fd);
>  	close(fd);
> @@ -1864,6 +1877,40 @@ uns:
>  	return &fstypes[0];
>  }
>  
> +static int dump_one_fs(struct mount_info *mi)
> +{
> +	struct mount_info *pm = mi;
> +	struct mount_info *t;
> +	bool first = true;
> +
> +	if (mi->is_ns_root || mi->need_plugin || mi->external || !mi->fstype->dump)
> +		return 0;
> +
> +	for (; &pm->mnt_bind != &mi->mnt_bind || first;
> +	     pm = list_entry(pm->mnt_bind.next, typeof(*pm), mnt_bind)) {
> +		int ret;
> +
> +		first = false;
> +
> +		if (!fsroot_mounted(pm))
> +			continue;
> +
> +		ret = pm->fstype->dump(pm);
> +		if (ret == 1)
> +			continue;
> +		if (ret < 0)
> +			return ret;
> +
> +		list_for_each_entry(t, &pm->mnt_bind, mnt_bind)
> +			t->dumped = true;
> +		return 0;
> +	}
> +
> +	pr_err("Unable to dump a file system for %d:%s\n",
> +				mi->mnt_id, mi->mountpoint);
> +	return -1;
> +}
> +
>  static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>  {
>  	MntEntry me = MNT_ENTRY__INIT;
> @@ -1876,16 +1923,9 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>  	if (me.fstype == FSTYPE__AUTO)
>  		me.fsname = pm->fsname;
>  
> -	if (pm->parent && !pm->dumped && !pm->need_plugin && !pm->external &&
> -	    pm->fstype->dump && fsroot_mounted(pm)) {
> -		struct mount_info *t;
> -
> -		if (pm->fstype->dump(pm))
> -			return -1;
>  
> -		list_for_each_entry(t, &pm->mnt_bind, mnt_bind)
> -			t->dumped = true;
> -	}
> +	if (!pm->dumped && dump_one_fs(pm))
> +		return -1;
>  
>  	me.mnt_id		= pm->mnt_id;
>  	me.root_dev		= pm->s_dev;
>