From 8136093f884153f8acad3602e12b1aaa64aa77e1 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 17 Jan 2018 21:17:26 -0500 Subject: [PATCH 1/2] 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 (cherry picked from commit 9803272f2db84df7955b16c0d847ad72cdc494d1) Signed-off-by: Brian Goff --- 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 | 7 ++----- 6 files changed, 7 insertions(+), 30 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 248b8bf88d..ff112cb131 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 { @@ -610,7 +606,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 57313c94c6..a2dcc0f652 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 707094af3a..24a36fb383 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 bcb21d87e4..cc3b35bf32 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 f1731ea935..7775dc2cc8 100644 --- a/components/engine/daemon/graphdriver/overlay2/overlay.go +++ b/components/engine/daemon/graphdriver/overlay2/overlay.go @@ -200,10 +200,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, @@ -334,7 +330,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 51d31bcfc5..8c8abb939c 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,9 +178,9 @@ func (d *Driver) String() string { return "zfs" } -// Cleanup is used to implement graphdriver.ProtoDriver. There is no cleanup required for this driver. +// Cleanup is called on daemon shutdown, it is used to clean up any remaining mounts func (d *Driver) Cleanup() error { - return nil + return mount.RecursiveUnmount(d.options.mountPath) } // Status returns information about the ZFS filesystem. It returns a two dimensional array of information From a22eabf65de25be051a882003abaff3b79df65d5 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 7 Feb 2018 14:49:20 -0500 Subject: [PATCH 2/2] Do not recursive unmount on cleanup of zfs/btrfs This was added in #36047 just as a way to make sure the tree is fully unmounted on shutdown. For ZFS this could be a breaking change since there was no unmount before. Someone could have setup the zfs tree themselves. It would be better, if we really do want the cleanup to actually the unpacked layers checking for mounts rather than a blind recursive unmount of the root. BTRFS does not use mounts and does not need to unmount anyway. These was only an unmount to begin with because for some reason the btrfs tree was being moutned with `private` propagation. For the other graphdrivers that still have a recursive unmount here... these were already being unmounted and performing the recursive unmount shouldn't break anything. If anyone had anything mounted at the graphdriver location it would have been unmounted on shutdown anyway. Signed-off-by: Brian Goff (cherry picked from commit 2fe4f888bee52b1f256d6fa5e20f9b061d30221c) Signed-off-by: Brian Goff --- components/engine/daemon/graphdriver/btrfs/btrfs.go | 3 +-- components/engine/daemon/graphdriver/zfs/zfs.go | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/components/engine/daemon/graphdriver/btrfs/btrfs.go b/components/engine/daemon/graphdriver/btrfs/btrfs.go index a2dcc0f652..193cddc7ff 100644 --- a/components/engine/daemon/graphdriver/btrfs/btrfs.go +++ b/components/engine/daemon/graphdriver/btrfs/btrfs.go @@ -29,7 +29,6 @@ import ( "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/containerfs" "github.com/docker/docker/pkg/idtools" - "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/system" "github.com/docker/go-units" @@ -163,7 +162,7 @@ func (d *Driver) Cleanup() error { return err } - return mount.RecursiveUnmount(d.home) + return nil } func free(p *C.char) { diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 8c8abb939c..b78104f0dd 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -178,9 +178,10 @@ func (d *Driver) String() string { return "zfs" } -// Cleanup is called on daemon shutdown, it is used to clean up any remaining mounts +// Cleanup is called on daemon shutdown, it is a no-op for ZFS. +// TODO(@cpuguy83): Walk layer tree and check mounts? func (d *Driver) Cleanup() error { - return mount.RecursiveUnmount(d.options.mountPath) + return nil } // Status returns information about the ZFS filesystem. It returns a two dimensional array of information