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

Submitted by Andrei Vagin on May 3, 2016, 5:55 a.m.

Details

Message ID 1462254950-21957-1-git-send-email-avagin@openvz.org
State Rejected
Series "mount: don't overmount a mount if it should be bind-mounted somewhere"
Headers show

Commit Message

Andrei Vagin May 3, 2016, 5:55 a.m.
From: Andrew Vagin <avagin@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

Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
---
 criu/include/mount.h |  1 +
 criu/mount.c         | 27 +++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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;

Comments

Andrey Vagin May 3, 2016, 9:11 p.m.
On Tue, May 03, 2016 at 08:55:50AM +0300, Andrey Vagin wrote:
> From: Andrew Vagin <avagin@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

Thanks,
Andrew

> 
> Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> Signed-off-by: Andrew Vagin <avagin@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
>
Andrey Vagin May 4, 2016, 6:55 p.m.
Cc: Stas

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@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. Stas, 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
> 
> Thanks,
> Andrew
> 
> > 
> > Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> > Signed-off-by: Andrew Vagin <avagin@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
> >
Andrey Vagin May 5, 2016, 7:24 a.m.
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@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@virtuozzo.com>
> > Signed-off-by: Andrew Vagin <avagin@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
> >
Pavel Emelianov May 6, 2016, 12:06 p.m.
On 05/03/2016 08:55 AM, Andrey Vagin wrote:
> From: Andrew Vagin <avagin@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

How about a test for this?

> Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> Signed-off-by: Andrew Vagin <avagin@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)) {

What if this if () is false, but mi->bind->fd >= 0 is true. We go ahead and corrupt
the rpath variable below.

>  		/* 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) {

Brace.

> +	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;
>
Andrey Vagin May 6, 2016, 5:09 p.m.
On Fri, May 06, 2016 at 03:06:45PM +0300, Pavel Emelyanov wrote:
> On 05/03/2016 08:55 AM, Andrey Vagin wrote:
> > From: Andrew Vagin <avagin@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
> 
> How about a test for this?
 
I have it in a queue and will send when you commit all bilding blocks
to pass this test.