From 9b47a9d16f0a160fb1b17cf7c97faad5aa7928d9 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 17 Jan 2018 21:17:26 -0500 Subject: [PATCH] Do not make graphdriver homes private mounts. The idea behind making the graphdrivers private is to prevent leaking mounts into other namespaces. Unfortunately this is not really what happens. There is one case where this does work, and that is when the namespace was created before the daemon's namespace. However with systemd each system servie winds up with it's own mount namespace. This causes a race betwen daemon startup and other system services as to if the mount is actually private. This also means there is a negative impact when other system services are started while the daemon is running. Basically there are too many things that the daemon does not have control over (nor should it) to be able to protect against these kinds of leakages. One thing is certain, setting the graphdriver roots to private disconnects the mount ns heirarchy preventing propagation of unmounts... new mounts are of course not propagated either, but the behavior is racey (or just bad in the case of restarting services)... so it's better to just be able to keep mount propagation in tact. It also does not protect situations like `-v /var/lib/docker:/var/lib/docker` where all mounts are recursively bound into the container anyway. Signed-off-by: Brian Goff Upstream-commit: 9803272f2db84df7955b16c0d847ad72cdc494d1 Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 6 +----- components/engine/daemon/graphdriver/btrfs/btrfs.go | 6 +----- components/engine/daemon/graphdriver/devmapper/driver.go | 6 +----- components/engine/daemon/graphdriver/overlay/overlay.go | 6 +----- components/engine/daemon/graphdriver/overlay2/overlay.go | 6 +----- components/engine/daemon/graphdriver/zfs/zfs.go | 8 ++------ 6 files changed, 7 insertions(+), 31 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index f019e95558..393e548091 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -136,10 +136,6 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } - if err := mountpk.MakePrivate(root); err != nil { - return nil, err - } - // Populate the dir structure for _, p := range paths { if err := idtools.MkdirAllAndChown(path.Join(root, p), 0700, idtools.IDPair{UID: rootUID, GID: rootGID}); err != nil { @@ -607,7 +603,7 @@ func (a *Driver) Cleanup() error { logrus.Debugf("aufs error unmounting %s: %s", m, err) } } - return mountpk.Unmount(a.root) + return mountpk.RecursiveUnmount(a.root) } func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err error) { diff --git a/components/engine/daemon/graphdriver/btrfs/btrfs.go b/components/engine/daemon/graphdriver/btrfs/btrfs.go index d4b9b89124..54bb9b7902 100644 --- a/components/engine/daemon/graphdriver/btrfs/btrfs.go +++ b/components/engine/daemon/graphdriver/btrfs/btrfs.go @@ -77,10 +77,6 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } - if err := mount.MakePrivate(home); err != nil { - return nil, err - } - opt, userDiskQuota, err := parseOptions(options) if err != nil { return nil, err @@ -167,7 +163,7 @@ func (d *Driver) Cleanup() error { return err } - return mount.Unmount(d.home) + return mount.RecursiveUnmount(d.home) } func free(p *C.char) { diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index ab75974a98..23b34b0b52 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -42,10 +42,6 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } - if err := mount.MakePrivate(home); err != nil { - return nil, err - } - d := &Driver{ DeviceSet: deviceSet, home: home, @@ -127,7 +123,7 @@ func (d *Driver) GetMetadata(id string) (map[string]string, error) { func (d *Driver) Cleanup() error { err := d.DeviceSet.Shutdown(d.home) - if err2 := mount.Unmount(d.home); err == nil { + if err2 := mount.RecursiveUnmount(d.home); err == nil { err = err2 } diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index e2789aff6d..6b2e1dd4ad 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -164,10 +164,6 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } - if err := mount.MakePrivate(home); err != nil { - return nil, err - } - d := &Driver{ home: home, uidMaps: uidMaps, @@ -248,7 +244,7 @@ func (d *Driver) GetMetadata(id string) (map[string]string, error) { // is being shutdown. For now, we just have to unmount the bind mounted // we had created. func (d *Driver) Cleanup() error { - return mount.Unmount(d.home) + return mount.RecursiveUnmount(d.home) } // CreateReadWrite creates a layer that is writable for use as a container diff --git a/components/engine/daemon/graphdriver/overlay2/overlay.go b/components/engine/daemon/graphdriver/overlay2/overlay.go index 4b596ae33e..ce29f16d70 100644 --- a/components/engine/daemon/graphdriver/overlay2/overlay.go +++ b/components/engine/daemon/graphdriver/overlay2/overlay.go @@ -201,10 +201,6 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } - if err := mount.MakePrivate(home); err != nil { - return nil, err - } - d := &Driver{ home: home, uidMaps: uidMaps, @@ -335,7 +331,7 @@ func (d *Driver) GetMetadata(id string) (map[string]string, error) { // is being shutdown. For now, we just have to unmount the bind mounted // we had created. func (d *Driver) Cleanup() error { - return mount.Unmount(d.home) + return mount.RecursiveUnmount(d.home) } // CreateReadWrite creates a layer that is writable for use as a container diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 8b3f78d829..9ab5d87ad0 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -108,9 +108,6 @@ func Init(base string, opt []string, uidMaps, gidMaps []idtools.IDMap) (graphdri return nil, fmt.Errorf("Failed to create '%s': %v", base, err) } - if err := mount.MakePrivate(base); err != nil { - return nil, err - } d := &Driver{ dataset: rootDataset, options: options, @@ -181,10 +178,9 @@ func (d *Driver) String() string { return "zfs" } -// Cleanup is called on daemon shutdown. It unmounts the bind mount -// created by mount.MakePrivate() in Init(). +// Cleanup is called on daemon shutdown, it is used to clean up any remaining mounts func (d *Driver) Cleanup() error { - return mount.Unmount(d.options.mountPath) + return mount.RecursiveUnmount(d.options.mountPath) } // Status returns information about the ZFS filesystem. It returns a two dimensional array of information