Skip to content

Commit c2af9f1

Browse files
huww98gopherbot
authored andcommitted
internal/runtime/cgroup: fix path on non-root mount point
We should trim the mount root (4th field in /proc/self/mountinfo) from cgroup path read from /proc/self/cgroup before appending it to the mount point. Non-root mount points are very common in containers with cgroup v1. parseCPURelativePath is renamed to parseCPUCgroup, as it is unclear what it is relative to. cgroups(7) says "This pathname is relative to the mount point of the hierarchy." It should mean the root of the hierarchy, and we cannot concat it to arbirary cgroup mount point. So just use the word cgroup, since it parses /proc/self/cgroup. It now returns errMalformedFile if the cgroup pathname does not start with "/", and errPathTooLong if the pathname can't fit into the buffer. We already rely on this when composing the path, just make this explicit to avoid incorrect paths. We now parse cgroup first then parse the mount point accordingly. We consider the previously read cgroup pathname and version to ensure we got the desired mount point. The out buffer is reused to pass in the cgroup, to avoid extra memory allocation. This should also resolve the race mentioned in the comments, so removing those comments. If our cgroup changed between the two read syscalls, we will stick with the cgroup read from /proc/self/cgroup. This is the same behavior as cgroup change after FindCPU() returns, so nothing special to comment about now. parseCPUMount now returns error when the combined path is too long, to avoid panic or truncation if we got a really long path from mountinfo. cgrouptest is changed to use dev returned from stat() to detect filesystem boundary, since we don't return mount point and sub-path separately now. This also avoid using os.Root since we don't handle untrusted input here. os.Root is too complex, and the performance is bad. Fixes #76390 Change-Id: Ia9cbd7be3e58a2d51caf27a973fbd201dac06afc Reviewed-on: https://go-review.googlesource.com/c/go/+/723241 Reviewed-by: Michael Pratt <mpratt@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Auto-Submit: Michael Knyszek <mknyszek@google.com> Auto-Submit: Michael Pratt <mpratt@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
1 parent 6be5de4 commit c2af9f1

File tree

5 files changed

+421
-153
lines changed

5 files changed

+421
-153
lines changed

src/internal/cgrouptest/cgrouptest_linux.go

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,8 @@ func (c *CgroupV2) SetCPUMax(quota, period int64) error {
5050
//
5151
// This must not be used in parallel tests, as it affects the entire process.
5252
func InCgroupV2(t *testing.T, fn func(*CgroupV2)) {
53-
mount, rel := findCurrent(t)
54-
parent := findOwnedParent(t, mount, rel)
55-
orig := filepath.Join(mount, rel)
53+
orig := findCurrent(t)
54+
parent := findOwnedParent(t, orig)
5655

5756
// Make sure the parent allows children to control cpu.
5857
b, err := os.ReadFile(filepath.Join(parent, "cgroup.subtree_control"))
@@ -93,34 +92,25 @@ func InCgroupV2(t *testing.T, fn func(*CgroupV2)) {
9392
fn(c)
9493
}
9594

96-
// Returns the mount and relative directory of the current cgroup the process
97-
// is in.
98-
func findCurrent(t *testing.T) (string, string) {
95+
// Returns the filesystem path to the current cgroup the process is in.
96+
func findCurrent(t *testing.T) string {
9997
// Find the path to our current CPU cgroup. Currently this package is
10098
// only used for CPU cgroup testing, so the distinction of different
10199
// controllers doesn't matter.
102100
var scratch [cgroup.ParseSize]byte
103101
buf := make([]byte, cgroup.PathSize)
104-
n, err := cgroup.FindCPUMountPoint(buf, scratch[:])
102+
n, ver, err := cgroup.FindCPU(buf, scratch[:])
105103
if err != nil {
106104
t.Skipf("cgroup: unable to find current cgroup mount: %v", err)
107105
}
108-
mount := string(buf[:n])
109-
110-
n, ver, err := cgroup.FindCPURelativePath(buf, scratch[:])
111-
if err != nil {
112-
t.Skipf("cgroup: unable to find current cgroup path: %v", err)
113-
}
114106
if ver != cgroup.V2 {
115107
t.Skipf("cgroup: running on cgroup v%d want v2", ver)
116108
}
117-
rel := string(buf[1:n]) // The returned path always starts with /, skip it.
118-
rel = filepath.Join(".", rel) // Make sure this isn't empty string at root.
119-
return mount, rel
109+
return string(buf[:n])
120110
}
121111

122112
// Returns a parent directory in which we can create our own cgroup subdirectory.
123-
func findOwnedParent(t *testing.T, mount, rel string) string {
113+
func findOwnedParent(t *testing.T, orig string) string {
124114
// There are many ways cgroups may be set up on a system. We don't try
125115
// to cover all of them, just common ones.
126116
//
@@ -142,7 +132,7 @@ func findOwnedParent(t *testing.T, mount, rel string) string {
142132

143133
// We want to create our own subdirectory that we can migrate into and
144134
// then manipulate at will. It is tempting to create a new subdirectory
145-
// inside the current cgroup we are already in, however that will likey
135+
// inside the current cgroup we are already in, however that will likely
146136
// not work. cgroup v2 only allows processes to be in leaf cgroups. Our
147137
// current cgroup likely contains multiple processes (at least this one
148138
// and the cmd/go test runner). If we make a subdirectory and try to
@@ -166,35 +156,37 @@ func findOwnedParent(t *testing.T, mount, rel string) string {
166156
// is empty. As far as I tell, the only purpose of this is to allow
167157
// reorganizing processes into a new set of subdirectories and then
168158
// adding controllers once done.
169-
root, err := os.OpenRoot(mount)
159+
var stat syscall.Stat_t
160+
err := syscall.Stat(orig, &stat)
170161
if err != nil {
171-
t.Fatalf("error opening cgroup mount root: %v", err)
162+
t.Fatalf("error stating orig cgroup: %v", err)
172163
}
173164

174165
uid := os.Getuid()
175166
var prev string
176-
for rel != "." {
177-
fi, err := root.Stat(rel)
167+
cur := filepath.Dir(orig)
168+
for cur != "/" {
169+
var curStat syscall.Stat_t
170+
err = syscall.Stat(cur, &curStat)
178171
if err != nil {
179172
t.Fatalf("error stating cgroup path: %v", err)
180173
}
181174

182-
st := fi.Sys().(*syscall.Stat_t)
183-
if int(st.Uid) != uid {
184-
// Stop at first directory we don't own.
175+
if int(curStat.Uid) != uid || curStat.Dev != stat.Dev {
176+
// Stop at first directory we don't own or filesystem boundary.
185177
break
186178
}
187179

188-
prev = rel
189-
rel = filepath.Join(rel, "..")
180+
prev = cur
181+
cur = filepath.Dir(cur)
190182
}
191183

192184
if prev == "" {
193185
t.Skipf("No parent cgroup owned by UID %d", uid)
194186
}
195187

196188
// We actually want the last directory where we were the owner.
197-
return filepath.Join(mount, prev)
189+
return prev
198190
}
199191

200192
// Migrate the current process to the cgroup directory dst.

0 commit comments

Comments
 (0)