Merge pull request #34573 from cyphar/dm-dos-prevention-remove-mountpoint

devicemapper: remove container rootfs mountPath after umount

daemon/graphdriver/devmapper/deviceset.go

@@ -2428,6 +2428,18 @@ func (devices *DeviceSet) UnmountDevice(hash, mountPath string) error {
 	}
 	logrus.Debug("devmapper: Unmount done")
 
+	// Remove the mountpoint here. Removing the mountpoint (in newer kernels)
+	// will cause all other instances of this mount in other mount namespaces
+	// to be killed (this is an anti-DoS measure that is necessary for things
+	// like devicemapper). This is necessary to avoid cases where a libdm mount
+	// that is present in another namespace will cause subsequent RemoveDevice
+	// operations to fail. We ignore any errors here because this may fail on
+	// older kernels which don't have
+	// torvalds/linux@8ed936b5671bfb33d89bc60bdcc7cf0470ba52fe applied.
+	if err := os.Remove(mountPath); err != nil {
+		logrus.Debugf("devmapper: error doing a remove on unmounted device %s: %v", mountPath, err)
+	}
+
 	return devices.deactivateDevice(info)
 }
 

daemon/graphdriver/devmapper/devmapper_test.go

@@ -5,12 +5,15 @@ package devmapper
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"syscall"
 	"testing"
 	"time"
 
 	"github.com/docker/docker/daemon/graphdriver"
 	"github.com/docker/docker/daemon/graphdriver/graphtest"
+	"github.com/docker/docker/pkg/parsers/kernel"
+	"golang.org/x/sys/unix"
 )
 
 func init() {
@@ -150,3 +153,53 @@ func TestDevmapperLockReleasedDeviceDeletion(t *testing.T) {
 	case <-doneChan:
 	}
 }
+
+// Ensure that mounts aren't leakedriver. It's non-trivial for us to test the full
+// reproducer of #34573 in a unit test, but we can at least make sure that a
+// simple command run in a new namespace doesn't break things horribly.
+func TestDevmapperMountLeaks(t *testing.T) {
+	if !kernel.CheckKernelVersion(3, 18, 0) {
+		t.Skipf("kernel version <3.18.0 and so is missing torvalds/linux@8ed936b5671bfb33d89bc60bdcc7cf0470ba52fe.")
+	}
+
+	driver := graphtest.GetDriver(t, "devicemapper", "dm.use_deferred_removal=false", "dm.use_deferred_deletion=false").(*graphtest.Driver).Driver.(*graphdriver.NaiveDiffDriver).ProtoDriver.(*Driver)
+	defer graphtest.PutDriver(t)
+
+	// We need to create a new (dummy) device.
+	if err := driver.Create("some-layer", "", nil); err != nil {
+		t.Fatalf("setting up some-layer: %v", err)
+	}
+
+	// Mount the device.
+	_, err := driver.Get("some-layer", "")
+	if err != nil {
+		t.Fatalf("mounting some-layer: %v", err)
+	}
+
+	// Create a new subprocess which will inherit our mountpoint, then
+	// intentionally leak it and stick around. We can't do this entirely within
+	// Go because forking and namespaces in Go are really not handled well at
+	// all.
+	cmd := exec.Cmd{
+		Path: "/bin/sh",
+		Args: []string{
+			"/bin/sh", "-c",
+			"mount --make-rprivate / && sleep 1000s",
+		},
+		SysProcAttr: &syscall.SysProcAttr{
+			Unshareflags: syscall.CLONE_NEWNS,
+		},
+	}
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("starting sub-command: %v", err)
+	}
+	defer func() {
+		unix.Kill(cmd.Process.Pid, unix.SIGKILL)
+		cmd.Wait()
+	}()
+
+	// Now try to "drop" the device.
+	if err := driver.Put("some-layer"); err != nil {
+		t.Fatalf("unmounting some-layer: %v", err)
+	}
+}

daemon/graphdriver/devmapper/driver.go

@@ -232,10 +232,12 @@ func (d *Driver) Put(id string) error {
 	if count := d.ctr.Decrement(mp); count > 0 {
 		return nil
 	}
+
 	err := d.DeviceSet.UnmountDevice(id, mp)
 	if err != nil {
-		logrus.Errorf("devmapper: Error unmounting device %s: %s", id, err)
+		logrus.Errorf("devmapper: Error unmounting device %s: %v", id, err)
 	}
+
 	return err
 }