[CRIU] [PATCH 1/2] [v4] mount: dump a file system only if a mount point isn't overmounted

[Andrew Vagin]

On Thu, May 12, 2016 at 05:44:09PM +0300, Pavel Emelyanov wrote:
> On 05/12/2016 08:52 AM, Andrey Vagin wrote:
> > From: Andrew Vagin <avagin at 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
> > v4: check that children are not overmounted a target mount instead
> > of getting a mnt_id for a file descriptor.
> > 
> > Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
> > ---
> >  criu/mount.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 48 insertions(+), 13 deletions(-)
> > 
> > diff --git a/criu/mount.c b/criu/mount.c
> > index e08f46e..83692a8 100644
> > --- a/criu/mount.c
> > +++ b/criu/mount.c
> > @@ -1155,6 +1155,7 @@ static char *get_clean_mnt(struct mount_info *mi, char *mnt_path_tmp, char *mnt_
> >  
> >  static int open_mountpoint(struct mount_info *pm)
> >  {
> > +	struct mount_info *c;
> >  	int fd = -1, ns_old = -1;
> >  	char mnt_path_tmp[] = "/tmp/cr-tmpfs.XXXXXX";
> >  	char mnt_path_root[] = "/cr-tmpfs.XXXXXX";
> > @@ -1170,6 +1171,13 @@ static int open_mountpoint(struct mount_info *pm)
> >  
> >  	pr_info("Something is mounted on top of %s\n", pm->mountpoint);
> >  
> > +	list_for_each_entry(c, &pm->children, siblings) {
> > +		if (!strcmp(c->mountpoint, pm->mountpoint)) {
> 
> A-ha! So we only don't open this thing if there's some children
> mounted at the same path. Don't we have c->fd for such beasts?
> From your previous set.

We c->fd on restore, but this patch is about dump

> 
> > +			pr_err("%d:%s is overmounted\n", pm->mnt_id, pm->mountpoint);
> > +			return -1;
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * To create a "private" copy, the target mount is bind-mounted
> >  	 * in a temporary place w/o MS_REC (non-recursively).
> > @@ -1252,7 +1260,7 @@ static int tmpfs_dump(struct mount_info *pm)
> >  
> >  	fd = open_mountpoint(pm);
> >  	if (fd < 0)
> > -		return -1;
> > +		return 1;
> 
> No, no, don't propagate every error from open_mountpoint() into "I'm overmounted"
> return code.

Why? criu enumirates all mount points to find one which is not
overmounted (can be opened).

We can't check that a mount point isn't overmounted before calling
fstype->dump(), because for example autofs is in many cases is
overmounted, but autofs->dump() can dump this mount point.

> 
> >  
> >  	/* 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
> > @@ -1463,7 +1471,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) {
> > @@ -1634,7 +1642,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) {
> > @@ -1691,7 +1699,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);
> > @@ -1893,6 +1901,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;
> > @@ -1905,16 +1947,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;
> > 
>