libct/cg/OpenFile: fix/improve openat2 handling #3030
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolyshkin
changed the title
|
No need to backport it to 1.0 -- this is a bug, but the fallback works so the impact is pretty low |
kolyshkin
force-pushed
the
openat2-improve
branch
from
2e55f8b to
1252538
Compare
kolyshkin
force-pushed
the
openat2-improve
branch
from
1252538 to
a0bd98c
Compare
|
This is a real bug fix, can @opencontainers/runc-maintainers PTAL 🙏🏻 ? |
kolyshkin
force-pushed
the
openat2-improve
branch
from
a0bd98c to
40b41b2
Compare
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 <[email protected]>
kolyshkin
force-pushed
the
openat2-improve
branch
from
40b41b2 to
b6c8beb
Compare
kolyshkin
commented
Aug 9, 2021
Comment on lines
+124
to
+130
| 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) |
There was a problem hiding this comment.
To help reviewers.
Basically, the code previously assumed that dir starts with /sys/fs/cgroup/ (including the ending slash), which is not always the case (such as when calling OpenFile("/sys/fs/cgroup", "cgroup.controllers")). So, the fallback was used for this case, which is not correct.
This commit removes such assumption, using path.Join(dir, file) as a full path. This simplifies the code, and helps the case above, as well as something like OpenFile("/sys", "fs/cgroup/cgroup.controllers").
AkihiroSuda
reviewed
Aug 11, 2021
libcontainer/cgroups/file.go
Outdated
|
|
||
| // Enhance the Path in the error to contain the | ||
| // cgroupFd value and the directory it is opened to, | ||
| // for example: "@[fd 7:/!=/sys/fs/cgroup]/cpu.stat". |
There was a problem hiding this comment.
Is this convention already widely used, or newly invented here?
Rather than defining a new microformat, can we just wrap os.PathError with our own struct?
There was a problem hiding this comment.
Is this convention already widely used, or newly invented here?
Newly invented (originally comes from #3026 (comment))
We are already using "fd N" in a few places where we operate on a file descriptor:
[kir@kir-rhat runc]$ git grep PathError | grep -w fd
libcontainer/rootfs_linux.go: return &os.PathError{Op: "fchdir", Path: "fd " + strconv.Itoa(newroot), Err: err}
libcontainer/rootfs_linux.go: return &os.PathError{Op: "fchdir", Path: "fd " + strconv.Itoa(oldroot), Err: err}
libcontainer/setns_init_linux.go: return &os.PathError{Op: "close log pipe", Path: "fd " + strconv.Itoa(l.logFd), Err: err}
libcontainer/standard_init_linux.go: return &os.PathError{Op: "close log pipe", Path: "fd " + strconv.Itoa(l.logFd), Err: err}
vendor/github.com/opencontainers/selinux/go-selinux/selinux_linux.go: return "", &os.PathError{Op: "getsockopt", Path: "fd " + strconv.Itoa(int(fd)), Err: err}but in this case something more useful is needed.
Rather than defining a new microformat, can we just wrap os.PathError with our own struct?
Done
There was a problem hiding this comment.
@AkihiroSuda PTAL ^^^
kolyshkin
force-pushed
the
openat2-improve
branch
from
b6c8beb to
4c6cc14
Compare
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 <[email protected]>
kolyshkin
force-pushed
the
openat2-improve
branch
from
4c6cc14 to
bd50e7c
Compare
AkihiroSuda
approved these changes
Sep 2, 2021
|
@cyphar @thaJeztah @mrunalp PTAL |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking into libct/cg: openat2 optimization not working across container boundaries #3026, I started writing a test case and found out that
OpenFile("/sys/fs/cgroup", name)falls back to using open and fstatfs.This is caused by a peculiarities of its
dirandnamearguments handling,and was never discovered before because of the fallback.
Fix the bug, add the test cases.
Improve the error message for case like in the issue libct/cg: openat2 optimization not working across container boundaries #3026, based on @cyphar suggestion.
See commit messages for more details.