libcontainer: remove all mount logic from nsexec

go.mod

@@ -10,7 +10,7 @@ require (
  github.com/cyphar/filepath-securejoin v0.2.4
  github.com/docker/go-units v0.5.0
  github.com/godbus/dbus/v5 v5.1.0
- github.com/moby/sys/mountinfo v0.6.2
+ github.com/moby/sys/mountinfo v0.7.1
  github.com/moby/sys/user v0.1.0
  github.com/mrunalp/fileutils v0.5.1
  github.com/opencontainers/runtime-spec v1.1.1-0.20230823135140-4fec88fd00a4

go.sum

@@ -26,8 +26,8 @@ github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
-github.com/moby/sys/mountinfo v0.6.2 h1:BzJjoreD5BMFNmD9Rus6gdd1pLuecOFPt8wC+Vygl78=
-github.com/moby/sys/mountinfo v0.6.2/go.mod h1:IJb6JQeOklcdMU9F5xQ8ZALD+CUr5VlGpwtX+VE0rpI=
+github.com/moby/sys/mountinfo v0.7.1 h1:/tTvQaSJRr2FshkhXiIpux6fQ2Zvc4j7tAhMTStAG2g=
+github.com/moby/sys/mountinfo v0.7.1/go.mod h1:IJb6JQeOklcdMU9F5xQ8ZALD+CUr5VlGpwtX+VE0rpI=
 github.com/moby/sys/user v0.1.0 h1:WmZ93f5Ux6het5iituh9x2zAG7NFY9Aqi49jjE1PaQg=
 github.com/moby/sys/user v0.1.0/go.mod h1:fKJhFOnsCN6xZ5gSfbM6zaHGgDJMrqt9/reuj4T7MmU=
 github.com/mrunalp/fileutils v0.5.1 h1:F+S7ZlNKnrwHfSwdlgNSkKo67ReVf8o9fel6C3dkm/Q=

libcontainer/apparmor/apparmor_linux.go

@@ -26,14 +26,19 @@ func isEnabled() bool {
 }
 
 func setProcAttr(attr, value string) error {
- // Under AppArmor you can only change your own attr, so use /proc/self/
- // instead of /proc/<tid>/ like libapparmor does
- attrPath := "/proc/self/attr/apparmor/" + attr
- if _, err := os.Stat(attrPath); errors.Is(err, os.ErrNotExist) {
+ attr = utils.CleanPath(attr)
+ attrSubPath := "attr/apparmor/" + attr
+ if _, err := os.Stat("/proc/self/" + attrSubPath); errors.Is(err, os.ErrNotExist) {
  // fall back to the old convention
- attrPath = "/proc/self/attr/" + attr
+ attrSubPath = "attr/" + attr
  }
 
+ // Under AppArmor you can only change your own attr, so there's no reason
+ // to not use /proc/thread-self/ (instead of /proc/<tid>/, like libapparmor
+ // does).
+ attrPath, closer := utils.ProcThreadSelf(attrSubPath)
+ defer closer()
+
  f, err := os.OpenFile(attrPath, os.O_WRONLY, 0)
  if err != nil {
  return err

libcontainer/cgroups/cgroups_test.go

@@ -5,6 +5,9 @@ import (
 )
 
 func TestParseCgroups(t *testing.T) {
+ // We don't need to use /proc/thread-self here because runc always runs
+ // with every thread in the same cgroup. This lets us avoid having to do
+ // runtime.LockOSThread.
  cgroups, err := ParseCgroupFile("/proc/self/cgroup")
  if err != nil {
  t.Fatal(err)

libcontainer/cgroups/file.go

@@ -136,13 +136,14 @@ func openFile(dir, file string, flags int) (*os.File, error) {
  //
  // TODO: if such usage will ever be common, amend this
  // to reopen cgroupFd and retry openat2.
- fdStr := strconv.Itoa(cgroupFd)
- fdDest, _ := os.Readlink("/proc/self/fd/" + fdStr)
+ fdPath, closer := utils.ProcThreadSelf("fd/" + strconv.Itoa(cgroupFd))
+ defer closer()
+ fdDest, _ := os.Readlink(fdPath)
  if fdDest != cgroupfsDir {
  // Wrap the error so it is clear that cgroupFd
  // is opened to an unexpected/wrong directory.
- err = fmt.Errorf("cgroupFd %s unexpectedly opened to %s != %s: %w",
- fdStr, fdDest, cgroupfsDir, err)
+ err = fmt.Errorf("cgroupFd %d unexpectedly opened to %s != %s: %w",
+ cgroupFd, fdDest, cgroupfsDir, err)
  }
  return nil, err
  }

libcontainer/cgroups/fs2/defaultpath.go

@@ -55,6 +55,9 @@ func _defaultDirPath(root, cgPath, cgParent, cgName string) (string, error) {
  return filepath.Join(root, innerPath), nil
  }
 
+ // we don't need to use /proc/thread-self here because runc always runs
+ // with every thread in the same cgroup. This lets us avoid having to do
+ // runtime.LockOSThread.
  ownCgroup, err := parseCgroupFile("/proc/self/cgroup")
  if err != nil {
  return "", err

libcontainer/cgroups/v1_utils.go

@@ -99,11 +99,12 @@ func tryDefaultPath(cgroupPath, subsystem string) string {
 // expensive), so it is assumed that cgroup mounts are not being changed.
 func readCgroupMountinfo() ([]*mountinfo.Info, error) {
  readMountinfoOnce.Do(func() {
+ // mountinfo.GetMounts uses /proc/thread-self, so we can use it without
+ // issues.
  cgroupMountinfo, readMountinfoErr = mountinfo.GetMounts(
  mountinfo.FSTypeFilter("cgroup"),
  )
  })
-
  return cgroupMountinfo, readMountinfoErr
 }
 
@@ -196,6 +197,9 @@ func getCgroupMountsV1(all bool) ([]Mount, error) {
  return nil, err
  }
 
+ // We don't need to use /proc/thread-self here because runc always runs
+ // with every thread in the same cgroup. This lets us avoid having to do
+ // runtime.LockOSThread.
  allSubsystems, err := ParseCgroupFile("/proc/self/cgroup")
  if err != nil {
  return nil, err
@@ -214,6 +218,10 @@ func GetOwnCgroup(subsystem string) (string, error) {
  if IsCgroup2UnifiedMode() {
  return "", errUnified
  }
+
+ // We don't need to use /proc/thread-self here because runc always runs
+ // with every thread in the same cgroup. This lets us avoid having to do
+ // runtime.LockOSThread.
  cgroups, err := ParseCgroupFile("/proc/self/cgroup")
  if err != nil {
  return "", err

libcontainer/configs/mount.go

@@ -3,5 +3,5 @@ package configs
 const (
  // EXT_COPYUP is a directive to copy up the contents of a directory when
  // a tmpfs is mounted over it.
- EXT_COPYUP = 1 << iota //nolint:golint // ignore "don't use ALL_CAPS" warning
+ EXT_COPYUP = 1 << iota //nolint:golint,revive // ignore "don't use ALL_CAPS" warning
 )

libcontainer/configs/mount_linux.go

@@ -2,6 +2,24 @@ package configs
 
 import "golang.org/x/sys/unix"
 
+type MountIDMapping struct {
+ // Recursive indicates if the mapping needs to be recursive.
+ Recursive bool `json:"recursive"`
+
+ // UserNSPath is a path to a user namespace that indicates the necessary
+ // id-mappings for MOUNT_ATTR_IDMAP. If set to non-"", UIDMappings and
+ // GIDMappings must be set to nil.
+ UserNSPath string `json:"userns_path,omitempty"`
+
+ // UIDMappings is the uid mapping set for this mount, to be used with
+ // MOUNT_ATTR_IDMAP.
+ UIDMappings []IDMap `json:"uid_mappings,omitempty"`
+
+ // GIDMappings is the gid mapping set for this mount, to be used with
+ // MOUNT_ATTR_IDMAP.
+ GIDMappings []IDMap `json:"gid_mappings,omitempty"`
+}
+
 type Mount struct {
  // Source path for the mount.
  Source string `json:"source"`
@@ -34,23 +52,15 @@ type Mount struct {
  // Extensions are additional flags that are specific to runc.
  Extensions int `json:"extensions"`
 
- // UIDMappings is used to changing file user owners w/o calling chown.
- // Note that, the underlying filesystem should support this feature to be
- // used.
- // Every mount point could have its own mapping.
- UIDMappings []IDMap `json:"uid_mappings,omitempty"`
-
- // GIDMappings is used to changing file group owners w/o calling chown.
- // Note that, the underlying filesystem should support this feature to be
- // used.
- // Every mount point could have its own mapping.
- GIDMappings []IDMap `json:"gid_mappings,omitempty"`
+ // Mapping is the MOUNT_ATTR_IDMAP configuration for the mount. If non-nil,
+ // the mount is configured to use MOUNT_ATTR_IDMAP-style id mappings.
+ IDMapping *MountIDMapping `json:"id_mapping,omitempty"`
 }
 
 func (m *Mount) IsBind() bool {
  return m.Flags&unix.MS_BIND != 0
 }
 
 func (m *Mount) IsIDMapped() bool {
- return len(m.UIDMappings) > 0 || len(m.GIDMappings) > 0
+ return m.IDMapping != nil
 }

libcontainer/configs/namespaces_linux.go

@@ -59,6 +59,9 @@ func IsNamespaceSupported(ns NamespaceType) bool {
  if nsFile == "" {
  return false
  }
+ // We don't need to use /proc/thread-self here because the list of
+ // namespace types is unrelated to the thread. This lets us avoid having to
+ // do runtime.LockOSThread.
  _, err := os.Stat("/proc/self/ns/" + nsFile)
  // a namespace is supported if it exists and we have permissions to read it
  supported = err == nil

libcontainer/configs/validate/validator.go

@@ -309,29 +309,32 @@ func checkBindOptions(m *configs.Mount) error {
 }
 
 func checkIDMapMounts(config *configs.Config, m *configs.Mount) error {
+ // Make sure MOUNT_ATTR_IDMAP is not set on any of our mounts. This
+ // attribute is handled differently to all other attributes (through
+ // m.IDMapping), so make sure we never store it in the actual config. This
+ // really shouldn't ever happen.
+ if m.RecAttr != nil && (m.RecAttr.Attr_set|m.RecAttr.Attr_clr)&unix.MOUNT_ATTR_IDMAP != 0 {
+ return errors.New("mount configuration cannot contain recAttr for MOUNT_ATTR_IDMAP")
+ }
  if !m.IsIDMapped() {
  return nil
  }
-
  if !m.IsBind() {
- return fmt.Errorf("gidMappings/uidMappings is supported only for mounts with the option 'bind'")
+ return errors.New("id-mapped mounts are only supported for bind-mounts")
  }
  if config.RootlessEUID {
- return fmt.Errorf("gidMappings/uidMappings is not supported when runc is being launched with EUID != 0, needs CAP_SYS_ADMIN on the runc parent's user namespace")
- }
- if len(config.UIDMappings) == 0 || len(config.GIDMappings) == 0 {
- return fmt.Errorf("not yet supported to use gidMappings/uidMappings in a mount without also using a user namespace")
- }
- if !sameMapping(config.UIDMappings, m.UIDMappings) {
- return fmt.Errorf("not yet supported for the mount uidMappings to be different than user namespace uidMapping")
- }
- if !sameMapping(config.GIDMappings, m.GIDMappings) {
- return fmt.Errorf("not yet supported for the mount gidMappings to be different than user namespace gidMapping")
+ return errors.New("id-mapped mounts are not supported for rootless containers")
  }
- if !filepath.IsAbs(m.Source) {
- return fmt.Errorf("mount source not absolute")
+ if m.IDMapping.UserNSPath == "" {
+ if len(m.IDMapping.UIDMappings) == 0 || len(m.IDMapping.GIDMappings) == 0 {
+ return errors.New("id-mapped mounts must have both uid and gid mappings specified")
+ }
+ } else {
+ if m.IDMapping.UIDMappings != nil || m.IDMapping.GIDMappings != nil {
+ // should never happen
+ return errors.New("[internal error] id-mapped mounts cannot have both userns_path and uid and gid mappings specified")
+ }
  }
-
  return nil
 }
 
@@ -356,27 +359,6 @@ func mountsStrict(config *configs.Config) error {
  return nil
 }
 
-// sameMapping checks if the mappings are the same. If the mappings are the same
-// but in different order, it returns false.
-func sameMapping(a, b []configs.IDMap) bool {
- if len(a) != len(b) {
- return false
- }
-
- for i := range a {
- if a[i].ContainerID != b[i].ContainerID {
- return false
- }
- if a[i].HostID != b[i].HostID {
- return false
- }
- if a[i].Size != b[i].Size {
- return false
- }
- }
- return true
-}
-
 func isHostNetNS(path string) (bool, error) {
  const currentProcessNetns = "/proc/self/ns/net"