libct: add mountViaFDs, simplify mount

1. Simplify mount call by removing the procfd argument, and use the new
   mount() where procfd is not used. Now, the mount() arguments are the
   same as for unix.Mount.

2. Introduce a new mountViaFDs function, which is similar to the old
   mount(), except it can take procfd for both source and target.
   The new arguments are called srcFD and dstFD.

3. Modify the mount error to show both srcFD and dstFD so it's clear
   which one is used for which purpose. This fixes the issue of having
   a somewhat cryptic errors like this:

> mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted

  (in which fd 11 is actually the source, and fd 12 is the target).

   After this change, it looks like

> mount src=/proc/self/fd/11, dst=/sys/fs/cgroup/systemd, dstFD=/proc/self/fd/12, flags=0x20502f: operation not permitted

   so it's clear that 12 is a destination fd.

4. Fix the mountViaFDs callers to use dstFD (rather than procfd) for the
   variable name.

5. Use srcFD where mountFd is set.

Signed-off-by: Kir Kolyshkin <[email protected]>

Author: kolyshkin

libcontainer/console_linux.go

@@ -18,7 +18,7 @@ func mountConsole(slavePath string) error {
 	if f != nil {
 		f.Close()
 	}
-	return mount(slavePath, "/dev/console", "", "bind", unix.MS_BIND, "")
+	return mount(slavePath, "/dev/console", "bind", unix.MS_BIND, "")
 }
 
 // dupStdio opens the slavePath for the console and dups the fds to the current

libcontainer/container_linux.go

