From 0bee5e0b01a5be4cc35932d54c56c770e0645f58 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 26 Apr 2021 19:59:32 -0700 Subject: [PATCH 1/3] libct/cg/fs: add GetStats benchmark On my CentOS 8 VM it shows: > BenchmarkGetStats-8 8376 625135 ns/op Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs_test.go | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/libcontainer/cgroups/fs/fs_test.go b/libcontainer/cgroups/fs/fs_test.go index c899ff8f18f..dfcad78b43c 100644 --- a/libcontainer/cgroups/fs/fs_test.go +++ b/libcontainer/cgroups/fs/fs_test.go @@ -104,3 +104,35 @@ func TestTryDefaultCgroupRoot(t *testing.T) { t.Errorf("tryDefaultCgroupRoot: want %q, got %q", exp, res) } } + +func BenchmarkGetStats(b *testing.B) { + if cgroups.IsCgroup2UnifiedMode() { + b.Skip("cgroup v2 is not supported") + } + + cg := &configs.Cgroup{ + Path: "/some/kind/of/a/path/here", + Resources: &configs.Resources{}, + } + m := NewManager(cg, nil, false) + err := m.Apply(-1) + if err != nil { + b.Fatal(err) + } + defer func() { + _ = m.Destroy() + }() + + var st *cgroups.Stats + + b.ResetTimer() + for i := 0; i < b.N; i++ { + st, err = m.GetStats() + if err != nil { + b.Fatal(err) + } + } + if st.CpuStats.CpuUsage.TotalUsage != 0 { + b.Fatalf("stats: %+v", st) + } +} From 9efd8466ab6f6aff270b5c7b9c84dcfaf4e6ecba Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 26 Apr 2021 18:02:08 -0700 Subject: [PATCH 2/3] libct/cg/fscommon.OpenFile: reverse checks order In case openat2() is not available, it does not make sense to calculate relpath (and check if path has /sys/fs/cgroup prefix). Reverse the order of checks to not do that in case openat2 is not available. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/open.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fscommon/open.go b/libcontainer/cgroups/fscommon/open.go index 0a7e3d95282..ddaac3f4532 100644 --- a/libcontainer/cgroups/fscommon/open.go +++ b/libcontainer/cgroups/fscommon/open.go @@ -71,11 +71,11 @@ func OpenFile(dir, file string, flags int) (*os.File, error) { flags |= os.O_TRUNC | os.O_CREATE mode = 0o600 } - reldir := strings.TrimPrefix(dir, cgroupfsPrefix) - if len(reldir) == len(dir) { // non-standard path, old system? + if prepareOpenat2() != nil { return openWithSecureJoin(dir, file, flags, mode) } - if prepareOpenat2() != nil { + reldir := strings.TrimPrefix(dir, cgroupfsPrefix) + if len(reldir) == len(dir) { // non-standard path, old system? return openWithSecureJoin(dir, file, flags, mode) } From 850b2c47b22c51f8d2c4a68452a6bea60d458192 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 29 Apr 2021 14:09:59 -0700 Subject: [PATCH 3/3] libct/cg/fscommon.OpenFile: speed up ro case Commit 88e8350de28, among the other things, replaced filepath.Join with securejoin.SecureJoin for both reads and writes to cgroupfs. Commits e76ac1c054 and 31f0f5b7e03a switched more code to use fscommon.ReadFile (and thus securejoin). Commit 0228226e6d introduced fscommon.OpenFile (which uses securejoin as the fallback if openat2(2) is not available, which is the case for older kernels), and commit c95e69007c4 switched most of cgroup/fs[2] code to use it. As a result, fs.GetStats() method became noticeable slower, mostly due to securejoin calling os.Lstat and filepath.Clean. Using securejoin as a security measure for cgroupfs files is not well justified, as cgroupfs do not contain symlinks, and none of the code using it have uncleaned paths. In particular, fs/fs2/systemd managers do check and sanitize their paths. This commit modifies the code to not use securejoin. Instead, it checks that the opened file is indeed on cgroupfs. Using BenchmarkGetStats on a CentOS 8 VM, I see the following improvement: Before: > BenchmarkGetStats-8 8376 625135 ns/op After: > BenchmarkGetStats-8 12226 485015 ns/op An intermediate version, with no fstatfs to check fstype: > BenchmarkGetStats-8 13162 452281 ns/op Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs_test.go | 8 ++++++ libcontainer/cgroups/fscommon/open.go | 31 ++++++++++++++++----- libcontainer/cgroups/fscommon/utils_test.go | 4 +++ 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/libcontainer/cgroups/fs/fs_test.go b/libcontainer/cgroups/fs/fs_test.go index dfcad78b43c..b19b26e44b3 100644 --- a/libcontainer/cgroups/fs/fs_test.go +++ b/libcontainer/cgroups/fs/fs_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" ) @@ -110,6 +111,13 @@ func BenchmarkGetStats(b *testing.B) { b.Skip("cgroup v2 is not supported") } + // Unset TestMode as we work with real cgroupfs here, + // and we want OpenFile to perform the fstype check. + fscommon.TestMode = false + defer func() { + fscommon.TestMode = true + }() + cg := &configs.Cgroup{ Path: "/some/kind/of/a/path/here", Resources: &configs.Resources{}, diff --git a/libcontainer/cgroups/fscommon/open.go b/libcontainer/cgroups/fscommon/open.go index ddaac3f4532..49af83b3c0d 100644 --- a/libcontainer/cgroups/fscommon/open.go +++ b/libcontainer/cgroups/fscommon/open.go @@ -5,7 +5,6 @@ import ( "strings" "sync" - securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -17,7 +16,7 @@ const ( ) var ( - // Set to true by fs unit tests + // TestMode is set to true by unit tests that need "fake" cgroupfs. TestMode bool cgroupFd int = -1 @@ -72,11 +71,11 @@ func OpenFile(dir, file string, flags int) (*os.File, error) { mode = 0o600 } if prepareOpenat2() != nil { - return openWithSecureJoin(dir, file, flags, mode) + return openFallback(dir, file, flags, mode) } reldir := strings.TrimPrefix(dir, cgroupfsPrefix) if len(reldir) == len(dir) { // non-standard path, old system? - return openWithSecureJoin(dir, file, flags, mode) + return openFallback(dir, file, flags, mode) } relname := reldir + "/" + file @@ -93,11 +92,29 @@ func OpenFile(dir, file string, flags int) (*os.File, error) { return os.NewFile(uintptr(fd), cgroupfsPrefix+relname), nil } -func openWithSecureJoin(dir, file string, flags int, mode os.FileMode) (*os.File, error) { - path, err := securejoin.SecureJoin(dir, file) +var errNotCgroupfs = errors.New("not a cgroup file") + +// openFallback is used when openat2(2) is not available. It checks the opened +// file is on cgroupfs, returning an error otherwise. +func openFallback(dir, file string, flags int, mode os.FileMode) (*os.File, error) { + path := dir + "/" + file + fd, err := os.OpenFile(path, flags, mode) if err != nil { return nil, err } + if TestMode { + return fd, nil + } + // Check this is a cgroupfs file. + var st unix.Statfs_t + if err := unix.Fstatfs(int(fd.Fd()), &st); err != nil { + _ = fd.Close() + return nil, &os.PathError{Op: "statfs", Path: path, Err: err} + } + if st.Type != unix.CGROUP_SUPER_MAGIC && st.Type != unix.CGROUP2_SUPER_MAGIC { + _ = fd.Close() + return nil, &os.PathError{Op: "open", Path: path, Err: errNotCgroupfs} + } - return os.OpenFile(path, flags, mode) + return fd, nil } diff --git a/libcontainer/cgroups/fscommon/utils_test.go b/libcontainer/cgroups/fscommon/utils_test.go index d0c5668b5ea..f56cdd7ef4f 100644 --- a/libcontainer/cgroups/fscommon/utils_test.go +++ b/libcontainer/cgroups/fscommon/utils_test.go @@ -17,6 +17,10 @@ const ( floatString = "2048" ) +func init() { + TestMode = true +} + func TestGetCgroupParamsInt(t *testing.T) { // Setup tempdir. tempDir, err := ioutil.TempDir("", "cgroup_utils_test")