[CRIU] [PATCH] mount: Umount nested temporary mount pathes in do_bind_mount()
Andrew Vagin
avagin at virtuozzo.com
Tue Mar 1 17:56:06 PST 2016
Hi Kirill,
Could you check attached patches.
Thanks,
Andrew
On Tue, Mar 01, 2016 at 03:52:19PM -0800, Andrew Vagin wrote:
> On Fri, Feb 26, 2016 at 01:43:13PM +0300, Kirill Tkhai wrote:
> > Hi, Andrew,
> >
> > On 26.02.2016 03:47, Andrew Vagin wrote:
> > > On Thu, Feb 25, 2016 at 04:54:27PM +0300, Kirill Tkhai wrote:
> > >> When a parent directory of restored mount is bound to itself,
> > >> a temporary mount is cloned, so we need to umount it twice:
> > >>
> > >> # mkdir -p parent/child /tmp/cr-XXX
> > >> # mount --bind parent parent
> > >> # mount --bind parent/child /tmp/cr-XXX/
> > >> # mount --bind /tmp/cr-XXX/ parent/child
> > >> # umount /tmp/cr-XXX/
> > >> # mount | grep "/tmp/cr-XXX"
> > >> /dev/sda3 on /tmp/cr-XXX type ext4 (rw,noatime,nodiratime,data=ordered)
> > >>
> > >> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
> > >> ---
> > >> criu/mount.c | 27 ++++++++++++++++++++++++++-
> > >> 1 file changed, 26 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/criu/mount.c b/criu/mount.c
> > >> index 1a6b034..bce2465 100644
> > >> --- a/criu/mount.c
> > >> +++ b/criu/mount.c
> > >> @@ -301,6 +301,26 @@ static bool mounts_equal(struct mount_info *a, struct mount_info *b)
> > >> }
> > >>
> > >> /*
> > >> + * Successively umount all file systems mounted on @path
> > >> + */
> > >> +static int umount2_nested(const char *path, int flags)
> > >> +{
> > >> + bool once = true;
> > >> + int ret;
> > >> +
> > >> + while (1) {
> > >> + ret = umount2(path, flags);
> > >> + if (ret)
> > >> + break;
> > >> + once = false;
> > >> + }
> > >> +
> > >> + if (once || errno != EINVAL)
> > >> + return -1;
> > >> + return 0;
> > >> +}
> > >> +
> > >> +/*
> > >> * mnt_roots is a temporary directory for restoring sub-trees of
> > >> * non-root namespaces.
> > >> */
> > >> @@ -2397,7 +2417,12 @@ static int do_bind_mount(struct mount_info *mi)
> > > if (mount(NULL, mnt_path, NULL, MS_PRIVATE, NULL)) {
> > >
> > > I think we don't need to make it private ^^^^, because it may be
> > > propagated to somewhere. Kirll, could you try to remove your changes and
> > > this mount and check that everythink works as expected in this case.
> >
> > No, it does not help. MS_PRIVATE would affect on child mounts, while the problem
> > is with parent. Try this example
> >
> > # mkdir -p parent/child /tmp/cr-XXX
> > # mount --bind parent parent
> > # mount --bind parent/child /tmp/cr-XXX/
> > # mount --bind /tmp/cr-XXX/ parent/child
> > # mount
> > /dev/sda2 on /tmp/cr-XXX type ext4 (rw,relatime,data=ordered)
> > /dev/sda2 on /root/parent/child type ext4 (rw,relatime,data=ordered)
> > /dev/sda2 on /tmp/cr-XXX type ext4 (rw,relatime,data=ordered)
> > /dev/sda2 on /root/parent/child type ext4 (rw,relatime,data=ordered)
> >
> > Nothing changes if you modify /tmp/cr-XXX or /root/parent/child
> > subtree type now.
> >
> > The only different solution in our example is to make ./parent MS_PRIVATE
> > right after it's bound to itself. Common solution will require to traverse
> > to top of the tree and check every parent subtree type, and do it private,
> > if it's need, and after all to return everything back. But it's too complex.
>
> You can't recreate shared dependences for mounts.
>
> The patch is incorrect, because "umount /tmp/cr-XXX" will umount
> /tmp/cr-XXX and parent/child
>
> [root at fc22-vm tmp]# bash -x test.sh
> + mkdir -p parent/child /tmp/cr-XXX
> + mount --bind parent parent
> + mount --bind parent/child /tmp/cr-XXX/
> + mount --bind /tmp/cr-XXX/ parent/child
> + mount --make-private /tmp/cr-XXX/
> + umount /tmp/cr-XXX/
> + umount /tmp/cr-XXX/
> + mount
> + grep parent
> /dev/vda2 on /root/tmp/parent type ext4 (rw,relatime,data=ordered)
>
> [root at fc22-vm tmp]# cat test.sh
> mkdir -p parent/child /tmp/cr-XXX
> mount --bind parent parent
> mount --bind parent/child /tmp/cr-XXX/
> mount --bind /tmp/cr-XXX/ parent/child
> mount --make-private /tmp/cr-XXX/
> umount /tmp/cr-XXX/
> umount /tmp/cr-XXX/
> mount | grep parent
>
> I really recomend to write tests for such sort of problems.
>
> I don't know how to fix this problem. Will think...
>
> Let me know if you have any ideas.
> >
> > Regards,
> > Kirill
> > >> pr_perror("Unable to make %s private", mnt_path);
> > >> return -1;
> > >> }
> > >> - if (umount2(mnt_path, MNT_DETACH)) {
> > >> + /*
> > >> + * If mi and mi->bind have the same mountpoint, and
> > >> + * a parent dir of the mountpoint is bound to itself,
> > >> + * then we have several mounts on mnt_path.
> > >> + */
> > >> + if (umount2_nested(mnt_path, MNT_DETACH)) {
> > >> pr_perror("Unable to umount %s", mnt_path);
> > >> return -1;
> > >> }
> > >>
> > >> _______________________________________________
> > >> CRIU mailing list
> > >> CRIU at openvz.org
> > >> https://lists.openvz.org/mailman/listinfo/criu
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
-------------- next part --------------
>From 7e5b05277e42d409615bdf639c4a002334607dd4 Mon Sep 17 00:00:00 2001
From: Andrew Vagin <avagin at virtuozzo.com>
Date: Wed, 2 Mar 2016 01:30:34 +0300
Subject: [PATCH 1/2] zdtm: Check a case when a few mounts are mounted by one
path
Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
---
test/zdtm/static/Makefile | 1 +
test/zdtm/static/mntns_shared_bind03.c | 79 +++++++++++++++++++++++++++++++
test/zdtm/static/mntns_shared_bind03.desc | 1 +
3 files changed, 81 insertions(+)
create mode 100644 test/zdtm/static/mntns_shared_bind03.c
create mode 100644 test/zdtm/static/mntns_shared_bind03.desc
diff --git a/test/zdtm/static/Makefile b/test/zdtm/static/Makefile
index f5b9e67..42ebd39 100644
--- a/test/zdtm/static/Makefile
+++ b/test/zdtm/static/Makefile
@@ -223,6 +223,7 @@ TST_DIR = \
mntns_link_ghost \
mntns_shared_bind \
mntns_shared_bind02 \
+ mntns_shared_bind03 \
mntns_root_bind \
mntns_root_bind02 \
mnt_ext_auto \
diff --git a/test/zdtm/static/mntns_shared_bind03.c b/test/zdtm/static/mntns_shared_bind03.c
new file mode 100644
index 0000000..54c5a64
--- /dev/null
+++ b/test/zdtm/static/mntns_shared_bind03.c
@@ -0,0 +1,79 @@
+#define _GNU_SOURCE
+#include <stdbool.h>
+#include <string.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <signal.h>
+#include <stdio.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <sched.h>
+#include <sys/wait.h>
+#include <stdlib.h>
+#include <limits.h>
+
+#include "zdtmtst.h"
+
+#ifndef CLONE_NEWNS
+#define CLONE_NEWNS 0x00020000
+#endif
+
+const char *test_doc = "Check a case when a few mounts are mounted by one path";
+const char *test_author = "Andrew Vagin <avagin at gmail.com>";
+
+char *dirname;
+TEST_OPTION(dirname, string, "directory name", 1);
+
+
+int main(int argc, char **argv)
+{
+ char path[PATH_MAX], spath[PATH_MAX];
+
+ test_init(argc, argv);
+
+ snprintf(path, sizeof(path), "%s/test", dirname);
+ snprintf(spath, sizeof(spath), "%s/test/sub", dirname);
+ if (mkdir(dirname, 0700)) {
+ pr_perror("mkdir");
+ return 1;
+ }
+
+ if (mount(NULL, "/", NULL, MS_SHARED, NULL)) {
+ pr_perror("mount");
+ return 1;
+ }
+
+#ifdef SHARED_BIND02
+ /* */
+ if (mount(dirname, dirname, "tmpfs", 0, NULL) ||
+ mount(NULL, dirname, NULL, MS_SHARED, NULL)) {
+ pr_perror("mount");
+ return 1;
+ }
+#endif
+
+ if (mkdir(path, 0700) ||
+ mkdir(spath, 0700)) {
+ pr_perror("mkdir");
+ return 1;
+ }
+ if (mount("test", path, "tmpfs", 0, NULL)) {
+ pr_perror("mount");
+ return 1;
+ }
+ if (mount(path, path, NULL, MS_BIND, NULL)) {
+ pr_perror("mount");
+ return 1;
+ }
+ if (mount(NULL, path, NULL, MS_PRIVATE, NULL)) {
+ pr_perror("mount");
+ return 1;
+ }
+
+ test_daemon();
+ test_waitsig();
+
+ pass();
+
+ return 0;
+}
diff --git a/test/zdtm/static/mntns_shared_bind03.desc b/test/zdtm/static/mntns_shared_bind03.desc
new file mode 100644
index 0000000..a8849e0
--- /dev/null
+++ b/test/zdtm/static/mntns_shared_bind03.desc
@@ -0,0 +1 @@
+{'flavor': 'ns uns', 'flags': 'suid', 'feature': 'mnt_id'}
--
2.5.0
-------------- next part --------------
>From 132e2a0ea88a43f669c3b53b8d63c965b6577d93 Mon Sep 17 00:00:00 2001
From: Andrew Vagin <avagin at virtuozzo.com>
Date: Wed, 2 Mar 2016 04:50:41 +0300
Subject: [PATCH 2/2] mount: fix a case when a few mounts are mounted by one
path
Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
---
criu/mount.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/criu/mount.c b/criu/mount.c
index 1a6b034..b32f847 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -2309,9 +2309,11 @@ static int do_bind_mount(struct mount_info *mi)
private = !mi->master_id && !shared;
cut_root = cut_root_for_bind(mi->root, mi->bind->root);
- if (list_empty(&mi->bind->children))
+ if (list_empty(&mi->bind->children) ||
+ !strcmp(mi->mountpoint, mi->bind->mountpoint))
mnt_path = mi->bind->mountpoint;
else {
+ /* mi->bind->mountpoint may be overmounted */
mnt_path = get_clean_mnt(mi->bind, mnt_path_tmp, mnt_path_root);
umount_mnt_path = true;
}
--
2.5.0
More information about the CRIU
mailing list