@@ -1357,8 +1357,8 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error {
 			// because during initial container creation mounts are
 			// set up in the order they are configured.
 			if m.Device == "bind" {
-				if err := utils.WithProcfd(c.config.Rootfs, m.Destination, func(procfd string) error {
-					if err := mount(m.Source, m.Destination, procfd, "", unix.MS_BIND|unix.MS_REC, ""); err != nil {
+				if err := utils.WithProcfd(c.config.Rootfs, m.Destination, func(dstFD string) error {
+					if err := mountViaFDs(m.Source, "", m.Destination, dstFD, "", unix.MS_BIND|unix.MS_REC, ""); err != nil {
 						return err
 					}
 					return nil
@@ -1411,7 +1411,7 @@ func (c *Container) Restore(process *Process, criuOpts *CriuOpts) error {
 	if err != nil {
 		return err
 	}
-	err = mount(c.config.Rootfs, root, "", "", unix.MS_BIND|unix.MS_REC, "")
+	err = mount(c.config.Rootfs, root, "", unix.MS_BIND|unix.MS_REC, "")
 	if err != nil {
 		return err
 	}

libcontainer/mount_linux.go

@@ -10,8 +10,9 @@ import (
 type mountError struct {
 	op     string
 	source string
+	srcFD  string
 	target string
-	procfd string
+	dstFD  string
 	flags  uintptr
 	data   string
 	err    error
@@ -22,19 +23,21 @@ func (e *mountError) Error() string {
 	out := e.op + " "
 
 	if e.source != "" {
-		out += e.source + ":" + e.target
-	} else {
-		out += e.target
+		out += "src=" + e.source + ", "
+		if e.srcFD != "" {
+			out += "srcFD=" + e.srcFD + ", "
+		}
 	}
-	if e.procfd != "" {
-		out += " (via " + e.procfd + ")"
+	out += "dst=" + e.target
+	if e.dstFD != "" {
+		out += ", dstFD=" + e.dstFD
 	}
 
 	if e.flags != uintptr(0) {
-		out += ", flags: 0x" + strconv.FormatUint(uint64(e.flags), 16)
+		out += ", flags=0x" + strconv.FormatUint(uint64(e.flags), 16)
 	}
 	if e.data != "" {
-		out += ", data: " + e.data
+		out += ", data=" + e.data
 	}
 
 	out += ": " + e.err.Error()
@@ -47,19 +50,36 @@ func (e *mountError) Unwrap() error {
 	return e.err
 }
 
-// mount is a simple unix.Mount wrapper. If procfd is not empty, it is used
-// instead of target (and the target is only used to add context to an error).
-func mount(source, target, procfd, fstype string, flags uintptr, data string) error {
+// mount is a simple unix.Mount wrapper, returning an error with more context
+// in case it failed.
+func mount(source, target, fstype string, flags uintptr, data string) error {
+	return mountViaFDs(source, "", target, "", fstype, flags, data)
+}
+
+// mountViaFDs is a unix.Mount wrapper which uses srcFD instead of source,
+// and dstFD instead of target, unless those are empty. The *FD arguments,
+// if non-empty, are expected to be in the form of a path to an opened file
+// descriptor on procfs (i.e. "/proc/self/fd/NN").
+//
+// If case an FD is used instead of a source or a target path, the
+// corresponding path is only used to add context to an error in case
+// the mount operation has failed.
+func mountViaFDs(source, srcFD, target, dstFD, fstype string, flags uintptr, data string) error {
+	src := source
+	if srcFD != "" {
+		src = srcFD
+	}
 	dst := target
-	if procfd != "" {
-		dst = procfd
+	if dstFD != "" {
+		dst = dstFD
 	}
-	if err := unix.Mount(source, dst, fstype, flags, data); err != nil {
+	if err := unix.Mount(src, dst, fstype, flags, data); err != nil {
 		return &mountError{
 			op:     "mount",
 			source: source,
+			srcFD:  srcFD,
 			target: target,
-			procfd: procfd,
+			dstFD:  dstFD,
 			flags:  flags,
 			data:   data,
 			err:    err,

libcontainer/rootfs_linux.go

@@ -187,10 +187,10 @@ func prepareTmp(topTmpDir string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	if err := mount(tmpdir, tmpdir, "", "bind", unix.MS_BIND, ""); err != nil {
+	if err := mount(tmpdir, tmpdir, "bind", unix.MS_BIND, ""); err != nil {
 		return "", err
 	}
-	if err := mount("", tmpdir, "", "", uintptr(unix.MS_PRIVATE), ""); err != nil {
+	if err := mount("", tmpdir, "", uintptr(unix.MS_PRIVATE), ""); err != nil {
 		return "", err
 	}
 	return tmpdir, nil
@@ -262,7 +262,7 @@ func mountCgroupV1(m *configs.Mount, c *mountConfig) error {
 			if err := os.MkdirAll(subsystemPath, 0o755); err != nil {
 				return err
 			}
-			if err := utils.WithProcfd(c.root, b.Destination, func(procfd string) error {
+			if err := utils.WithProcfd(c.root, b.Destination, func(dstFD string) error {
 				flags := defaultMountFlags
 				if m.Flags&unix.MS_RDONLY != 0 {
 					flags = flags | unix.MS_RDONLY
@@ -275,7 +275,7 @@ func mountCgroupV1(m *configs.Mount, c *mountConfig) error {
 					data = cgroups.CgroupNamePrefix + data
 					source = "systemd"
 				}
-				return mount(source, b.Destination, procfd, "cgroup", uintptr(flags), data)
+				return mountViaFDs(source, "", b.Destination, dstFD, "cgroup", uintptr(flags), data)
 			}); err != nil {
 				return err
 			}
@@ -306,8 +306,8 @@ func mountCgroupV2(m *configs.Mount, c *mountConfig) error {
 	if err := os.MkdirAll(dest, 0o755); err != nil {
 		return err
 	}
-	err = utils.WithProcfd(c.root, m.Destination, func(procfd string) error {
-		return mount(m.Source, m.Destination, procfd, "cgroup2", uintptr(m.Flags), m.Data)
+	err = utils.WithProcfd(c.root, m.Destination, func(dstFD string) error {
+		return mountViaFDs(m.Source, "", m.Destination, dstFD, "cgroup2", uintptr(m.Flags), m.Data)
 	})
 	if err == nil || !(errors.Is(err, unix.EPERM) || errors.Is(err, unix.EBUSY)) {
 		return err
@@ -373,15 +373,15 @@ func doTmpfsCopyUp(m *configs.Mount, rootfs, mountLabel string) (Err error) {
 		}
 	}()
 
-	return utils.WithProcfd(rootfs, m.Destination, func(procfd string) (Err error) {
+	return utils.WithProcfd(rootfs, m.Destination, func(dstFD string) (Err error) {
 		// Copy the container data to the host tmpdir. We append "/" to force
 		// CopyDirectory to resolve the symlink rather than trying to copy the
 		// symlink itself.
-		if err := fileutils.CopyDirectory(procfd+"/", tmpDir); err != nil {
-			return fmt.Errorf("tmpcopyup: failed to copy %s to %s (%s): %w", m.Destination, procfd, tmpDir, err)
+		if err := fileutils.CopyDirectory(dstFD+"/", tmpDir); err != nil {
+			return fmt.Errorf("tmpcopyup: failed to copy %s to %s (%s): %w", m.Destination, dstFD, tmpDir, err)
 		}
 		// Now move the mount into the container.
-		if err := mount(tmpDir, m.Destination, procfd, "", unix.MS_MOVE, ""); err != nil {
+		if err := mountViaFDs(tmpDir, "", m.Destination, dstFD, "", unix.MS_MOVE, ""); err != nil {
 			return fmt.Errorf("tmpcopyup: failed to move mount: %w", err)
 		}
 		return nil
@@ -688,8 +688,8 @@ func bindMountDeviceNode(rootfs, dest string, node *devices.Device) error {
 	if f != nil {
 		_ = f.Close()
 	}
-	return utils.WithProcfd(rootfs, dest, func(procfd string) error {
-		return mount(node.Path, dest, procfd, "bind", unix.MS_BIND, "")
+	return utils.WithProcfd(rootfs, dest, func(dstFD string) error {
+		return mountViaFDs(node.Path, "", dest, dstFD, "bind", unix.MS_BIND, "")
 	})
 }
 
@@ -786,7 +786,7 @@ func rootfsParentMountPrivate(rootfs string) error {
 	// shared. Secondly when we bind mount rootfs it will propagate to
 	// parent namespace and we don't want that to happen.
 	if sharedMount {
-		return mount("", parentMount, "", "", unix.MS_PRIVATE, "")
+		return mount("", parentMount, "", unix.MS_PRIVATE, "")
 	}
 
 	return nil
@@ -797,7 +797,7 @@ func prepareRoot(config *configs.Config) error {
 	if config.RootPropagation != 0 {
 		flag = config.RootPropagation
 	}
-	if err := mount("", "/", "", "", uintptr(flag), ""); err != nil {
+	if err := mount("", "/", "", uintptr(flag), ""); err != nil {
 		return err
 	}
 
@@ -808,13 +808,13 @@ func prepareRoot(config *configs.Config) error {
 		return err
 	}
 
-	return mount(config.Rootfs, config.Rootfs, "", "bind", unix.MS_BIND|unix.MS_REC, "")
+	return mount(config.Rootfs, config.Rootfs, "bind", unix.MS_BIND|unix.MS_REC, "")
 }
 
 func setReadonly() error {
 	flags := uintptr(unix.MS_BIND | unix.MS_REMOUNT | unix.MS_RDONLY)
 
-	err := mount("", "/", "", "", flags, "")
+	err := mount("", "/", "", flags, "")
 	if err == nil {
 		return nil
 	}
@@ -823,7 +823,7 @@ func setReadonly() error {
 		return &os.PathError{Op: "statfs", Path: "/", Err: err}
 	}
 	flags |= uintptr(s.Flags)
-	return mount("", "/", "", "", flags, "")
+	return mount("", "/", "", flags, "")
 }
 
 func setupPtmx(config *configs.Config) error {
@@ -881,7 +881,7 @@ func pivotRoot(rootfs string) error {
 	// known to cause issues due to races where we still have a reference to a
 	// mount while a process in the host namespace are trying to operate on
 	// something they think has no mounts (devicemapper in particular).
-	if err := mount("", ".", "", "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil {
+	if err := mount("", ".", "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil {
 		return err
 	}
 	// Perform the unmount. MNT_DETACH allows us to unmount /proc/self/cwd.
@@ -930,7 +930,7 @@ func msMoveRoot(rootfs string) error {
 	for _, info := range mountinfos {
 		p := info.Mountpoint
 		// Be sure umount events are not propagated to the host.
-		if err := mount("", p, "", "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil {
+		if err := mount("", p, "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil {
 			if errors.Is(err, unix.ENOENT) {
 				// If the mountpoint doesn't exist that means that we've
 				// already blasted away some parent directory of the mountpoint
@@ -945,15 +945,15 @@ func msMoveRoot(rootfs string) error {
 			} else {
 				// If we have not privileges for umounting (e.g. rootless), then
 				// cover the path.
-				if err := mount("tmpfs", p, "", "tmpfs", 0, ""); err != nil {
+				if err := mount("tmpfs", p, "tmpfs", 0, ""); err != nil {
 					return err
 				}
 			}
 		}
 	}
 
 	// Move the rootfs on top of "/" in our mount namespace.
-	if err := mount(rootfs, "/", "", "", unix.MS_MOVE, ""); err != nil {
+	if err := mount(rootfs, "/", "", unix.MS_MOVE, ""); err != nil {
 		return err
 	}
 	return chroot()
@@ -991,7 +991,7 @@ func createIfNotExists(path string, isDir bool) error {
 
 // readonlyPath will make a path read only.
 func readonlyPath(path string) error {
-	if err := mount(path, path, "", "", unix.MS_BIND|unix.MS_REC, ""); err != nil {
+	if err := mount(path, path, "", unix.MS_BIND|unix.MS_REC, ""); err != nil {
 		if errors.Is(err, os.ErrNotExist) {
 			return nil
 		}
@@ -1004,7 +1004,7 @@ func readonlyPath(path string) error {
 	}
 	flags := uintptr(s.Flags) & (unix.MS_NOSUID | unix.MS_NODEV | unix.MS_NOEXEC)
 
-	if err := mount(path, path, "", "", flags|unix.MS_BIND|unix.MS_REMOUNT|unix.MS_RDONLY, ""); err != nil {
+	if err := mount(path, path, "", flags|unix.MS_BIND|unix.MS_REMOUNT|unix.MS_RDONLY, ""); err != nil {
 		return err
 	}
 
@@ -1025,7 +1025,7 @@ func remountReadonly(m *configs.Mount) error {
 		// nosuid, etc.). So, let's use that case so that we can do
 		// this re-mount without failing in a userns.
 		flags |= unix.MS_REMOUNT | unix.MS_BIND | unix.MS_RDONLY
-		if err := mount("", dest, "", "", uintptr(flags), ""); err != nil {
+		if err := mount("", dest, "", uintptr(flags), ""); err != nil {
 			if errors.Is(err, unix.EBUSY) {
 				time.Sleep(100 * time.Millisecond)
 				continue
@@ -1043,9 +1043,9 @@ func remountReadonly(m *configs.Mount) error {
 // For files, maskPath bind mounts /dev/null over the top of the specified path.
 // For directories, maskPath mounts read-only tmpfs over the top of the specified path.
 func maskPath(path string, mountLabel string) error {
-	if err := mount("/dev/null", path, "", "", unix.MS_BIND, ""); err != nil && !errors.Is(err, os.ErrNotExist) {
+	if err := mount("/dev/null", path, "", unix.MS_BIND, ""); err != nil && !errors.Is(err, os.ErrNotExist) {
 		if errors.Is(err, unix.ENOTDIR) {
-			return mount("tmpfs", path, "", "tmpfs", unix.MS_RDONLY, label.FormatMountLabel("", mountLabel))
+			return mount("tmpfs", path, "tmpfs", unix.MS_RDONLY, label.FormatMountLabel("", mountLabel))
 		}
 		return err
 	}
@@ -1060,28 +1060,28 @@ func writeSystemProperty(key, value string) error {
 }
 
 func remount(m *configs.Mount, rootfs string, mountFd *int) error {
-	source := m.Source
+	srcFD := ""
 	if mountFd != nil {
-		source = "/proc/self/fd/" + strconv.Itoa(*mountFd)
+		srcFD = "/proc/self/fd/" + strconv.Itoa(*mountFd)
 	}
 
-	return utils.WithProcfd(rootfs, m.Destination, func(procfd string) error {
+	return utils.WithProcfd(rootfs, m.Destination, func(dstFD string) error {
 		flags := uintptr(m.Flags | unix.MS_REMOUNT)
-		err := mount(source, m.Destination, procfd, m.Device, flags, "")
+		err := mountViaFDs(m.Source, srcFD, m.Destination, dstFD, m.Device, flags, "")
 		if err == nil {
 			return nil
 		}
 		// Check if the source has ro flag...
 		var s unix.Statfs_t
-		if err := unix.Statfs(source, &s); err != nil {
-			return &os.PathError{Op: "statfs", Path: source, Err: err}
+		if err := unix.Statfs(m.Source, &s); err != nil {
+			return &os.PathError{Op: "statfs", Path: m.Source, Err: err}
 		}
 		if s.Flags&unix.MS_RDONLY != unix.MS_RDONLY {
 			return err
 		}
 		// ... and retry the mount with ro flag set.
 		flags |= unix.MS_RDONLY
-		return mount(source, m.Destination, procfd, m.Device, flags, "")
+		return mountViaFDs(m.Source, srcFD, m.Destination, dstFD, m.Device, flags, "")
 	})
 }
 
@@ -1100,26 +1100,26 @@ func mountPropagate(m *configs.Mount, rootfs string, mountLabel string, mountFd
 		flags &= ^unix.MS_RDONLY
 	}
 
+	srcFD := ""
+	if mountFd != nil {
+		srcFD = "/proc/self/fd/" + strconv.Itoa(*mountFd)
+	}
+
 	// Because the destination is inside a container path which might be
 	// mutating underneath us, we verify that we are actually going to mount
 	// inside the container with WithProcfd() -- mounting through a procfd
 	// mounts on the target.
-	source := m.Source
-	if mountFd != nil {
-		source = "/proc/self/fd/" + strconv.Itoa(*mountFd)
-	}
-
-	if err := utils.WithProcfd(rootfs, m.Destination, func(procfd string) error {
-		return mount(source, m.Destination, procfd, m.Device, uintptr(flags), data)
+	if err := utils.WithProcfd(rootfs, m.Destination, func(dstFD string) error {
+		return mountViaFDs(m.Source, srcFD, m.Destination, dstFD, m.Device, uintptr(flags), data)
 	}); err != nil {
 		return err
 	}
 	// We have to apply mount propagation flags in a separate WithProcfd() call
 	// because the previous call invalidates the passed procfd -- the mount
 	// target needs to be re-opened.
-	if err := utils.WithProcfd(rootfs, m.Destination, func(procfd string) error {
+	if err := utils.WithProcfd(rootfs, m.Destination, func(dstFD string) error {
 		for _, pflag := range m.PropagationFlags {
-			if err := mount("", m.Destination, procfd, "", uintptr(pflag), ""); err != nil {
+			if err := mountViaFDs("", "", m.Destination, dstFD, "", uintptr(pflag), ""); err != nil {
 				return err
 			}
 		}