[CRIU] [PATCH v2] mount: don't overmount a mount if it should be bind-mounted somewhere

[Andrew Vagin]

On Tue, May 03, 2016 at 02:11:24PM -0700, Andrew Vagin wrote:
> On Tue, May 03, 2016 at 08:55:50AM +0300, Andrey Vagin wrote:
> > From: Andrew Vagin <avagin at virtuozzo.com>
> > 
> > It's impossiable to make a bind-mount if a source is overmounted.
> > 
> > v2: make a bind-mount from an underlying mount via a file descriptor
> 
> Unfortunately this way doesn't work for autofs. Stat, could you give any
> comment about this error?
> 
> (00.390353)      1: Error (mount.c:2483): mnt: Can't mount at .criu.mntns.s2mn7B/14/proc/sys/fs/binfmt_misc: Too many levels of symbolic links

I found an answer why we get ELOOP:

Documentation/filesystems/autofs4.txt

	The automount daemon is only able to mange a single mount location for
	an autofs filesystem and if mounts on that are not 'shared', other
	locations will not behave as expected.  In particular access to those
	other locations will likely result in the `ELOOP` error

So this patch is correct.

Today I found a real reason why we have a problem to dump and restore a
clean container with a binfmt_misc mount and httpd.

httpd lives in a separate mount namespace. An autofs mount is a shared
mount and binfmt_misc is a child of it, so we need to mount autofs in
both namespaces and only then mount binfmt_misc.

This logic should be fixed by these patches:

mount: handle a case when parent and child mounts in the same directory
mount: remove an extra condition from mounts_equal()

> 
> Thanks,
> Andrew
> 
> > 
> > Reported-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
> > Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
> > ---
> >  criu/include/mount.h |  1 +
> >  criu/mount.c         | 27 +++++++++++++++++++++++----
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/criu/include/mount.h b/criu/include/mount.h
> > index 59e7f9a..c7992ac 100644
> > --- a/criu/include/mount.h
> > +++ b/criu/include/mount.h
> > @@ -49,6 +49,7 @@ struct mount_info {
> >  	 */
> >  	char			*mountpoint;
> >  	char			*ns_mountpoint;
> > +	int			fd;
> >  	unsigned		flags;
> >  	unsigned		sb_flags;
> >  	int			master_id;
> > diff --git a/criu/mount.c b/criu/mount.c
> > index 0b1c2ba..6133a72 100644
> > --- a/criu/mount.c
> > +++ b/criu/mount.c
> > @@ -2447,11 +2447,15 @@ static int do_bind_mount(struct mount_info *mi)
> >  	 */
> >  	mi->private = mi->bind->private;
> >  
> > -	if (list_empty(&mi->bind->children))
> > -		mnt_path = mi->bind->mountpoint;
> > -	else {
> > +	mnt_path = mi->bind->mountpoint;
> > +	if (mi->bind->fd >= 0) {
> > +		snprintf(rpath, sizeof(rpath), "/proc/self/fd/%d", mi->bind->fd);
> > +		mnt_path = rpath;
> > +	}
> > +
> > +	if (!list_empty(&mi->bind->children)) {
> >  		/* mi->bind->mountpoint may be overmounted */
> > -		if (mount(mi->bind->mountpoint, mnt_clean_path, NULL, MS_BIND, NULL)) {
> > +		if (mount(mnt_path, mnt_clean_path, NULL, MS_BIND, NULL)) {
> >  			pr_perror("Unable to bind-mount %s to %s",
> >  					mi->bind->mountpoint, mnt_clean_path);
> >  		}
> > @@ -2606,6 +2610,11 @@ static int do_mount_root(struct mount_info *mi)
> >  	return fetch_rt_stat(mi, mi->mountpoint);
> >  }
> >  
> > +static int do_close_one(struct mount_info *mi) {
> > +	close_safe(&mi->fd);
> > +	return 0;
> > +}
> > +
> >  static int do_mount_one(struct mount_info *mi)
> >  {
> >  	int ret;
> > @@ -2618,6 +2627,14 @@ static int do_mount_one(struct mount_info *mi)
> >  		return 1;
> >  	}
> >  
> > +	if (mi->parent && !strcmp(mi->parent->mountpoint, mi->mountpoint)) {
> > +		mi->parent->fd = open(mi->parent->mountpoint, O_PATH);
> > +		if (mi->parent->fd < 0) {
> > +			pr_perror("Unable to open %s", mi->mountpoint);
> > +			return -1;
> > +		}
> > +	}
> > +
> >  	pr_debug("\tMounting %s @%s (%d)\n", mi->fstype->name, mi->mountpoint, mi->need_plugin);
> >  
> >  	if (!mi->parent) {
> > @@ -2740,6 +2757,7 @@ struct mount_info *mnt_entry_alloc()
> >  
> >  	new = xzalloc(sizeof(struct mount_info));
> >  	if (new) {
> > +		new->fd = -1;
> >  		INIT_LIST_HEAD(&new->children);
> >  		INIT_LIST_HEAD(&new->siblings);
> >  		INIT_LIST_HEAD(&new->mnt_slave_list);
> > @@ -3211,6 +3229,7 @@ static int populate_mnt_ns(void)
> >  		return -1;
> >  
> >  	ret = mnt_tree_for_each(pms, do_mount_one);
> > +	mnt_tree_for_each(pms, do_close_one);
> >  
> >  	if (umount_clean_path())
> >  		return -1;
> > -- 
> > 2.5.5
> >