From c2d9668cc587fe2314e5329550d4cf5e97aea958 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 14 Jun 2021 19:13:12 -0700 Subject: [PATCH 1/2] libct/cg/OpenFile: fix openat2 vs top cgroup dir Fix reading cgroup files from the top cgroup directory, i.e. /sys/fs/cgroup. The code was working for for any subdirectory of /sys/fs/cgroup, but for dir="/sys/fs/cgroup" a fallback (open and fstatfs) was used, because of the way the function worked with the dir argument. Fix those cases, and add unit tests to make sure they work. While at it, make the rules for dir and name components more relaxed, and add test cases for this, too. While at it, improve OpenFile documentation, and remove a duplicated doc comment for openFile. Without these fixes, the unit test fails the following cases: file_test.go:67: case {path:/sys/fs/cgroup name:cgroup.controllers}: fallback file_test.go:67: case {path:/sys/fs/cgroup/ name:cgroup.controllers}: openat2 /sys/fs/cgroup//cgroup.controllers: invalid cross-device link file_test.go:67: case {path:/sys/fs/cgroup/ name:/cgroup.controllers}: openat2 /sys/fs/cgroup///cgroup.controllers: invalid cross-device link file_test.go:67: case {path:/ name:/sys/fs/cgroup/cgroup.controllers}: fallback file_test.go:67: case {path:/ name:sys/fs/cgroup/cgroup.controllers}: fallback file_test.go:67: case {path:/sys/fs/cgroup/cgroup.controllers name:}: openat2 /sys/fs/cgroup/cgroup.controllers/: not a directory Here "fallback" means openat2-based implementation fails, and the fallback code is used (and works). Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/file.go | 33 ++++++++++++++++++------------- libcontainer/cgroups/file_test.go | 33 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/libcontainer/cgroups/file.go b/libcontainer/cgroups/file.go index b1888f7ed..4ac77c24c 100644 --- a/libcontainer/cgroups/file.go +++ b/libcontainer/cgroups/file.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "path" "strings" "sync" @@ -13,7 +14,11 @@ import ( ) // OpenFile opens a cgroup file in a given dir with given flags. -// It is supposed to be used for cgroup files only. +// It is supposed to be used for cgroup files only, and returns +// an error if the file is not a cgroup file. +// +// Arguments dir and file are joined together to form an absolute path +// to a file being opened. func OpenFile(dir, file string, flags int) (*os.File, error) { if dir == "" { return nil, fmt.Errorf("no directory specified for %s", file) @@ -109,8 +114,6 @@ func prepareOpenat2() error { return prepErr } -// OpenFile opens a cgroup file in a given dir with given flags. -// It is supposed to be used for cgroup files only. func openFile(dir, file string, flags int) (*os.File, error) { mode := os.FileMode(0) if TestMode && flags&os.O_WRONLY != 0 { @@ -118,34 +121,36 @@ func openFile(dir, file string, flags int) (*os.File, error) { flags |= os.O_TRUNC | os.O_CREATE mode = 0o600 } + path := path.Join(dir, file) if prepareOpenat2() != nil { - return openFallback(dir, file, flags, mode) + return openFallback(path, flags, mode) } - reldir := strings.TrimPrefix(dir, cgroupfsPrefix) - if len(reldir) == len(dir) { // non-standard path, old system? - return openFallback(dir, file, flags, mode) + relPath := strings.TrimPrefix(path, cgroupfsPrefix) + if len(relPath) == len(path) { // non-standard path, old system? + return openFallback(path, flags, mode) } - relname := reldir + "/" + file - fd, err := unix.Openat2(cgroupFd, relname, + fd, err := unix.Openat2(cgroupFd, relPath, &unix.OpenHow{ Resolve: resolveFlags, Flags: uint64(flags) | unix.O_CLOEXEC, Mode: uint64(mode), }) if err != nil { - return nil, &os.PathError{Op: "openat2", Path: dir + "/" + file, Err: err} + return nil, &os.PathError{Op: "openat2", Path: path, Err: err} } - return os.NewFile(uintptr(fd), cgroupfsPrefix+relname), nil + return os.NewFile(uintptr(fd), path), nil } var errNotCgroupfs = errors.New("not a cgroup file") -// openFallback is used when openat2(2) is not available. It checks the opened +// Can be changed by unit tests. +var openFallback = openAndCheck + +// openAndCheck 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 +func openAndCheck(path string, flags int, mode os.FileMode) (*os.File, error) { fd, err := os.OpenFile(path, flags, mode) if err != nil { return nil, err diff --git a/libcontainer/cgroups/file_test.go b/libcontainer/cgroups/file_test.go index 4b2cb8950..9f7be0b88 100644 --- a/libcontainer/cgroups/file_test.go +++ b/libcontainer/cgroups/file_test.go @@ -3,6 +3,7 @@ package cgroups import ( + "errors" "fmt" "os" "path/filepath" @@ -40,3 +41,35 @@ func TestWriteCgroupFileHandlesInterrupt(t *testing.T) { } } } + +func TestOpenat2(t *testing.T) { + if !IsCgroup2UnifiedMode() { + // The reason is many test cases below test opening files from + // the top-level directory, where cgroup v1 has no files. + t.Skip("test requires cgroup v2") + } + + // Make sure we test openat2, not its fallback. + openFallback = func(_ string, _ int, _ os.FileMode) (*os.File, error) { + return nil, errors.New("fallback") + } + defer func() { openFallback = openAndCheck }() + + for _, tc := range []struct{ dir, file string }{ + {"/sys/fs/cgroup", "cgroup.controllers"}, + {"/sys/fs/cgroup", "/cgroup.controllers"}, + {"/sys/fs/cgroup/", "cgroup.controllers"}, + {"/sys/fs/cgroup/", "/cgroup.controllers"}, + {"/sys/fs/cgroup/user.slice", "cgroup.controllers"}, + {"/sys/fs/cgroup/user.slice/", "/cgroup.controllers"}, + {"/", "/sys/fs/cgroup/cgroup.controllers"}, + {"/", "sys/fs/cgroup/cgroup.controllers"}, + {"/sys/fs/cgroup/cgroup.controllers", ""}, + } { + fd, err := OpenFile(tc.dir, tc.file, os.O_RDONLY) + if err != nil { + t.Errorf("case %+v: %v", tc, err) + } + fd.Close() + } +} From bd50e7c420457a25212ddb02cb91996a714a8237 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 14 Jun 2021 19:50:54 -0700 Subject: [PATCH 2/2] libct/cg/OpenFile: check cgroupFd on error opencontainers/runc issue 3026 describes a scenario in which OpenFile failed to open a legitimate existing cgroupfs file. Added debug (similar to what this commit does) shown that cgroupFd is no longer opened to "/sys/fs/cgroup", but to "/" (it's not clear what caused it, and the source code is not available, but they might be using the same process on the both sides of the container/chroot/pivot_root/mntns boundary, or remounting /sys/fs/cgroup). Consider such use incorrect, but give a helpful hint as two what is going on by wrapping the error in a more useful message. NB: this can potentially be fixed by reopening the cgroupFd once we detected that it's screwed, and retrying openat2. Alas I do not have a test case for this, so left this as a TODO suggestion. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/file.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/libcontainer/cgroups/file.go b/libcontainer/cgroups/file.go index 4ac77c24c..0cdaf7478 100644 --- a/libcontainer/cgroups/file.go +++ b/libcontainer/cgroups/file.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path" + "strconv" "strings" "sync" @@ -137,7 +138,23 @@ func openFile(dir, file string, flags int) (*os.File, error) { Mode: uint64(mode), }) if err != nil { - return nil, &os.PathError{Op: "openat2", Path: path, Err: err} + err = &os.PathError{Op: "openat2", Path: path, Err: err} + // Check if cgroupFd is still opened to cgroupfsDir + // (happens when this package is incorrectly used + // across the chroot/pivot_root/mntns boundary, or + // when /sys/fs/cgroup is remounted). + // + // 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) + 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) + } + return nil, err } return os.NewFile(uintptr(fd), path), nil