From b303e56ddb4ecc46cc99670761efa9ac033253d6 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 26 Apr 2021 18:02:08 -0700 Subject: [PATCH 1/2] 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 (cherry picked from commit 272d34f5e82b7a0dd354fa828e65e218241080ad) 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 0a7e3d952..ddaac3f45 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 2a0978ddd67f1d2c5375011b0b0f2cdaa32ace09 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 26 Apr 2021 19:27:58 -0700 Subject: [PATCH 2/2] 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 while reading cgroupfs files is not well justified, as cgroupfs do not contain symlinks, and none of the GetStats code have uncleaned paths. This patch modifies the code to not use securejoin in case the file is to be opened read-only. Using BenchmarkGetStats on a CentOS 8 VM, I see the following improvement: Before: > BenchmarkGetStats-8 8376 625135 ns/op After: > BenchmarkGetStats-8 13162 452281 ns/op Signed-off-by: Kir Kolyshkin (cherry picked from commit 474a605c79101fad941da5ecc3b7a817ff049bf3) Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/open.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/libcontainer/cgroups/fscommon/open.go b/libcontainer/cgroups/fscommon/open.go index ddaac3f45..48adc0f6e 100644 --- a/libcontainer/cgroups/fscommon/open.go +++ b/libcontainer/cgroups/fscommon/open.go @@ -72,11 +72,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,10 +93,18 @@ 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) - if err != nil { - return nil, err +func openFallback(dir, file string, flags int, mode os.FileMode) (*os.File, error) { + var path string + // Do not use securejoin for files opened read-only, + // to speed up operations like GetStats. + if flags&unix.O_ACCMODE == unix.O_RDONLY { + path = dir + "/" + file + } else { + var err error + path, err = securejoin.SecureJoin(dir, file) + if err != nil { + return nil, err + } } return os.OpenFile(path, flags, mode)