diff --git a/libcontainer/cgroups/fs/blkio.go b/libcontainer/cgroups/fs/blkio.go index 52c118d68c1..031a6bbdab2 100644 --- a/libcontainer/cgroups/fs/blkio.go +++ b/libcontainer/cgroups/fs/blkio.go @@ -22,12 +22,8 @@ func (s *BlkioGroup) Name() string { return "blkio" } -func (s *BlkioGroup) Apply(d *cgroupData) error { - _, err := d.join("blkio") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *BlkioGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *BlkioGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -74,10 +70,6 @@ func (s *BlkioGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *BlkioGroup) Remove(d *cgroupData) error { - return removePath(d.path("blkio")) -} - /* examples: diff --git a/libcontainer/cgroups/fs/cpu.go b/libcontainer/cgroups/fs/cpu.go index 7386aace14f..6fb1c2078a5 100644 --- a/libcontainer/cgroups/fs/cpu.go +++ b/libcontainer/cgroups/fs/cpu.go @@ -21,17 +21,7 @@ func (s *CpuGroup) Name() string { return "cpu" } -func (s *CpuGroup) Apply(d *cgroupData) error { - // We always want to join the cpu group, to allow fair cpu scheduling - // on a container basis - path, err := d.path("cpu") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return s.ApplyDir(path, d.config, d.pid) -} - -func (s *CpuGroup) ApplyDir(path string, cgroup *configs.Cgroup, pid int) error { +func (s *CpuGroup) Apply(path string, d *cgroupData) error { // This might happen if we have no cpu cgroup mounted. // Just do nothing and don't fail. if path == "" { @@ -43,12 +33,12 @@ func (s *CpuGroup) ApplyDir(path string, cgroup *configs.Cgroup, pid int) error // We should set the real-Time group scheduling settings before moving // in the process because if the process is already in SCHED_RR mode // and no RT bandwidth is set, adding it will fail. - if err := s.SetRtSched(path, cgroup); err != nil { + if err := s.SetRtSched(path, d.config); err != nil { return err } - // because we are not using d.join we need to place the pid into the procs file - // unlike the other subsystems - return cgroups.WriteCgroupProc(path, pid) + // Since we are not using join(), we need to place the pid + // into the procs file unlike other subsystems. + return cgroups.WriteCgroupProc(path, d.pid) } func (s *CpuGroup) SetRtSched(path string, cgroup *configs.Cgroup) error { @@ -96,10 +86,6 @@ func (s *CpuGroup) Set(path string, cgroup *configs.Cgroup) error { return s.SetRtSched(path, cgroup) } -func (s *CpuGroup) Remove(d *cgroupData) error { - return removePath(d.path("cpu")) -} - func (s *CpuGroup) GetStats(path string, stats *cgroups.Stats) error { f, err := os.Open(filepath.Join(path, "cpu.stat")) if err != nil { diff --git a/libcontainer/cgroups/fs/cpu_test.go b/libcontainer/cgroups/fs/cpu_test.go index 2eeb489efba..4a8ecf9950e 100644 --- a/libcontainer/cgroups/fs/cpu_test.go +++ b/libcontainer/cgroups/fs/cpu_test.go @@ -182,7 +182,9 @@ func TestCpuSetRtSchedAtApply(t *testing.T) { helper.CgroupData.config.Resources.CpuRtRuntime = rtRuntimeAfter helper.CgroupData.config.Resources.CpuRtPeriod = rtPeriodAfter cpu := &CpuGroup{} - if err := cpu.ApplyDir(helper.CgroupPath, helper.CgroupData.config, 1234); err != nil { + + helper.CgroupData.pid = 1234 + if err := cpu.Apply(helper.CgroupPath, helper.CgroupData); err != nil { t.Fatal(err) } diff --git a/libcontainer/cgroups/fs/cpuacct.go b/libcontainer/cgroups/fs/cpuacct.go index dd8aea0c2b9..d8279eae36d 100644 --- a/libcontainer/cgroups/fs/cpuacct.go +++ b/libcontainer/cgroups/fs/cpuacct.go @@ -40,23 +40,14 @@ func (s *CpuacctGroup) Name() string { return "cpuacct" } -func (s *CpuacctGroup) Apply(d *cgroupData) error { - // we just want to join this group even though we don't set anything - if _, err := d.join("cpuacct"); err != nil && !cgroups.IsNotFound(err) { - return err - } - - return nil +func (s *CpuacctGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *CpuacctGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *CpuacctGroup) Remove(d *cgroupData) error { - return removePath(d.path("cpuacct")) -} - func (s *CpuacctGroup) GetStats(path string, stats *cgroups.Stats) error { userModeUsage, kernelModeUsage, err := getCpuUsageBreakdown(path) if err != nil { diff --git a/libcontainer/cgroups/fs/cpuset.go b/libcontainer/cgroups/fs/cpuset.go index 6a7725e3591..ba17519baea 100644 --- a/libcontainer/cgroups/fs/cpuset.go +++ b/libcontainer/cgroups/fs/cpuset.go @@ -23,12 +23,8 @@ func (s *CpusetGroup) Name() string { return "cpuset" } -func (s *CpusetGroup) Apply(d *cgroupData) error { - dir, err := d.path("cpuset") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return s.ApplyDir(dir, d.config, d.pid) +func (s *CpusetGroup) Apply(path string, d *cgroupData) error { + return s.ApplyDir(path, d.config, d.pid) } func (s *CpusetGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -45,10 +41,6 @@ func (s *CpusetGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *CpusetGroup) Remove(d *cgroupData) error { - return removePath(d.path("cpuset")) -} - func (s *CpusetGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/devices.go b/libcontainer/cgroups/fs/devices.go index a70619cbb34..3ec81621e3f 100644 --- a/libcontainer/cgroups/fs/devices.go +++ b/libcontainer/cgroups/fs/devices.go @@ -22,14 +22,13 @@ func (s *DevicesGroup) Name() string { return "devices" } -func (s *DevicesGroup) Apply(d *cgroupData) error { - _, err := d.join("devices") - if err != nil { - // We will return error even it's `not found` error, devices - // cgroup is hard requirement for container's security. - return err +func (s *DevicesGroup) Apply(path string, d *cgroupData) error { + if path == "" { + // Return error here, since devices cgroup + // is a hard requirement for container's security. + return errSubsystemDoesNotExist } - return nil + return join(path, d.pid) } func loadEmulator(path string) (*devices.Emulator, error) { @@ -103,10 +102,6 @@ func (s *DevicesGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *DevicesGroup) Remove(d *cgroupData) error { - return removePath(d.path("devices")) -} - func (s *DevicesGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/freezer.go b/libcontainer/cgroups/fs/freezer.go index b5b77da9421..11cb1646f48 100644 --- a/libcontainer/cgroups/fs/freezer.go +++ b/libcontainer/cgroups/fs/freezer.go @@ -22,12 +22,8 @@ func (s *FreezerGroup) Name() string { return "freezer" } -func (s *FreezerGroup) Apply(d *cgroupData) error { - _, err := d.join("freezer") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *FreezerGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *FreezerGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -61,10 +57,6 @@ func (s *FreezerGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *FreezerGroup) Remove(d *cgroupData) error { - return removePath(d.path("freezer")) -} - func (s *FreezerGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index 245ce84f4c4..c0103a3adaf 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -3,11 +3,9 @@ package fs import ( - "bufio" "fmt" "os" "path/filepath" - "strings" "sync" "github.com/opencontainers/runc/libcontainer/cgroups" @@ -18,7 +16,7 @@ import ( ) var ( - subsystemsLegacy = subsystemSet{ + subsystems = []subsystem{ &CpusetGroup{}, &DevicesGroup{}, &MemoryGroup{}, @@ -38,26 +36,13 @@ var ( var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist") -type subsystemSet []subsystem - -func (s subsystemSet) Get(name string) (subsystem, error) { - for _, ss := range s { - if ss.Name() == name { - return ss, nil - } - } - return nil, errSubsystemDoesNotExist -} - type subsystem interface { // Name returns the name of the subsystem. Name() string // Returns the stats, as 'stats', corresponding to the cgroup under 'path'. GetStats(path string, stats *cgroups.Stats) error - // Removes the cgroup represented by 'cgroupData'. - Remove(*cgroupData) error // Creates and joins the cgroup represented by 'cgroupData'. - Apply(*cgroupData) error + Apply(path string, c *cgroupData) error // Set the cgroup represented by cgroup. Set(path string, cgroup *configs.Cgroup) error } @@ -78,66 +63,9 @@ func NewManager(cg *configs.Cgroup, paths map[string]string, rootless bool) cgro } // The absolute path to the root of the cgroup hierarchies. -var cgroupRootLock sync.Mutex -var cgroupRoot string - -// Gets the cgroupRoot. -func getCgroupRoot() (string, error) { - cgroupRootLock.Lock() - defer cgroupRootLock.Unlock() - - if cgroupRoot != "" { - return cgroupRoot, nil - } - - f, err := os.Open("/proc/self/mountinfo") - if err != nil { - return "", err - } - defer f.Close() - - var root string - scanner := bufio.NewScanner(f) - for scanner.Scan() { - text := scanner.Text() - fields := strings.Split(text, " ") - // Safe as mountinfo encodes mountpoints with spaces as \040. - index := strings.Index(text, " - ") - postSeparatorFields := strings.Fields(text[index+3:]) - numPostFields := len(postSeparatorFields) - - // This is an error as we can't detect if the mount is for "cgroup" - if numPostFields == 0 { - return "", fmt.Errorf("mountinfo: found no fields post '-' in %q", text) - } - - if postSeparatorFields[0] == "cgroup" { - // Check that the mount is properly formatted. - if numPostFields < 3 { - return "", fmt.Errorf("Error found less than 3 fields post '-' in %q", text) - } - - root = filepath.Dir(fields[4]) - break - } - } - if err := scanner.Err(); err != nil { - return "", err - } - if root == "" { - return "", errors.New("no cgroup mount found in mountinfo") - } - - if _, err := os.Stat(root); err != nil { - return "", err - } - - cgroupRoot = root - return cgroupRoot, nil -} +const cgroupRoot = "/sys/fs/cgroup" type cgroupData struct { - root string innerPath string config *configs.Cgroup pid int @@ -166,10 +94,6 @@ func isIgnorableError(rootless bool, err error) bool { return false } -func (m *manager) getSubsystems() subsystemSet { - return subsystemsLegacy -} - func (m *manager) Apply(pid int) (err error) { if m.cgroups == nil { return nil @@ -179,27 +103,26 @@ func (m *manager) Apply(pid int) (err error) { var c = m.cgroups - d, err := getCgroupData(m.cgroups, pid) - if err != nil { - return err - } - m.paths = make(map[string]string) if c.Paths != nil { for name, path := range c.Paths { - _, err := d.path(name) + cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup") if err != nil { - if cgroups.IsNotFound(err) { - continue - } return err } - m.paths[name] = path + if _, ok := cgMap[name]; ok { + m.paths[name] = path + } } return cgroups.EnterPid(m.paths, pid) } - for _, sys := range m.getSubsystems() { + d, err := getCgroupData(m.cgroups, pid) + if err != nil { + return err + } + + for _, sys := range subsystems { p, err := d.path(sys.Name()) if err != nil { // The non-presence of the devices subsystem is @@ -211,7 +134,7 @@ func (m *manager) Apply(pid int) (err error) { } m.paths[sys.Name()] = p - if err := sys.Apply(d); err != nil { + if err := sys.Apply(p, d); err != nil { // In the case of rootless (including euid=0 in userns), where an // explicit cgroup path hasn't been set, we don't bail on error in // case of permission problems. Cases where limits have been set @@ -250,9 +173,9 @@ func (m *manager) GetStats() (*cgroups.Stats, error) { m.mu.Lock() defer m.mu.Unlock() stats := cgroups.NewStats() - for name, path := range m.paths { - sys, err := m.getSubsystems().Get(name) - if err == errSubsystemDoesNotExist || !cgroups.PathExists(path) { + for _, sys := range subsystems { + path := m.paths[sys.Name()] + if path == "" { continue } if err := sys.GetStats(path, stats); err != nil { @@ -275,7 +198,7 @@ func (m *manager) Set(container *configs.Config) error { m.mu.Lock() defer m.mu.Unlock() - for _, sys := range m.getSubsystems() { + for _, sys := range subsystems { path := m.paths[sys.Name()] if err := sys.Set(path, container.Cgroups); err != nil { if m.rootless && sys.Name() == "devices" { @@ -298,7 +221,7 @@ func (m *manager) Set(container *configs.Config) error { // Freeze toggles the container's freezer cgroup depending on the state // provided -func (m *manager) Freeze(state configs.FreezerState) (Err error) { +func (m *manager) Freeze(state configs.FreezerState) error { path := m.Path("freezer") if m.cgroups == nil || path == "" { return errors.New("cannot toggle freezer: cgroups not configured for container") @@ -306,17 +229,9 @@ func (m *manager) Freeze(state configs.FreezerState) (Err error) { prevState := m.cgroups.Resources.Freezer m.cgroups.Resources.Freezer = state - defer func() { - if Err != nil { - m.cgroups.Resources.Freezer = prevState - } - }() - - freezer, err := m.getSubsystems().Get("freezer") - if err != nil { - return err - } + freezer := &FreezerGroup{} if err := freezer.Set(path, m.cgroups); err != nil { + m.cgroups.Resources.Freezer = prevState return err } return nil @@ -331,11 +246,6 @@ func (m *manager) GetAllPids() ([]int, error) { } func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { - root, err := getCgroupRoot() - if err != nil { - return nil, err - } - if (c.Name != "" || c.Parent != "") && c.Path != "" { return nil, errors.New("cgroup: either Path or Name and Parent should be used") } @@ -351,7 +261,6 @@ func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { } return &cgroupData{ - root: root, innerPath: innerPath, config: c, pid: pid, @@ -359,16 +268,16 @@ func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { } func (raw *cgroupData) path(subsystem string) (string, error) { - mnt, err := cgroups.FindCgroupMountpoint(raw.root, subsystem) - // If we didn't mount the subsystem, there is no point we make the path. - if err != nil { - return "", err - } - // If the cgroup name/path is absolute do not look relative to the cgroup of the init process. if filepath.IsAbs(raw.innerPath) { + mnt, err := cgroups.FindCgroupMountpoint(cgroupRoot, subsystem) + // If we didn't mount the subsystem, there is no point we make the path. + if err != nil { + return "", err + } + // Sometimes subsystems can be mounted together as 'cpu,cpuacct'. - return filepath.Join(raw.root, filepath.Base(mnt), raw.innerPath), nil + return filepath.Join(cgroupRoot, filepath.Base(mnt), raw.innerPath), nil } // Use GetOwnCgroupPath instead of GetInitCgroupPath, because the creating @@ -382,18 +291,14 @@ func (raw *cgroupData) path(subsystem string) (string, error) { return filepath.Join(parentPath, raw.innerPath), nil } -func (raw *cgroupData) join(subsystem string) (string, error) { - path, err := raw.path(subsystem) - if err != nil { - return "", err +func join(path string, pid int) error { + if path == "" { + return nil } if err := os.MkdirAll(path, 0755); err != nil { - return "", err - } - if err := cgroups.WriteCgroupProc(path, raw.pid); err != nil { - return "", err + return err } - return path, nil + return cgroups.WriteCgroupProc(path, pid) } func removePath(p string, err error) error { @@ -418,13 +323,12 @@ func (m *manager) GetCgroups() (*configs.Cgroup, error) { func (m *manager) GetFreezerState() (configs.FreezerState, error) { dir := m.Path("freezer") - freezer, err := m.getSubsystems().Get("freezer") - // If the container doesn't have the freezer cgroup, say it's undefined. - if err != nil || dir == "" { + if dir == "" { return configs.Undefined, nil } - return freezer.(*FreezerGroup).GetState(dir) + freezer := &FreezerGroup{} + return freezer.GetState(dir) } func (m *manager) Exists() bool { diff --git a/libcontainer/cgroups/fs/fs_test.go b/libcontainer/cgroups/fs/fs_test.go index 4971d547554..bc036d0a584 100644 --- a/libcontainer/cgroups/fs/fs_test.go +++ b/libcontainer/cgroups/fs/fs_test.go @@ -15,283 +15,74 @@ func TestInvalidCgroupPath(t *testing.T) { if cgroups.IsCgroup2UnifiedMode() { t.Skip("cgroup v2 is not supported") } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Path: "../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -func TestInvalidAbsoluteCgroupPath(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Path: "/../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidCgroupParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "../../../../../../../../../../some/path", - Name: "name", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidAbsoluteCgroupParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "/../../../../../../../../../../some/path", - Name: "name", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidCgroupName(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "parent", - Name: "../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } - -} -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidAbsoluteCgroupName(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "parent", - Name: "/../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidCgroupNameAndParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "../../../../../../../../../../some/path", - Name: "../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidAbsoluteCgroupNameAndParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "/../../../../../../../../../../some/path", - Name: "/../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + testCases := []struct { + test string + path, name, parent string + }{ + { + test: "invalid cgroup path", + path: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup path", + path: "/../../../../../../../../../../some/path", + }, + { + test: "invalid cgroup parent", + parent: "../../../../../../../../../../some/path", + name: "name", + }, + { + test: "invalid absolute cgroup parent", + parent: "/../../../../../../../../../../some/path", + name: "name", + }, + { + test: "invalid cgroup name", + parent: "parent", + name: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup name", + parent: "parent", + name: "/../../../../../../../../../../some/path", + }, + { + test: "invalid cgroup name and parent", + parent: "../../../../../../../../../../some/path", + name: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup name and parent", + parent: "/../../../../../../../../../../some/path", + name: "/../../../../../../../../../../some/path", + }, + } + + for _, tc := range testCases { + t.Run(tc.test, func(t *testing.T) { + config := &configs.Cgroup{Path: tc.path, Name: tc.name, Parent: tc.parent} + + data, err := getCgroupData(config, 0) + if err != nil { + t.Fatalf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(cgroupRoot, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Fatalf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } + }) } } diff --git a/libcontainer/cgroups/fs/hugetlb.go b/libcontainer/cgroups/fs/hugetlb.go index 68719c2e6de..f3946611e04 100644 --- a/libcontainer/cgroups/fs/hugetlb.go +++ b/libcontainer/cgroups/fs/hugetlb.go @@ -19,12 +19,8 @@ func (s *HugetlbGroup) Name() string { return "hugetlb" } -func (s *HugetlbGroup) Apply(d *cgroupData) error { - _, err := d.join("hugetlb") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *HugetlbGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *HugetlbGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -37,10 +33,6 @@ func (s *HugetlbGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *HugetlbGroup) Remove(d *cgroupData) error { - return removePath(d.path("hugetlb")) -} - func (s *HugetlbGroup) GetStats(path string, stats *cgroups.Stats) error { hugetlbStats := cgroups.HugetlbStats{} for _, pageSize := range HugePageSizes { diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index f7b8f999e87..41adcd38f47 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -37,11 +37,8 @@ func (s *MemoryGroup) Name() string { return "memory" } -func (s *MemoryGroup) Apply(d *cgroupData) (err error) { - path, err := d.path("memory") - if err != nil && !cgroups.IsNotFound(err) { - return err - } else if path == "" { +func (s *MemoryGroup) Apply(path string, d *cgroupData) (err error) { + if path == "" { return nil } if memoryAssigned(d.config) { @@ -66,11 +63,7 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) { // We need to join memory cgroup after set memory limits, because // kmem.limit_in_bytes can only be set when the cgroup is empty. - _, err = d.join("memory") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil + return join(path, d.pid) } func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error { @@ -165,10 +158,6 @@ func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *MemoryGroup) Remove(d *cgroupData) error { - return removePath(d.path("memory")) -} - func (s *MemoryGroup) GetStats(path string, stats *cgroups.Stats) error { // Set stats from memory.stat. statsFile, err := os.Open(filepath.Join(path, "memory.stat")) diff --git a/libcontainer/cgroups/fs/name.go b/libcontainer/cgroups/fs/name.go index d8cf1d87c04..9a5af709d24 100644 --- a/libcontainer/cgroups/fs/name.go +++ b/libcontainer/cgroups/fs/name.go @@ -16,10 +16,10 @@ func (s *NameGroup) Name() string { return s.GroupName } -func (s *NameGroup) Apply(d *cgroupData) error { +func (s *NameGroup) Apply(path string, d *cgroupData) error { if s.Join { // ignore errors if the named cgroup does not exist - d.join(s.GroupName) + join(path, d.pid) } return nil } @@ -28,13 +28,6 @@ func (s *NameGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *NameGroup) Remove(d *cgroupData) error { - if s.Join { - removePath(d.path(s.GroupName)) - } - return nil -} - func (s *NameGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/net_cls.go b/libcontainer/cgroups/fs/net_cls.go index 0212015255e..901e9554431 100644 --- a/libcontainer/cgroups/fs/net_cls.go +++ b/libcontainer/cgroups/fs/net_cls.go @@ -17,12 +17,8 @@ func (s *NetClsGroup) Name() string { return "net_cls" } -func (s *NetClsGroup) Apply(d *cgroupData) error { - _, err := d.join("net_cls") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *NetClsGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *NetClsGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -35,10 +31,6 @@ func (s *NetClsGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *NetClsGroup) Remove(d *cgroupData) error { - return removePath(d.path("net_cls")) -} - func (s *NetClsGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/net_prio.go b/libcontainer/cgroups/fs/net_prio.go index 2bdeedf80af..a9645de261b 100644 --- a/libcontainer/cgroups/fs/net_prio.go +++ b/libcontainer/cgroups/fs/net_prio.go @@ -15,12 +15,8 @@ func (s *NetPrioGroup) Name() string { return "net_prio" } -func (s *NetPrioGroup) Apply(d *cgroupData) error { - _, err := d.join("net_prio") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *NetPrioGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *NetPrioGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -33,10 +29,6 @@ func (s *NetPrioGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *NetPrioGroup) Remove(d *cgroupData) error { - return removePath(d.path("net_prio")) -} - func (s *NetPrioGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/perf_event.go b/libcontainer/cgroups/fs/perf_event.go index 5693676d3a5..dd1e98d0aed 100644 --- a/libcontainer/cgroups/fs/perf_event.go +++ b/libcontainer/cgroups/fs/perf_event.go @@ -14,22 +14,14 @@ func (s *PerfEventGroup) Name() string { return "perf_event" } -func (s *PerfEventGroup) Apply(d *cgroupData) error { - // we just want to join this group even though we don't set anything - if _, err := d.join("perf_event"); err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *PerfEventGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *PerfEventGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *PerfEventGroup) Remove(d *cgroupData) error { - return removePath(d.path("perf_event")) -} - func (s *PerfEventGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } diff --git a/libcontainer/cgroups/fs/pids.go b/libcontainer/cgroups/fs/pids.go index 7bf68013589..2b6848fde72 100644 --- a/libcontainer/cgroups/fs/pids.go +++ b/libcontainer/cgroups/fs/pids.go @@ -19,12 +19,8 @@ func (s *PidsGroup) Name() string { return "pids" } -func (s *PidsGroup) Apply(d *cgroupData) error { - _, err := d.join("pids") - if err != nil && !cgroups.IsNotFound(err) { - return err - } - return nil +func (s *PidsGroup) Apply(path string, d *cgroupData) error { + return join(path, d.pid) } func (s *PidsGroup) Set(path string, cgroup *configs.Cgroup) error { @@ -44,10 +40,6 @@ func (s *PidsGroup) Set(path string, cgroup *configs.Cgroup) error { return nil } -func (s *PidsGroup) Remove(d *cgroupData) error { - return removePath(d.path("pids")) -} - func (s *PidsGroup) GetStats(path string, stats *cgroups.Stats) error { current, err := fscommon.GetCgroupParamUint(path, "pids.current") if err != nil { diff --git a/libcontainer/cgroups/fs/util_test.go b/libcontainer/cgroups/fs/util_test.go index 2c50d6f3a29..3745d06b612 100644 --- a/libcontainer/cgroups/fs/util_test.go +++ b/libcontainer/cgroups/fs/util_test.go @@ -39,8 +39,7 @@ func NewCgroupTestUtil(subsystem string, t *testing.T) *cgroupTestUtil { if err != nil { t.Fatal(err) } - d.root = tempDir - testCgroupPath := filepath.Join(d.root, subsystem) + testCgroupPath := filepath.Join(tempDir, subsystem) if err != nil { t.Fatal(err) } diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index d139a7df6a5..2ab4e65ceb4 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -41,18 +41,7 @@ type subsystem interface { var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist") -type subsystemSet []subsystem - -func (s subsystemSet) Get(name string) (subsystem, error) { - for _, ss := range s { - if ss.Name() == name { - return ss, nil - } - } - return nil, errSubsystemDoesNotExist -} - -var legacySubsystems = subsystemSet{ +var legacySubsystems = []subsystem{ &fs.CpusetGroup{}, &fs.DevicesGroup{}, &fs.MemoryGroup{}, @@ -116,16 +105,14 @@ func (m *legacyManager) Apply(pid int) error { defer m.mu.Unlock() if c.Paths != nil { paths := make(map[string]string) + cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup") + if err != nil { + return err + } for name, path := range c.Paths { - _, err := getSubsystemPath(m.cgroups, name) - if err != nil { - // Don't fail if a cgroup hierarchy was not found, just skip this subsystem - if cgroups.IsNotFound(err) { - continue - } - return err + if _, ok := cgMap[name]; ok { + paths[name] = path } - paths[name] = path } m.paths = paths return cgroups.EnterPid(m.paths, pid) @@ -190,14 +177,16 @@ func (m *legacyManager) Apply(pid int) error { return err } - if err := joinCgroups(c, pid); err != nil { - return err - } - paths := make(map[string]string) for _, s := range legacySubsystems { subsystemPath, err := getSubsystemPath(m.cgroups, s.Name()) if err != nil { + // Even if it's `not found` error, we'll return err + // because devices cgroup is hard requirement for + // container security. + if s.Name() == "devices" { + return err + } // Don't fail if a cgroup hierarchy was not found, just skip this subsystem if cgroups.IsNotFound(err) { continue @@ -207,6 +196,11 @@ func (m *legacyManager) Apply(pid int) error { paths[s.Name()] = subsystemPath } m.paths = paths + + if err := m.joinCgroups(pid); err != nil { + return err + } + return nil } @@ -235,48 +229,25 @@ func (m *legacyManager) Path(subsys string) string { return m.paths[subsys] } -func join(c *configs.Cgroup, subsystem string, pid int) (string, error) { - path, err := getSubsystemPath(c, subsystem) - if err != nil { - return "", err - } - - if err := os.MkdirAll(path, 0755); err != nil { - return "", err - } - if err := cgroups.WriteCgroupProc(path, pid); err != nil { - return "", err - } - return path, nil -} - -func joinCgroups(c *configs.Cgroup, pid int) error { +func (m *legacyManager) joinCgroups(pid int) error { for _, sys := range legacySubsystems { name := sys.Name() switch name { case "name=systemd": // let systemd handle this case "cpuset": - path, err := getSubsystemPath(c, name) - if err != nil && !cgroups.IsNotFound(err) { - return err - } - s := &fs.CpusetGroup{} - if err := s.ApplyDir(path, c, pid); err != nil { - return err + if path, ok := m.paths[name]; ok { + s := &fs.CpusetGroup{} + if err := s.ApplyDir(path, m.cgroups, pid); err != nil { + return err + } } default: - _, err := join(c, name, pid) - if err != nil { - // Even if it's `not found` error, we'll return err - // because devices cgroup is hard requirement for - // container security. - if name == "devices" { + if path, ok := m.paths[name]; ok { + if err := os.MkdirAll(path, 0755); err != nil { return err } - // For other subsystems, omit the `not found` error - // because they are optional. - if !cgroups.IsNotFound(err) { + if err := cgroups.WriteCgroupProc(path, pid); err != nil { return err } } @@ -313,17 +284,14 @@ func getSubsystemPath(c *configs.Cgroup, subsystem string) (string, error) { } func (m *legacyManager) Freeze(state configs.FreezerState) error { - path, err := getSubsystemPath(m.cgroups, "freezer") - if err != nil { - return err + path, ok := m.paths["freezer"] + if !ok { + return errSubsystemDoesNotExist } prevState := m.cgroups.Resources.Freezer m.cgroups.Resources.Freezer = state - freezer, err := legacySubsystems.Get("freezer") - if err != nil { - return err - } - err = freezer.Set(path, m.cgroups) + freezer := &fs.FreezerGroup{} + err := freezer.Set(path, m.cgroups) if err != nil { m.cgroups.Resources.Freezer = prevState return err @@ -332,17 +300,17 @@ func (m *legacyManager) Freeze(state configs.FreezerState) error { } func (m *legacyManager) GetPids() ([]int, error) { - path, err := getSubsystemPath(m.cgroups, "devices") - if err != nil { - return nil, err + path, ok := m.paths["devices"] + if !ok { + return nil, errSubsystemDoesNotExist } return cgroups.GetPids(path) } func (m *legacyManager) GetAllPids() ([]int, error) { - path, err := getSubsystemPath(m.cgroups, "devices") - if err != nil { - return nil, err + path, ok := m.paths["devices"] + if !ok { + return nil, errSubsystemDoesNotExist } return cgroups.GetAllPids(path) } @@ -351,9 +319,9 @@ func (m *legacyManager) GetStats() (*cgroups.Stats, error) { m.mu.Lock() defer m.mu.Unlock() stats := cgroups.NewStats() - for name, path := range m.paths { - sys, err := legacySubsystems.Get(name) - if err == errSubsystemDoesNotExist || !cgroups.PathExists(path) { + for _, sys := range legacySubsystems { + path := m.paths[sys.Name()] + if path == "" { continue } if err := sys.GetStats(path, stats); err != nil { @@ -411,9 +379,9 @@ func (m *legacyManager) Set(container *configs.Config) error { for _, sys := range legacySubsystems { // Get the subsystem path, but don't error out for not found cgroups. - path, err := getSubsystemPath(container.Cgroups, sys.Name()) - if err != nil && !cgroups.IsNotFound(err) { - return err + path, ok := m.paths[sys.Name()] + if !ok { + continue } if err := sys.Set(path, container.Cgroups); err != nil { return err @@ -455,15 +423,12 @@ func (m *legacyManager) GetCgroups() (*configs.Cgroup, error) { } func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) { - path, err := getSubsystemPath(m.cgroups, "freezer") - if err != nil && !cgroups.IsNotFound(err) { - return configs.Undefined, err - } - freezer, err := legacySubsystems.Get("freezer") - if err != nil { - return configs.Undefined, err + path, ok := m.paths["freezer"] + if !ok { + return configs.Undefined, nil } - return freezer.(*fs.FreezerGroup).GetState(path) + freezer := &fs.FreezerGroup{} + return freezer.GetState(path) } func (m *legacyManager) Exists() bool { diff --git a/libcontainer/cgroups/v1_utils.go b/libcontainer/cgroups/v1_utils.go index f8487b0a977..a740ae762a7 100644 --- a/libcontainer/cgroups/v1_utils.go +++ b/libcontainer/cgroups/v1_utils.go @@ -8,6 +8,10 @@ import ( "os" "path/filepath" "strings" + "syscall" + + securejoin "github.com/cyphar/filepath-securejoin" + "golang.org/x/sys/unix" ) // Code in this source file are specific to cgroup v1, @@ -19,6 +23,8 @@ const ( var ( errUnified = errors.New("not implemented for cgroup v2 unified hierarchy") + + defaultPrefix = "/sys/fs/cgroup" ) type NotFoundError struct { @@ -43,11 +49,59 @@ func IsNotFound(err error) bool { return ok } +func tryDefaultPath(cgroupPath, subsystem string) string { + if !strings.HasPrefix(defaultPrefix, cgroupPath) { + return "" + } + + // remove possible prefix + subsystem = strings.TrimPrefix(subsystem, CgroupNamePrefix) + + // Make sure we're still under defaultPrefix, and resolve + // a possible symlink (like cpu -> cpu,cpuacct). + path, err := securejoin.SecureJoin(defaultPrefix, subsystem) + if err != nil { + return "" + } + + // (1) path should be a directory. + st, err := os.Lstat(path) + if err != nil || !st.IsDir() { + return "" + } + + // (2) path should be a mount point. + pst, err := os.Lstat(filepath.Dir(path)) + if err != nil { + return "" + } + + if st.Sys().(*syscall.Stat_t).Dev == pst.Sys().(*syscall.Stat_t).Dev { + // parent dir has the same dev -- path is not a mount point + return "" + } + + // (3) path should have 'cgroup' fs type. + fst := unix.Statfs_t{} + err = unix.Statfs(path, &fst) + if err != nil || fst.Type != unix.CGROUP_SUPER_MAGIC { + return "" + } + + return path +} + // https://www.kernel.org/doc/Documentation/cgroup-v1/cgroups.txt func FindCgroupMountpoint(cgroupPath, subsystem string) (string, error) { if IsCgroup2UnifiedMode() { return "", errUnified } + + // Avoid parsing mountinfo by trying the default path first, if possible. + if path := tryDefaultPath(cgroupPath, subsystem); path != "" { + return path, nil + } + mnt, _, err := FindCgroupMountpointAndRoot(cgroupPath, subsystem) return mnt, err } @@ -57,9 +111,7 @@ func FindCgroupMountpointAndRoot(cgroupPath, subsystem string) (string, string, return "", "", errUnified } - // We are not using mount.GetMounts() because it's super-inefficient, - // parsing it directly sped up x10 times because of not using Sscanf. - // It was one of two major performance drawbacks in container start. + // Avoid parsing mountinfo by checking if subsystem is valid/available if !isSubsystemAvailable(subsystem) { return "", "", NewNotFoundError(subsystem) }