Split NsEnterMounter and Mounter implementation of doBindSubpath

nsenter implementation needs to mount different thing in the end and do
different checks on the result.

pkg/util/mount/BUILD

@@ -106,6 +106,7 @@ go_test(
     ] + select({
         "@io_bazel_rules_go//go/platform:linux": [
             "//vendor/github.com/golang/glog:go_default_library",
+            "//vendor/golang.org/x/sys/unix:go_default_library",
             "//vendor/k8s.io/utils/exec:go_default_library",
         ],
         "@io_bazel_rules_go//go/platform:windows": [

pkg/util/mount/mount_linux.go

@@ -720,7 +720,7 @@ func getSELinuxSupport(path string, mountInfoFilename string) (bool, error) {
 }
 
 func (mounter *Mounter) PrepareSafeSubpath(subPath Subpath) (newHostPath string, cleanupAction func(), err error) {
-	newHostPath, err = doBindSubPath(mounter, subPath, os.Getpid())
+	newHostPath, err = doBindSubPath(mounter, subPath)
 
 	// There is no action when the container starts. Bind-mount will be cleaned
 	// when container stops by CleanSubPaths.
@@ -742,6 +742,11 @@ func safeOpenSubPath(mounter Interface, subpath Subpath) (int, error) {
 
 // prepareSubpathTarget creates target for bind-mount of subpath. It returns
 // "true" when the target already exists and something is mounted there.
+// Given Subpath must have all paths with already resolved symlinks and with
+// paths relevant to kubelet (when it runs in a container).
+// This function is called also by NsEnterMounter. It works because
+// /var/lib/kubelet is mounted from the host into the container with Kubelet as
+// /var/lib/kubelet too.
 func prepareSubpathTarget(mounter Interface, subpath Subpath) (bool, string, error) {
 	// Early check for already bind-mounted subpath.
 	bindPathTarget := getSubpathBindTarget(subpath)
@@ -793,10 +798,12 @@ func getSubpathBindTarget(subpath Subpath) string {
 	return filepath.Join(subpath.PodDir, containerSubPathDirectoryName, subpath.VolumeName, subpath.ContainerName, strconv.Itoa(subpath.VolumeMountIndex))
 }
 
-// This implementation is shared between Linux and NsEnterMounter
-// kubeletPid is PID of kubelet in the PID namespace where bind-mount is done,
-// i.e. pid on the *host* if kubelet runs in a container.
-func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath string, err error) {
+func doBindSubPath(mounter Interface, subpath Subpath) (hostPath string, err error) {
+	// Linux, kubelet runs on the host:
+	// - safely open the subpath
+	// - bind-mount /proc/<pid of kubelet>/fd/<fd> to subpath target
+	// User can't change /proc/<pid of kubelet>/fd/<fd> to point to a bad place.
+
 	// Evaluate all symlinks here once for all subsequent functions.
 	newVolumePath, err := filepath.EvalSymlinks(subpath.VolumePath)
 	if err != nil {
@@ -810,7 +817,6 @@ func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath
 	subpath.VolumePath = newVolumePath
 	subpath.Path = newPath
 
-	// Check the subpath is correct and open it
 	fd, err := safeOpenSubPath(mounter, subpath)
 	if err != nil {
 		return "", err
@@ -836,6 +842,7 @@ func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath
 		}
 	}()
 
+	kubeletPid := os.Getpid()
 	mountSource := fmt.Sprintf("/proc/%d/fd/%v", kubeletPid, fd)
 
 	// Do the bind mount
@@ -1193,6 +1200,9 @@ func findExistingPrefix(base, pathname string) (string, []string, error) {
 // Symlinks are disallowed (pathname must already resolve symlinks),
 // and the path must be within the base directory.
 func doSafeOpen(pathname string, base string) (int, error) {
+	pathname = filepath.Clean(pathname)
+	base = filepath.Clean(base)
+
 	// Calculate segments to follow
 	subpath, err := filepath.Rel(base, pathname)
 	if err != nil {

pkg/util/mount/mount_linux_test.go

@@ -1291,7 +1291,7 @@ func TestBindSubPath(t *testing.T) {
 		}
 
 		_, subpathMount := getTestPaths(base)
-		bindPathTarget, err := doBindSubPath(fm, subpath, 1)
+		bindPathTarget, err := doBindSubPath(fm, subpath)
 		if test.expectError {
 			if err == nil {
 				t.Errorf("test %q failed: expected error, got success", test.name)

pkg/util/mount/nsenter_mount.go

@@ -22,13 +22,12 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"regexp"
-	"strconv"
 	"strings"
+	"syscall"
 
 	"github.com/golang/glog"
+	"golang.org/x/sys/unix"
 	utilfile "k8s.io/kubernetes/pkg/util/file"
-	utilio "k8s.io/kubernetes/pkg/util/io"
 	"k8s.io/kubernetes/pkg/util/nsenter"
 )
 
@@ -37,13 +36,6 @@ const (
 	hostProcMountsPath = "/rootfs/proc/1/mounts"
 	// hostProcMountinfoPath is the default mount info path for rootfs
 	hostProcMountinfoPath = "/rootfs/proc/1/mountinfo"
-	// hostProcSelfStatusPath is the default path to /proc/self/status on the host
-	hostProcSelfStatusPath = "/rootfs/proc/self/status"
-)
-
-var (
-	// pidRegExp matches "Pid:	<pid>" in /proc/self/status
-	pidRegExp = regexp.MustCompile(`\nPid:\t([0-9]*)\n`)
 )
 
 // Currently, all docker containers receive their own mount namespaces.
@@ -297,29 +289,9 @@ func (mounter *NsenterMounter) CleanSubPaths(podDir string, volumeName string) e
 	return doCleanSubPaths(mounter, podDir, volumeName)
 }
 
-// getPidOnHost returns kubelet's pid in the host pid namespace
-func (mounter *NsenterMounter) getPidOnHost(procStatusPath string) (int, error) {
-	// Get the PID from /rootfs/proc/self/status
-	statusBytes, err := utilio.ConsistentRead(procStatusPath, maxListTries)
-	if err != nil {
-		return 0, fmt.Errorf("error reading %s: %s", procStatusPath, err)
-	}
-	matches := pidRegExp.FindSubmatch(statusBytes)
-	if len(matches) < 2 {
-		return 0, fmt.Errorf("cannot parse %s: no Pid:", procStatusPath)
-	}
-	return strconv.Atoi(string(matches[1]))
-}
-
 func (mounter *NsenterMounter) PrepareSafeSubpath(subPath Subpath) (newHostPath string, cleanupAction func(), err error) {
-	hostPid, err := mounter.getPidOnHost(hostProcSelfStatusPath)
-	if err != nil {
-		return "", nil, err
-	}
-	glog.V(4).Infof("Kubelet's PID on the host is %d", hostPid)
-
 	// Bind-mount the subpath to avoid using symlinks in subpaths.
-	newHostPath, err = doBindSubPath(mounter, subPath, hostPid)
+	newHostPath, err = doNsEnterBindSubPath(mounter, subPath)
 
 	// There is no action when the container starts. Bind-mount will be cleaned
 	// when container stops by CleanSubPaths.
@@ -352,6 +324,95 @@ func (mounter *NsenterMounter) GetMountRefs(pathname string) ([]string, error) {
 	return getMountRefsByDev(mounter, hostpath)
 }
 
+func doNsEnterBindSubPath(mounter *NsenterMounter, subpath Subpath) (hostPath string, err error) {
+	// Linux, kubelet runs in a container:
+	// - safely open the subpath
+	// - bind-mount the subpath to target (this can be unsafe)
+	// - check that we mounted the right thing by comparing device ID and inode
+	//   of the subpath (via safely opened fd) and the target (that's under our
+	//   control)
+
+	// Evaluate all symlinks here once for all subsequent functions.
+	evaluatedHostVolumePath, err := mounter.ne.EvalSymlinks(subpath.VolumePath, true /*mustExist*/)
+	if err != nil {
+		return "", fmt.Errorf("error resolving symlinks in %q: %v", subpath.VolumePath, err)
+	}
+	evaluatedHostSubpath, err := mounter.ne.EvalSymlinks(subpath.Path, true /*mustExist*/)
+	if err != nil {
+		return "", fmt.Errorf("error resolving symlinks in %q: %v", subpath.Path, err)
+	}
+	glog.V(5).Infof("doBindSubPath %q (%q) for volumepath %q", subpath.Path, evaluatedHostSubpath, subpath.VolumePath)
+	subpath.VolumePath = mounter.ne.KubeletPath(evaluatedHostVolumePath)
+	subpath.Path = mounter.ne.KubeletPath(evaluatedHostSubpath)
+
+	// Check the subpath is correct and open it
+	fd, err := safeOpenSubPath(mounter, subpath)
+	if err != nil {
+		return "", err
+	}
+	defer syscall.Close(fd)
+
+	alreadyMounted, bindPathTarget, err := prepareSubpathTarget(mounter, subpath)
+	if err != nil {
+		return "", err
+	}
+	if alreadyMounted {
+		return bindPathTarget, nil
+	}
+
+	success := false
+	defer func() {
+		// Cleanup subpath on error
+		if !success {
+			glog.V(4).Infof("doNsEnterBindSubPath() failed for %q, cleaning up subpath", bindPathTarget)
+			if cleanErr := cleanSubPath(mounter, subpath); cleanErr != nil {
+				glog.Errorf("Failed to clean subpath %q: %v", bindPathTarget, cleanErr)
+			}
+		}
+	}()
+
+	// Leap of faith: optimistically expect that nobody has modified previously
+	// expanded evalSubPath with evil symlinks and bind-mount it.
+	// Mount is done on the host! don't use kubelet path!
+	glog.V(5).Infof("bind mounting %q at %q", evaluatedHostSubpath, bindPathTarget)
+	if err = mounter.Mount(evaluatedHostSubpath, bindPathTarget, "" /*fstype*/, []string{"bind"}); err != nil {
+		return "", fmt.Errorf("error mounting %s: %s", evaluatedHostSubpath, err)
+	}
+
+	// Check that the bind-mount target is the same inode and device as the
+	// source that we keept open, i.e. we mounted the right thing.
+	err = checkDeviceInode(fd, bindPathTarget)
+	if err != nil {
+		return "", fmt.Errorf("error checking bind mount for subpath %s: %s", subpath.VolumePath, err)
+	}
+
+	success = true
+	glog.V(3).Infof("Bound SubPath %s into %s", subpath.Path, bindPathTarget)
+	return bindPathTarget, nil
+}
+
+// checkDeviceInode checks that opened file and path represent the same file.
+func checkDeviceInode(fd int, path string) error {
+	var srcStat, dstStat unix.Stat_t
+	err := unix.Fstat(fd, &srcStat)
+	if err != nil {
+		return fmt.Errorf("error running fstat on subpath FD: %v", err)
+	}
+
+	err = unix.Stat(path, &dstStat)
+	if err != nil {
+		return fmt.Errorf("error running fstat on %s: %v", path, err)
+	}
+
+	if srcStat.Dev != dstStat.Dev {
+		return fmt.Errorf("different device number")
+	}
+	if srcStat.Ino != dstStat.Ino {
+		return fmt.Errorf("different inode")
+	}
+	return nil
+}
+
 func (mounter *NsenterMounter) GetFSGroup(pathname string) (int64, error) {
 	hostPath, err := mounter.ne.EvalSymlinks(pathname, true /* mustExist */)
 	if err != nil {

pkg/util/mount/nsenter_mount_test.go

@@ -21,9 +21,11 @@ package mount
 import (
 	"io/ioutil"
 	"os"
-	"path"
-	"strconv"
+	"path/filepath"
+	"strings"
 	"testing"
+
+	"golang.org/x/sys/unix"
 )
 
 func TestParseFindMnt(t *testing.T) {
@@ -72,120 +74,76 @@ func TestParseFindMnt(t *testing.T) {
 	}
 }
 
-func TestGetPidOnHost(t *testing.T) {
-	tempDir, err := ioutil.TempDir("", "get_pid_on_host_tests")
+func TestCheckDeviceInode(t *testing.T) {
+	testDir, err := ioutil.TempDir("", "nsenter-mounter-device-")
 	if err != nil {
-		t.Fatalf(err.Error())
+		t.Fatalf("Cannot create temporary directory: %s", err)
 	}
-	defer os.RemoveAll(tempDir)
+	defer os.RemoveAll(testDir)
 
 	tests := []struct {
 		name        string
-		procFile    string
-		expectedPid int
-		expectError bool
+		srcPath     string
+		dstPath     string
+		expectError string
 	}{
 		{
-			name: "valid status file",
-			procFile: `Name:	cat
-Umask:	0002
-State:	R (running)
-Tgid:	15041
-Ngid:	0
-Pid:	15041
-PPid:	22699
-TracerPid:	0
-Uid:	1000	1000	1000	1000
-Gid:	1000	1000	1000	1000
-FDSize:	256
-Groups:	10 135 156 157 158 973 984 1000 1001
-NStgid:	15041
-NSpid:	15041
-NSpgid:	15041
-NSsid:	22699
-VmPeak:	  115016 kB
-VmSize:	  115016 kB
-VmLck:	       0 kB
-VmPin:	       0 kB
-VmHWM:	     816 kB
-VmRSS:	     816 kB
-RssAnon:	      64 kB
-RssFile:	     752 kB
-RssShmem:	       0 kB
-VmData:	     312 kB
-VmStk:	     136 kB
-VmExe:	      32 kB
-VmLib:	    2060 kB
-VmPTE:	      44 kB
-VmPMD:	      12 kB
-VmSwap:	       0 kB
-HugetlbPages:	       0 kB
-Threads:	1
-SigQ:	2/60752
-SigPnd:	0000000000000000
-ShdPnd:	0000000000000000
-SigBlk:	0000000000000000
-SigIgn:	0000000000000000
-SigCgt:	0000000000000000
-CapInh:	0000000000000000
-CapPrm:	0000000000000000
-CapEff:	0000000000000000
-CapBnd:	0000003fffffffff
-CapAmb:	0000000000000000
-NoNewPrivs:	0
-Seccomp:	0
-Cpus_allowed:	ff
-Cpus_allowed_list:	0-7
-Mems_allowed:	00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000001
-Mems_allowed_list:	0
-voluntary_ctxt_switches:	0
-nonvoluntary_ctxt_switches:	0
-`,
-			expectedPid: 15041,
+			name:        "the same file",
+			srcPath:     filepath.Join(testDir, "1"),
+			dstPath:     filepath.Join(testDir, "1"),
+			expectError: "",
 		},
 		{
-			name: "no Pid:",
-			procFile: `Name:	cat
-Umask:	0002
-State:	R (running)
-Tgid:	15041
-Ngid:	0
-PPid:	22699
-`,
-			expectedPid: 0,
-			expectError: true,
+			name:        "different file on the same FS",
+			srcPath:     filepath.Join(testDir, "2.1"),
+			dstPath:     filepath.Join(testDir, "2.2"),
+			expectError: "different inode",
 		},
 		{
-			name: "invalid Pid:",
-			procFile: `Name:	cat
-Umask:	0002
-State:	R (running)
-Tgid:	15041
-Ngid:	0
-Pid:	invalid
-PPid:	22699
-`,
-			expectedPid: 0,
-			expectError: true,
+			name:    "different file on different device",
+			srcPath: filepath.Join(testDir, "3"),
+			// /proc is always on a different "device" than /tmp (or $TEMP)
+			dstPath:     "/proc/self/status",
+			expectError: "different device",
 		},
 	}
 
-	for i, test := range tests {
-		filename := path.Join(tempDir, strconv.Itoa(i))
-		err := ioutil.WriteFile(filename, []byte(test.procFile), 0666)
+	for _, test := range tests {
+		if err := ioutil.WriteFile(test.srcPath, []byte{}, 0644); err != nil {
+			t.Errorf("Test %q: cannot create srcPath %s: %s", test.name, test.srcPath, err)
+			continue
+		}
+
+		// Don't create dst if it exists
+		if _, err := os.Stat(test.dstPath); os.IsNotExist(err) {
+			if err := ioutil.WriteFile(test.dstPath, []byte{}, 0644); err != nil {
+				t.Errorf("Test %q: cannot create dstPath %s: %s", test.name, test.dstPath, err)
+				continue
+			}
+		} else if err != nil {
+			t.Errorf("Test %q: cannot check existence of dstPath %s: %s", test.name, test.dstPath, err)
+			continue
+		}
+
+		fd, err := unix.Open(test.srcPath, unix.O_CREAT, 0644)
 		if err != nil {
-			t.Fatalf(err.Error())
+			t.Errorf("Test %q: cannot open srcPath %s: %s", test.name, test.srcPath, err)
+			continue
 		}
-		mounter := NsenterMounter{}
-		pid, err := mounter.getPidOnHost(filename)
-		if err != nil && !test.expectError {
-			t.Errorf("Test %q: unexpected error: %s", test.name, err)
+
+		err = checkDeviceInode(fd, test.dstPath)
+
+		if test.expectError == "" && err != nil {
+			t.Errorf("Test %q: expected no error, got %s", test.name, err)
 		}
-		if err == nil && test.expectError {
-			t.Errorf("Test %q: expected error, got none", test.name)
-		}
-		if pid != test.expectedPid {
-			t.Errorf("Test %q: expected pid %d, got %d", test.name, test.expectedPid, pid)
+		if test.expectError != "" {
+			if err == nil {
+				t.Errorf("Test %q: expected error, got none", test.name)
+			} else {
+				if !strings.Contains(err.Error(), test.expectError) {
+					t.Errorf("Test %q: expected error %q, got %q", test.name, test.expectError, err)
+				}
+			}
 		}
 	}
 }