From 19b542a576a2db3d66a9b384e4822cc506bd52fc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Aug 2021 12:57:43 -0700 Subject: [PATCH] libct/cg/fs: move internal code out of fs.go Now fs.go is not very readable as its public API functions are intermixed with internal stuff about getting cgroup paths. Move that out to paths.go, without changing any code. Same for the tests -- move paths-related tests to paths_test.go. This commit is separate to make the review easier. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs.go | 163 ------------------------ libcontainer/cgroups/fs/fs_test.go | 96 -------------- libcontainer/cgroups/fs/paths.go | 175 ++++++++++++++++++++++++++ libcontainer/cgroups/fs/paths_test.go | 104 +++++++++++++++ 4 files changed, 279 insertions(+), 259 deletions(-) create mode 100644 libcontainer/cgroups/fs/paths.go create mode 100644 libcontainer/cgroups/fs/paths_test.go diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index 1c1e3f3e6..391c9532b 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "sync" "golang.org/x/sys/unix" @@ -12,7 +11,6 @@ import ( "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" - libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils" ) var ( @@ -63,105 +61,6 @@ 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 - cgroupRoot string -) - -const defaultCgroupRoot = "/sys/fs/cgroup" - -func tryDefaultCgroupRoot() string { - var st, pst unix.Stat_t - - // (1) it should be a directory... - err := unix.Lstat(defaultCgroupRoot, &st) - if err != nil || st.Mode&unix.S_IFDIR == 0 { - return "" - } - - // (2) ... and a mount point ... - err = unix.Lstat(filepath.Dir(defaultCgroupRoot), &pst) - if err != nil { - return "" - } - - if st.Dev == pst.Dev { - // parent dir has the same dev -- not a mount point - return "" - } - - // (3) ... of 'tmpfs' fs type. - var fst unix.Statfs_t - err = unix.Statfs(defaultCgroupRoot, &fst) - if err != nil || fst.Type != unix.TMPFS_MAGIC { - return "" - } - - // (4) it should have at least 1 entry ... - dir, err := os.Open(defaultCgroupRoot) - if err != nil { - return "" - } - names, err := dir.Readdirnames(1) - if err != nil { - return "" - } - if len(names) < 1 { - return "" - } - // ... which is a cgroup mount point. - err = unix.Statfs(filepath.Join(defaultCgroupRoot, names[0]), &fst) - if err != nil || fst.Type != unix.CGROUP_SUPER_MAGIC { - return "" - } - - return defaultCgroupRoot -} - -// Gets the cgroupRoot. -func getCgroupRoot() (string, error) { - cgroupRootLock.Lock() - defer cgroupRootLock.Unlock() - - if cgroupRoot != "" { - return cgroupRoot, nil - } - - // fast path - cgroupRoot = tryDefaultCgroupRoot() - if cgroupRoot != "" { - return cgroupRoot, nil - } - - // slow path: parse mountinfo - mi, err := cgroups.GetCgroupMounts(false) - if err != nil { - return "", err - } - if len(mi) < 1 { - return "", errors.New("no cgroup mount found in mountinfo") - } - - // Get the first cgroup mount (e.g. "/sys/fs/cgroup/memory"), - // use its parent directory. - root := filepath.Dir(mi[0].Mountpoint) - - if _, err := os.Stat(root); err != nil { - return "", err - } - - cgroupRoot = root - return cgroupRoot, nil -} - -type cgroupData struct { - root string - innerPath string - config *configs.Cgroup - pid int -} - // isIgnorableError returns whether err is a permission error (in the loose // sense of the word). This includes EROFS (which for an unprivileged user is // basically a permission error) and EACCES (for similar reasons) as well as @@ -315,68 +214,6 @@ func (m *manager) GetAllPids() ([]int, error) { return cgroups.GetAllPids(m.Path("devices")) } -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") - } - - // XXX: Do not remove this code. Path safety is important! -- cyphar - cgPath := libcontainerUtils.CleanPath(c.Path) - cgParent := libcontainerUtils.CleanPath(c.Parent) - cgName := libcontainerUtils.CleanPath(c.Name) - - innerPath := cgPath - if innerPath == "" { - innerPath = filepath.Join(cgParent, cgName) - } - - return &cgroupData{ - root: root, - innerPath: innerPath, - config: c, - pid: pid, - }, nil -} - -func (raw *cgroupData) path(subsystem string) (string, error) { - // 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(raw.root, 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 - } - - // Use GetOwnCgroupPath instead of GetInitCgroupPath, because the creating - // process could in container and shared pid namespace with host, and - // /proc/1/cgroup could point to whole other world of cgroups. - parentPath, err := cgroups.GetOwnCgroupPath(subsystem) - if err != nil { - return "", err - } - - return filepath.Join(parentPath, raw.innerPath), nil -} - -func join(path string, pid int) error { - if path == "" { - return nil - } - if err := os.MkdirAll(path, 0o755); err != nil { - return err - } - return cgroups.WriteCgroupProc(path, pid) -} - func (m *manager) GetPaths() map[string]string { m.mu.Lock() defer m.mu.Unlock() diff --git a/libcontainer/cgroups/fs/fs_test.go b/libcontainer/cgroups/fs/fs_test.go index b4430603f..7a9176860 100644 --- a/libcontainer/cgroups/fs/fs_test.go +++ b/libcontainer/cgroups/fs/fs_test.go @@ -1,108 +1,12 @@ package fs import ( - "path/filepath" - "strings" "testing" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" ) -func TestInvalidCgroupPath(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - - root, err := getCgroupRoot() - if err != nil { - t.Fatalf("couldn't get cgroup root: %v", err) - } - - 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(root, "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!") - } - }) - } -} - -func TestTryDefaultCgroupRoot(t *testing.T) { - res := tryDefaultCgroupRoot() - exp := defaultCgroupRoot - if cgroups.IsCgroup2UnifiedMode() { - // checking that tryDefaultCgroupRoot does return "" - // in case /sys/fs/cgroup is not cgroup v1 root dir. - exp = "" - } - if res != exp { - t.Errorf("tryDefaultCgroupRoot: want %q, got %q", exp, res) - } -} - func BenchmarkGetStats(b *testing.B) { if cgroups.IsCgroup2UnifiedMode() { b.Skip("cgroup v2 is not supported") diff --git a/libcontainer/cgroups/fs/paths.go b/libcontainer/cgroups/fs/paths.go new file mode 100644 index 000000000..befa81bcb --- /dev/null +++ b/libcontainer/cgroups/fs/paths.go @@ -0,0 +1,175 @@ +package fs + +import ( + "errors" + "os" + "path/filepath" + "sync" + + "golang.org/x/sys/unix" + + "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runc/libcontainer/utils" +) + +// The absolute path to the root of the cgroup hierarchies. +var ( + cgroupRootLock sync.Mutex + cgroupRoot string +) + +const defaultCgroupRoot = "/sys/fs/cgroup" + +func tryDefaultCgroupRoot() string { + var st, pst unix.Stat_t + + // (1) it should be a directory... + err := unix.Lstat(defaultCgroupRoot, &st) + if err != nil || st.Mode&unix.S_IFDIR == 0 { + return "" + } + + // (2) ... and a mount point ... + err = unix.Lstat(filepath.Dir(defaultCgroupRoot), &pst) + if err != nil { + return "" + } + + if st.Dev == pst.Dev { + // parent dir has the same dev -- not a mount point + return "" + } + + // (3) ... of 'tmpfs' fs type. + var fst unix.Statfs_t + err = unix.Statfs(defaultCgroupRoot, &fst) + if err != nil || fst.Type != unix.TMPFS_MAGIC { + return "" + } + + // (4) it should have at least 1 entry ... + dir, err := os.Open(defaultCgroupRoot) + if err != nil { + return "" + } + names, err := dir.Readdirnames(1) + if err != nil { + return "" + } + if len(names) < 1 { + return "" + } + // ... which is a cgroup mount point. + err = unix.Statfs(filepath.Join(defaultCgroupRoot, names[0]), &fst) + if err != nil || fst.Type != unix.CGROUP_SUPER_MAGIC { + return "" + } + + return defaultCgroupRoot +} + +// Gets the cgroupRoot. +func getCgroupRoot() (string, error) { + cgroupRootLock.Lock() + defer cgroupRootLock.Unlock() + + if cgroupRoot != "" { + return cgroupRoot, nil + } + + // fast path + cgroupRoot = tryDefaultCgroupRoot() + if cgroupRoot != "" { + return cgroupRoot, nil + } + + // slow path: parse mountinfo + mi, err := cgroups.GetCgroupMounts(false) + if err != nil { + return "", err + } + if len(mi) < 1 { + return "", errors.New("no cgroup mount found in mountinfo") + } + + // Get the first cgroup mount (e.g. "/sys/fs/cgroup/memory"), + // use its parent directory. + root := filepath.Dir(mi[0].Mountpoint) + + if _, err := os.Stat(root); err != nil { + return "", err + } + + cgroupRoot = root + return cgroupRoot, nil +} + +type cgroupData struct { + root string + innerPath string + config *configs.Cgroup + pid int +} + +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") + } + + // XXX: Do not remove this code. Path safety is important! -- cyphar + cgPath := utils.CleanPath(c.Path) + cgParent := utils.CleanPath(c.Parent) + cgName := utils.CleanPath(c.Name) + + innerPath := cgPath + if innerPath == "" { + innerPath = filepath.Join(cgParent, cgName) + } + + return &cgroupData{ + root: root, + innerPath: innerPath, + config: c, + pid: pid, + }, nil +} + +func (raw *cgroupData) path(subsystem string) (string, error) { + // 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(raw.root, 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 + } + + // Use GetOwnCgroupPath instead of GetInitCgroupPath, because the creating + // process could in container and shared pid namespace with host, and + // /proc/1/cgroup could point to whole other world of cgroups. + parentPath, err := cgroups.GetOwnCgroupPath(subsystem) + if err != nil { + return "", err + } + + return filepath.Join(parentPath, raw.innerPath), nil +} + +func join(path string, pid int) error { + if path == "" { + return nil + } + if err := os.MkdirAll(path, 0o755); err != nil { + return err + } + return cgroups.WriteCgroupProc(path, pid) +} diff --git a/libcontainer/cgroups/fs/paths_test.go b/libcontainer/cgroups/fs/paths_test.go new file mode 100644 index 000000000..58ebfc7c4 --- /dev/null +++ b/libcontainer/cgroups/fs/paths_test.go @@ -0,0 +1,104 @@ +package fs + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/configs" +) + +func TestInvalidCgroupPath(t *testing.T) { + if cgroups.IsCgroup2UnifiedMode() { + t.Skip("cgroup v2 is not supported") + } + + root, err := getCgroupRoot() + if err != nil { + t.Fatalf("couldn't get cgroup root: %v", err) + } + + 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(root, "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!") + } + }) + } +} + +func TestTryDefaultCgroupRoot(t *testing.T) { + res := tryDefaultCgroupRoot() + exp := defaultCgroupRoot + if cgroups.IsCgroup2UnifiedMode() { + // checking that tryDefaultCgroupRoot does return "" + // in case /sys/fs/cgroup is not cgroup v1 root dir. + exp = "" + } + if res != exp { + t.Errorf("tryDefaultCgroupRoot: want %q, got %q", exp, res) + } +}