From 487c6c7e73dbb7871e80d75f176dd2a3539a2947 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 24 Jan 2018 15:10:01 -0800 Subject: [PATCH] Ensure daemon root is unmounted on shutdown This is only for the case when dockerd has had to re-mount the daemon root as shared. Signed-off-by: Brian Goff --- daemon/daemon_linux.go | 38 ++++++++++++++++++++-- daemon/daemon_linux_test.go | 64 +++++++++++++++++++++++++++++++++++++ daemon/oci_linux.go | 51 +++++++++++++---------------- 3 files changed, 123 insertions(+), 30 deletions(-) diff --git a/daemon/daemon_linux.go b/daemon/daemon_linux.go index 911ef7f498..d27e7333b7 100644 --- a/daemon/daemon_linux.go +++ b/daemon/daemon_linux.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/pkg/fileutils" "github.com/docker/docker/pkg/mount" + "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -67,9 +68,29 @@ func (daemon *Daemon) cleanupMountsFromReaderByID(reader io.Reader, id string, u return nil } -// cleanupMounts umounts shm/mqueue mounts for old containers +// cleanupMounts umounts used by container resources and the daemon root mount func (daemon *Daemon) cleanupMounts() error { - return daemon.cleanupMountsByID("") + if err := daemon.cleanupMountsByID(""); err != nil { + return err + } + + infos, err := mount.GetMounts() + if err != nil { + return errors.Wrap(err, "error reading mount table for cleanup") + } + + info := getMountInfo(infos, daemon.root) + // `info.Root` here is the root mountpoint of the passed in path (`daemon.root`). + // The ony cases that need to be cleaned up is when the daemon has performed a + // `mount --bind /daemon/root /daemon/root && mount --make-shared /daemon/root` + // This is only done when the daemon is started up and `/daemon/root` is not + // already on a shared mountpoint. + if !shouldUnmountRoot(daemon.root, info) { + return nil + } + + logrus.WithField("mountpoint", daemon.root).Debug("unmounting daemon root") + return mount.Unmount(daemon.root) } func getCleanPatterns(id string) (regexps []*regexp.Regexp) { @@ -91,3 +112,16 @@ func getCleanPatterns(id string) (regexps []*regexp.Regexp) { func getRealPath(path string) (string, error) { return fileutils.ReadSymlinkedDirectory(path) } + +func shouldUnmountRoot(root string, info *mount.Info) bool { + if info == nil { + return false + } + if info.Mountpoint != root { + return false + } + if !strings.HasSuffix(root, info.Root) { + return false + } + return hasMountinfoOption(info.Optional, sharedPropagationOption) +} diff --git a/daemon/daemon_linux_test.go b/daemon/daemon_linux_test.go index ac9404cba9..ad651e3e1c 100644 --- a/daemon/daemon_linux_test.go +++ b/daemon/daemon_linux_test.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/container" "github.com/docker/docker/oci" "github.com/docker/docker/pkg/idtools" + "github.com/docker/docker/pkg/mount" "github.com/stretchr/testify/assert" ) @@ -164,3 +165,66 @@ func TestValidateContainerIsolationLinux(t *testing.T) { _, err := d.verifyContainerSettings("linux", &containertypes.HostConfig{Isolation: containertypes.IsolationHyperV}, nil, false) assert.EqualError(t, err, "invalid isolation 'hyperv' on linux") } + +func TestShouldUnmountRoot(t *testing.T) { + for _, test := range []struct { + desc string + root string + info *mount.Info + expect bool + }{ + { + desc: "root is at /", + root: "/docker", + info: &mount.Info{Root: "/docker", Mountpoint: "/docker"}, + expect: true, + }, + { + desc: "not a mountpoint", + root: "/docker", + info: nil, + expect: false, + }, + { + desc: "root is at in a submount from `/`", + root: "/foo/docker", + info: &mount.Info{Root: "/docker", Mountpoint: "/foo/docker"}, + expect: true, + }, + { + desc: "root is mounted in from a parent mount namespace same root dir", // dind is an example of this + root: "/docker", + info: &mount.Info{Root: "/docker/volumes/1234657/_data", Mountpoint: "/docker"}, + expect: false, + }, + { + desc: "root is mounted in from a parent mount namespace different root dir", + root: "/foo/bar", + info: &mount.Info{Root: "/docker/volumes/1234657/_data", Mountpoint: "/foo/bar"}, + expect: false, + }, + } { + t.Run(test.desc, func(t *testing.T) { + for _, options := range []struct { + desc string + Optional string + expect bool + }{ + {desc: "shared", Optional: "shared:", expect: true}, + {desc: "slave", Optional: "slave:", expect: false}, + {desc: "private", Optional: "private:", expect: false}, + } { + t.Run(options.desc, func(t *testing.T) { + expect := options.expect + if expect { + expect = test.expect + } + if test.info != nil { + test.info.Optional = options.Optional + } + assert.Equal(t, expect, shouldUnmountRoot(test.root, test.info)) + }) + } + }) + } +} diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go index 2a19dd5db8..41c0a71f55 100644 --- a/daemon/oci_linux.go +++ b/daemon/oci_linux.go @@ -24,6 +24,7 @@ import ( "github.com/opencontainers/runc/libcontainer/devices" "github.com/opencontainers/runc/libcontainer/user" specs "github.com/opencontainers/runtime-spec/specs-go" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -419,52 +420,46 @@ func getSourceMount(source string) (string, string, error) { return "", "", fmt.Errorf("Could not find source mount of %s", source) } +const ( + sharedPropagationOption = "shared:" + slavePropagationOption = "master:" +) + +// hasMountinfoOption checks if any of the passed any of the given option values +// are set in the passed in option string. +func hasMountinfoOption(opts string, vals ...string) bool { + for _, opt := range strings.Split(opts, " ") { + for _, val := range vals { + if strings.HasPrefix(opt, val) { + return true + } + } + } + return false +} + // Ensure mount point on which path is mounted, is shared. func ensureShared(path string) error { - sharedMount := false - sourceMount, optionalOpts, err := getSourceMount(path) if err != nil { return err } // Make sure source mount point is shared. - optsSplit := strings.Split(optionalOpts, " ") - for _, opt := range optsSplit { - if strings.HasPrefix(opt, "shared:") { - sharedMount = true - break - } - } - - if !sharedMount { - return fmt.Errorf("path %s is mounted on %s but it is not a shared mount", path, sourceMount) + if !hasMountinfoOption(optionalOpts, sharedPropagationOption) { + return errors.Errorf("path %s is mounted on %s but it is not a shared mount", path, sourceMount) } return nil } // Ensure mount point on which path is mounted, is either shared or slave. func ensureSharedOrSlave(path string) error { - sharedMount := false - slaveMount := false - sourceMount, optionalOpts, err := getSourceMount(path) if err != nil { return err } - // Make sure source mount point is shared. - optsSplit := strings.Split(optionalOpts, " ") - for _, opt := range optsSplit { - if strings.HasPrefix(opt, "shared:") { - sharedMount = true - break - } else if strings.HasPrefix(opt, "master:") { - slaveMount = true - break - } - } - if !sharedMount && !slaveMount { - return fmt.Errorf("path %s is mounted on %s but it is not a shared or slave mount", path, sourceMount) + if !hasMountinfoOption(optionalOpts, sharedPropagationOption, slavePropagationOption) { + return errors.Errorf("path %s is mounted on %s but it is not a shared or slave mount", path, sourceMount) } return nil }