[CRIU] [PATCH 2/3] mount: allow to dump content even if a part of fs is overmounted (v2)

[Pavel Emelyanov]

On 08/01/2013 04:44 PM, Andrey Vagin wrote:
> for that the mount point is bind-mounted in a temporary place.
> 
> v2: * check, that the fs you get access to at the end is _really_ the
>       one you wanted to
>     * use switch_ns/restore_ns helpers
> 
> Signed-off-by: Andrey Vagin <avagin at openvz.org>
> ---
>  mount.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/mount.c b/mount.c
> index 68965bb..1ba0ddc 100644
> --- a/mount.c
> +++ b/mount.c
> @@ -197,18 +197,13 @@ static struct mount_info *mnt_build_tree(struct mount_info *list)
>  	return tree;
>  }
>  
> -static DIR *open_mountpoint(struct mount_info *pm)
> +static DIR *__open_mountpoint(struct mount_info *pm)
>  {
>  	int fd, ret;
>  	char path[PATH_MAX + 1];
>  	struct stat st;
>  	DIR *fdir;
>  
> -	if (!list_empty(&pm->children)) {
> -		pr_err("Something is mounted on top of %s\n", pm->fstype->name);
> -		return NULL;
> -	}
> -
>  	snprintf(path, sizeof(path), ".%s", pm->mountpoint);
>  	fd = openat(mntns_root, path, O_RDONLY);
>  	if (fd < 0) {
> @@ -249,6 +244,94 @@ static int close_mountpoint(DIR *dfd)
>  	return 0;
>  }
>  
> +static DIR *open_mountpoint(struct mount_info *pm)
> +{
> +	int fd = -1, ns_old = -1;
> +	DIR *fdir = NULL;
> +	char buf[PATH_MAX];
> +	struct stat st;
> +	char mnt_path[] = "/tmp/cr-tmpfs.XXXXXX";
> +	struct ns_desc ns_desc = NS_DESC_ENTRY(CLONE_NEWNS, "mnt");

= &mnt_ns_desc ? And the on-stack variable is not required if yes ;)

> +
> +	/*
> +	 * If a mount doesn't have children, we can open a mount point,
> +	 * otherwise we need to create a "private" copy.
> +	 */
> +	if (list_empty(&pm->children))
> +		return __open_mountpoint(pm);
> +
> +	pr_info("Something is mounted on top of %s\n", pm->mountpoint);
> +
> +	/*
> +	 * To create a "private" copy, the target mount is bind-mounted
> +	 * in a temporary place w/o MS_REC (non-recursively).
> +	 * A mount point can't be bind-mounted in criu's namespace, it will be
> +	 * mounted in a target namespace. The sequence of actions is
> +	 * mkdtemp, setns(tgt), mount, open, detach, setns(old).
> +	 */
> +
> +	if (switch_ns(root_item->pid.real, &ns_desc, &ns_old) < 0)
> +		return NULL;
> +
> +	if (mkdtemp(mnt_path) == NULL) {
> +		pr_perror("Can't create a temporary directory");
> +		goto out;
> +	}
> +
> +	snprintf(buf, sizeof(buf), "/proc/self/root/%s", pm->mountpoint);
> +	if (mount(buf, mnt_path, NULL, MS_BIND, NULL)) {
> +		pr_perror("Can't bind-mount %d:%s to %s",
> +				pm->mnt_id, pm->mountpoint, mnt_path);
> +		rmdir(mnt_path);
> +		goto out;
> +	}
> +
> +	fd = open(mnt_path, O_RDONLY | O_DIRECTORY);
> +	if (fd < 0)
> +		pr_perror("Can't open %s\n", mnt_path);

Can we move this pr_perror into fd < 0 checking below?

> +
> +	if (umount2(mnt_path, MNT_DETACH)) {
> +		pr_perror("Can't umount %s", mnt_path);
> +		goto out;
> +	}
> +
> +	if (rmdir(mnt_path)) {
> +		pr_perror("Can't remove the directory %s", mnt_path);
> +		goto out;
> +	}
> +
> +	if (fd < 0)
> +		goto out;
> +
> +	if (fstat(fd, &st)) {
> +		pr_perror("Can't get file status");
> +		goto out;
> +	}
> +
> +	if (pm->s_dev != st.st_dev) {
> +		pr_err("Can't create a temporary mount "
> +			"(device mismatch %lx != %x\n", st.st_dev, pm->s_dev);
> +		goto out;
> +	}

We have quite a lot of the above in the __open_mountpoint. Can we
somehow merge these two codes?

> +	if (restore_ns(ns_old, &ns_desc))
> +		goto out;
> +
> +	fdir = fdopendir(fd);
> +	if (fdir == NULL) {
> +		close(fd);
> +		pr_perror("Can't open %s", pm->mountpoint);
> +		return NULL;

Can we make the error paths all-in-one-chain? E.g. here should
be goto out_close, with out_close: in proper place below.

> +	}
> +
> +	return fdir;
> +out:
> +	if (ns_old >= 0 && restore_ns(ns_old, &ns_desc))
> +		pr_perror("Can't return into the origin mount namespace");

restore_ns prints error itself.

> +	close_safe(&fd);
> +	return NULL;
> +}
> +
>  static int tmpfs_dump(struct mount_info *pm)
>  {
>  	int ret = -1;
>