From 8b365117a20e3770065b60d4ccd7b27ff5807be0 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 7 Jul 2017 11:22:53 +0100 Subject: [PATCH 1/4] containerd-shim: Do not remount root MS_SLAVE Mounting as MS_SLAVE here breaks use cases which want to use rootPropagation=shared in order to expose mounts to the host (and other containers binding the same subtree), mounting as e.g. MS_SHARED is pointless in this context so just remove. Having done this we also need to arrange to manually clean up the mounts on delete, so do so. Note that runc will also setup root as required by rootPropagation, defaulting to MS_PRIVATE. Fixes #1132. Signed-off-by: Ian Campbell --- cmd/containerd-shim/main_unix.go | 3 -- cmd/containerd-shim/shim_linux.go | 6 ---- cmd/containerd-shim/shim_unix.go | 5 --- linux/shim/init.go | 59 ++++++++++++++++++++++++------- mount/mount.go | 16 +++++++++ 5 files changed, 62 insertions(+), 27 deletions(-) diff --git a/cmd/containerd-shim/main_unix.go b/cmd/containerd-shim/main_unix.go index 16889183b..8a13299e4 100644 --- a/cmd/containerd-shim/main_unix.go +++ b/cmd/containerd-shim/main_unix.go @@ -67,9 +67,6 @@ func main() { if err != nil { return err } - if err := setupRoot(); err != nil { - return err - } path, err := os.Getwd() if err != nil { return err diff --git a/cmd/containerd-shim/shim_linux.go b/cmd/containerd-shim/shim_linux.go index 10d51b78b..43ab3db1b 100644 --- a/cmd/containerd-shim/shim_linux.go +++ b/cmd/containerd-shim/shim_linux.go @@ -10,7 +10,6 @@ import ( "google.golang.org/grpc/credentials" "golang.org/x/net/context" - "golang.org/x/sys/unix" "github.com/containerd/containerd/reaper" "github.com/containerd/containerd/sys" @@ -33,11 +32,6 @@ func setupSignals() (chan os.Signal, error) { return signals, nil } -// setupRoot sets up the root as the shim is started in its own mount namespace -func setupRoot() error { - return unix.Mount("", "/", "", unix.MS_SLAVE|unix.MS_REC, "") -} - func newServer() *grpc.Server { return grpc.NewServer(grpc.Creds(NewUnixSocketCredentils(0, 0))) } diff --git a/cmd/containerd-shim/shim_unix.go b/cmd/containerd-shim/shim_unix.go index 110123d7f..b6bf2a6a0 100644 --- a/cmd/containerd-shim/shim_unix.go +++ b/cmd/containerd-shim/shim_unix.go @@ -23,11 +23,6 @@ func setupSignals() (chan os.Signal, error) { return signals, nil } -// setupRoot is a no op except on Linux -func setupRoot() error { - return nil -} - func newServer() *grpc.Server { return grpc.NewServer() } diff --git a/linux/shim/init.go b/linux/shim/init.go index 41c7d96ad..9e40daaac 100644 --- a/linux/shim/init.go +++ b/linux/shim/init.go @@ -39,17 +39,19 @@ type initProcess struct { // the reaper interface. mu sync.Mutex - id string - bundle string - console console.Console - io runc.IO - runtime *runc.Runc - status int - exited time.Time - pid int - closers []io.Closer - stdin io.Closer - stdio stdio + id string + bundle string + console console.Console + io runc.IO + runtime *runc.Runc + status int + exited time.Time + pid int + closers []io.Closer + stdin io.Closer + stdio stdio + rootfs string + nrRootMounts int // Number of rootfs overmounts } func newInitProcess(context context.Context, path, namespace string, r *shimapi.CreateTaskRequest) (*initProcess, error) { @@ -64,15 +66,27 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. } options = *v.(*runcopts.CreateOptions) } + + rootfs := filepath.Join(path, "rootfs") + // count the number of successful mounts so we can undo + // what was actually done rather than what should have been + // done. + nrRootMounts := 0 for _, rm := range r.Rootfs { m := &mount.Mount{ Type: rm.Type, Source: rm.Source, Options: rm.Options, } - if err := m.Mount(filepath.Join(path, "rootfs")); err != nil { + if err := m.Mount(rootfs); err != nil { return nil, errors.Wrapf(err, "failed to mount rootfs component %v", m) } + nrRootMounts++ + } + cleanupMounts := func() { + if err2 := mount.UnmountN(rootfs, 0, nrRootMounts); err2 != nil { + log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") + } } runtime := &runc.Runc{ Command: r.Runtime, @@ -91,6 +105,8 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. stderr: r.Stderr, terminal: r.Terminal, }, + rootfs: rootfs, + nrRootMounts: nrRootMounts, } var ( err error @@ -99,12 +115,14 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. ) if r.Terminal { if socket, err = runc.NewConsoleSocket(filepath.Join(path, "pty.sock")); err != nil { + cleanupMounts() return nil, errors.Wrap(err, "failed to create OCI runtime console socket") } defer os.Remove(socket.Path()) } else { // TODO: get uid/gid if io, err = runc.NewPipeIO(0, 0); err != nil { + cleanupMounts() return nil, errors.Wrap(err, "failed to create OCI runtime io pipes") } p.io = io @@ -124,6 +142,7 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. NoSubreaper: true, } if _, err := p.runtime.Restore(context, r.ID, r.Bundle, opts); err != nil { + cleanupMounts() return nil, p.runtimeError(err, "OCI runtime restore failed") } } else { @@ -137,6 +156,7 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. opts.ConsoleSocket = socket } if err := p.runtime.Create(context, r.ID, r.Bundle, opts); err != nil { + cleanupMounts() return nil, p.runtimeError(err, "OCI runtime create failed") } } @@ -152,14 +172,17 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. if socket != nil { console, err := socket.ReceiveMaster() if err != nil { + cleanupMounts() return nil, errors.Wrap(err, "failed to retrieve console master") } p.console = console if err := copyConsole(context, console, r.Stdin, r.Stdout, r.Stderr, &p.WaitGroup, ©WaitGroup); err != nil { + cleanupMounts() return nil, errors.Wrap(err, "failed to start console copy") } } else { if err := copyPipes(context, io, r.Stdin, r.Stdout, r.Stderr, &p.WaitGroup, ©WaitGroup); err != nil { + cleanupMounts() return nil, errors.Wrap(err, "failed to start io pipe copy") } } @@ -167,6 +190,7 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. copyWaitGroup.Wait() pid, err := runc.ReadPidFile(pidFile) if err != nil { + cleanupMounts() return nil, errors.Wrap(err, "failed to retrieve OCI runtime container pid") } p.pid = pid @@ -229,7 +253,16 @@ func (p *initProcess) Delete(context context.Context) error { } p.io.Close() } - return p.runtimeError(err, "OCI runtime delete failed") + err = p.runtimeError(err, "OCI runtime delete failed") + + if err2 := mount.UnmountN(p.rootfs, 0, p.nrRootMounts); err2 != nil { + log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") + if err == nil { + err = errors.Wrap(err2, "Failed rootfs umount") + } + } + + return err } func (p *initProcess) Resize(ws console.WinSize) error { diff --git a/mount/mount.go b/mount/mount.go index 94086f178..2ef4a0164 100644 --- a/mount/mount.go +++ b/mount/mount.go @@ -22,3 +22,19 @@ func MountAll(mounts []Mount, target string) error { } return nil } + +// UnmountN tries to unmount the given mount point nr times, which is +// useful for undoing a stack of mounts on the same mount +// point. Returns the first error encountered, but always attempts the +// full nr umounts. +func UnmountN(mount string, flags, nr int) error { + var err error + for i := 0; i < nr; i++ { + if err2 := Unmount(mount, flags); err2 != nil { + if err == nil { + err = err2 + } + } + } + return err +} From 300f08312714f01104c0858cc26bb4fa70dec914 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Wed, 12 Jul 2017 15:36:00 +0100 Subject: [PATCH 2/4] Cleanup mounts if we fail to mount one element of rootfs Signed-off-by: Ian Campbell --- linux/shim/init.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/linux/shim/init.go b/linux/shim/init.go index 9e40daaac..8d6e1bfb1 100644 --- a/linux/shim/init.go +++ b/linux/shim/init.go @@ -72,6 +72,11 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. // what was actually done rather than what should have been // done. nrRootMounts := 0 + cleanupMounts := func() { + if err2 := mount.UnmountN(rootfs, 0, nrRootMounts); err2 != nil { + log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") + } + } for _, rm := range r.Rootfs { m := &mount.Mount{ Type: rm.Type, @@ -79,15 +84,11 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. Options: rm.Options, } if err := m.Mount(rootfs); err != nil { + cleanupMounts() return nil, errors.Wrapf(err, "failed to mount rootfs component %v", m) } nrRootMounts++ } - cleanupMounts := func() { - if err2 := mount.UnmountN(rootfs, 0, nrRootMounts); err2 != nil { - log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") - } - } runtime := &runc.Runc{ Command: r.Runtime, Log: filepath.Join(path, "log.json"), From d63d2ecf6c24975c50fccb2c8d70dbaa9585f0a8 Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Wed, 12 Jul 2017 16:11:38 +0100 Subject: [PATCH 3/4] Simplify mount cleanup on failure by using defer This avoids someone adding a new error path and forgetting to call the cleanup function. We prefer to use an explicit flag to gate the clean rather than relying on `err != nil` so we don't have to rely on people never accidentally shadowing the `err` as seen by the closure. Signed-off-by: Ian Campbell --- linux/shim/init.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/linux/shim/init.go b/linux/shim/init.go index 8d6e1bfb1..f54daaaf7 100644 --- a/linux/shim/init.go +++ b/linux/shim/init.go @@ -55,6 +55,8 @@ type initProcess struct { } func newInitProcess(context context.Context, path, namespace string, r *shimapi.CreateTaskRequest) (*initProcess, error) { + var success bool + if err := identifiers.Validate(r.ID); err != nil { return nil, errors.Wrapf(err, "invalid task id") } @@ -72,11 +74,14 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. // what was actually done rather than what should have been // done. nrRootMounts := 0 - cleanupMounts := func() { + defer func() { + if success { + return + } if err2 := mount.UnmountN(rootfs, 0, nrRootMounts); err2 != nil { log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") } - } + }() for _, rm := range r.Rootfs { m := &mount.Mount{ Type: rm.Type, @@ -84,7 +89,6 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. Options: rm.Options, } if err := m.Mount(rootfs); err != nil { - cleanupMounts() return nil, errors.Wrapf(err, "failed to mount rootfs component %v", m) } nrRootMounts++ @@ -116,14 +120,12 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. ) if r.Terminal { if socket, err = runc.NewConsoleSocket(filepath.Join(path, "pty.sock")); err != nil { - cleanupMounts() return nil, errors.Wrap(err, "failed to create OCI runtime console socket") } defer os.Remove(socket.Path()) } else { // TODO: get uid/gid if io, err = runc.NewPipeIO(0, 0); err != nil { - cleanupMounts() return nil, errors.Wrap(err, "failed to create OCI runtime io pipes") } p.io = io @@ -143,7 +145,6 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. NoSubreaper: true, } if _, err := p.runtime.Restore(context, r.ID, r.Bundle, opts); err != nil { - cleanupMounts() return nil, p.runtimeError(err, "OCI runtime restore failed") } } else { @@ -157,7 +158,6 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. opts.ConsoleSocket = socket } if err := p.runtime.Create(context, r.ID, r.Bundle, opts); err != nil { - cleanupMounts() return nil, p.runtimeError(err, "OCI runtime create failed") } } @@ -173,17 +173,14 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. if socket != nil { console, err := socket.ReceiveMaster() if err != nil { - cleanupMounts() return nil, errors.Wrap(err, "failed to retrieve console master") } p.console = console if err := copyConsole(context, console, r.Stdin, r.Stdout, r.Stderr, &p.WaitGroup, ©WaitGroup); err != nil { - cleanupMounts() return nil, errors.Wrap(err, "failed to start console copy") } } else { if err := copyPipes(context, io, r.Stdin, r.Stdout, r.Stderr, &p.WaitGroup, ©WaitGroup); err != nil { - cleanupMounts() return nil, errors.Wrap(err, "failed to start io pipe copy") } } @@ -191,10 +188,10 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. copyWaitGroup.Wait() pid, err := runc.ReadPidFile(pidFile) if err != nil { - cleanupMounts() return nil, errors.Wrap(err, "failed to retrieve OCI runtime container pid") } p.pid = pid + success = true return p, nil } From d42cb88ba2b08d2ca6c8c017d629b394bf1dd08c Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Thu, 13 Jul 2017 09:59:19 +0100 Subject: [PATCH 4/4] Loop umount'ing rootfs until there are no more mounts This is simpler than trying to count how many successful mounts we made. Signed-off-by: Ian Campbell --- linux/shim/init.go | 34 +++++++++++++++------------------- mount/mount.go | 16 ---------------- mount/mount_linux.go | 19 +++++++++++++++++++ mount/mount_unix.go | 4 ++++ mount/mount_windows.go | 4 ++++ 5 files changed, 42 insertions(+), 35 deletions(-) diff --git a/linux/shim/init.go b/linux/shim/init.go index f54daaaf7..f34defe70 100644 --- a/linux/shim/init.go +++ b/linux/shim/init.go @@ -39,19 +39,18 @@ type initProcess struct { // the reaper interface. mu sync.Mutex - id string - bundle string - console console.Console - io runc.IO - runtime *runc.Runc - status int - exited time.Time - pid int - closers []io.Closer - stdin io.Closer - stdio stdio - rootfs string - nrRootMounts int // Number of rootfs overmounts + id string + bundle string + console console.Console + io runc.IO + runtime *runc.Runc + status int + exited time.Time + pid int + closers []io.Closer + stdin io.Closer + stdio stdio + rootfs string } func newInitProcess(context context.Context, path, namespace string, r *shimapi.CreateTaskRequest) (*initProcess, error) { @@ -73,12 +72,11 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. // count the number of successful mounts so we can undo // what was actually done rather than what should have been // done. - nrRootMounts := 0 defer func() { if success { return } - if err2 := mount.UnmountN(rootfs, 0, nrRootMounts); err2 != nil { + if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") } }() @@ -91,7 +89,6 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. if err := m.Mount(rootfs); err != nil { return nil, errors.Wrapf(err, "failed to mount rootfs component %v", m) } - nrRootMounts++ } runtime := &runc.Runc{ Command: r.Runtime, @@ -110,8 +107,7 @@ func newInitProcess(context context.Context, path, namespace string, r *shimapi. stderr: r.Stderr, terminal: r.Terminal, }, - rootfs: rootfs, - nrRootMounts: nrRootMounts, + rootfs: rootfs, } var ( err error @@ -253,7 +249,7 @@ func (p *initProcess) Delete(context context.Context) error { } err = p.runtimeError(err, "OCI runtime delete failed") - if err2 := mount.UnmountN(p.rootfs, 0, p.nrRootMounts); err2 != nil { + if err2 := mount.UnmountAll(p.rootfs, 0); err2 != nil { log.G(context).WithError(err2).Warn("Failed to cleanup rootfs mount") if err == nil { err = errors.Wrap(err2, "Failed rootfs umount") diff --git a/mount/mount.go b/mount/mount.go index 2ef4a0164..94086f178 100644 --- a/mount/mount.go +++ b/mount/mount.go @@ -22,19 +22,3 @@ func MountAll(mounts []Mount, target string) error { } return nil } - -// UnmountN tries to unmount the given mount point nr times, which is -// useful for undoing a stack of mounts on the same mount -// point. Returns the first error encountered, but always attempts the -// full nr umounts. -func UnmountN(mount string, flags, nr int) error { - var err error - for i := 0; i < nr; i++ { - if err2 := Unmount(mount, flags); err2 != nil { - if err == nil { - err = err2 - } - } - } - return err -} diff --git a/mount/mount_linux.go b/mount/mount_linux.go index 86df8bbc1..5995051b8 100644 --- a/mount/mount_linux.go +++ b/mount/mount_linux.go @@ -15,6 +15,25 @@ func Unmount(mount string, flags int) error { return unix.Unmount(mount, flags) } +// UnmountAll repeatedly unmounts the given mount point until there +// are no mounts remaining (EINVAL is returned by mount), which is +// useful for undoing a stack of mounts on the same mount point. +func UnmountAll(mount string, flags int) error { + for { + if err := Unmount(mount, flags); err != nil { + // EINVAL is returned if the target is not a + // mount point, indicating that we are + // done. It can also indicate a few other + // things (such as invalid flags) which we + // unfortunately end up squelching here too. + if err == unix.EINVAL { + return nil + } + return err + } + } +} + // parseMountOptions takes fstab style mount options and parses them for // use with a standard mount() syscall func parseMountOptions(options []string) (int, string) { diff --git a/mount/mount_unix.go b/mount/mount_unix.go index 32ea2691a..23467a8cc 100644 --- a/mount/mount_unix.go +++ b/mount/mount_unix.go @@ -15,3 +15,7 @@ func (m *Mount) Mount(target string) error { func Unmount(mount string, flags int) error { return ErrNotImplementOnUnix } + +func UnmountAll(mount string, flags int) error { + return ErrNotImplementOnUnix +} diff --git a/mount/mount_windows.go b/mount/mount_windows.go index 35dead411..8eeca6817 100644 --- a/mount/mount_windows.go +++ b/mount/mount_windows.go @@ -13,3 +13,7 @@ func (m *Mount) Mount(target string) error { func Unmount(mount string, flags int) error { return ErrNotImplementOnWindows } + +func UnmountAll(mount string, flags int) error { + return ErrNotImplementOnWindows +}