Fix BPF-related fd leaks#60
Merged
Merged
Conversation
Closing the fd just drops our user-space reference to the object. The fds are not returned to the caller, we can just defer the close. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
rata
force-pushed
the
main
branch
2 times, most recently
from
c9032b9 to
8dc8d27
Compare
The function is not exported and the only caller was not using the
closer() function returned. Let's just remove it.
This also simplifies the fd leak fixes on the next patch. The semantics
of this closer function are not nice:
* If we return the closer function, we need to keep the fd open and
close it when the closer is called (it uses the prog.Fd() to search
for the BPF program, so the fd needs to be open).
* If the closer function fails, it's not clear when we should close the
prog fd to avoid issues on retries.
Let's just remove this closer function that is not used. It simplifies
things significantly.
Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
The program is either attached and the kernel keeps this, we can close the fd, or we fail and we should also close the fd. Let's just defer the close to it. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
On error we were not closing the program fds. Let's add a defer to handle that case. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
Member
Author
haircommander
approved these changes
Jun 17, 2026
haircommander
left a comment
haircommander
left a comment
Contributor
There was a problem hiding this comment.
I imagine a unit test to catch the leaked files would be too cumbersome to write, but that could be cool to make sure we don't regress in the future
kolyshkin
approved these changes
Jun 17, 2026
kolyshkin
left a comment
kolyshkin
left a comment
Contributor
There was a problem hiding this comment.
Loosely related: opencontainers/runc#5218 (which I intend to finalize once I have time).
I barely remember removing nilCloser, too, when working on the above.
All commits LGTM.
Contributor
|
@rata v0.0.7 (shaken, not stirred) is tagged. |
Member
Author
|
@kolyshkin hahaha, awesome! |
Member
Author
Great mind think alike :-D |
rata
added a commit
to rata/runc
that referenced
this pull request
Jun 18, 2026
This new version includes fixes for EPBF fd leaks: opencontainers/cgroups#60 Let's update to it. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
rata
added a commit
to rata/runc
that referenced
this pull request
Jun 18, 2026
This new version includes fixes for EPBF fd leaks: opencontainers/cgroups#60 Let's update to it. However, the Stats() method was added in the new release to the interface, so tests were failing with: # github.com/opencontainers/runc/libcontainer [github.com/opencontainers/runc/libcontainer.test] ./container_linux_test.go:125:18: cannot use &mockCgroupManager{…} (value of type *mockCgroupManager) as cgroups.Manager value in struct literal: *mockCgroupManager does not implement cgroups.Manager (missing method Stats) Let's add the method to the mock too. In the future, probably the mock should come from the cgroups module. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
rata
added a commit
to rata/runc
that referenced
this pull request
Jun 18, 2026
This new version includes fixes for EPBF fd leaks: opencontainers/cgroups#60 Let's update to it. However, the Stats() method was added in the new release to the interface, so tests were failing with: # github.com/opencontainers/runc/libcontainer [github.com/opencontainers/runc/libcontainer.test] ./container_linux_test.go:125:18: cannot use &mockCgroupManager{…} (value of type *mockCgroupManager) as cgroups.Manager value in struct literal: *mockCgroupManager does not implement cgroups.Manager (missing method Stats) Let's add the method to the mock too. In the future, probably the mock should come from the cgroups module. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
kolyshkin
pushed a commit
to rata/runc
that referenced
this pull request
Jun 22, 2026
This new version includes fixes for EPBF fd leaks: opencontainers/cgroups#60 Let's update to it. However, the Stats() method was added in the new release to the interface, so tests were failing with: # github.com/opencontainers/runc/libcontainer [github.com/opencontainers/runc/libcontainer.test] ./container_linux_test.go:125:18: cannot use &mockCgroupManager{…} (value of type *mockCgroupManager) as cgroups.Manager value in struct literal: *mockCgroupManager does not implement cgroups.Manager (missing method Stats) Let's add the method to the mock too. In the future, probably the mock should come from the cgroups module. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
kolyshkin
pushed a commit
to kolyshkin/runc
that referenced
this pull request
Jun 23, 2026
This new version includes fixes for EPBF fd leaks: opencontainers/cgroups#60 Let's update to it. However, the Stats() method was added in the new release to the interface, so tests were failing with: # github.com/opencontainers/runc/libcontainer [github.com/opencontainers/runc/libcontainer.test] ./container_linux_test.go:125:18: cannot use &mockCgroupManager{…} (value of type *mockCgroupManager) as cgroups.Manager value in struct literal: *mockCgroupManager does not implement cgroups.Manager (missing method Stats) Let's add the method to the mock too. In the future, probably the mock should come from the cgroups module. Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Runc test exclude some fds when testing fd leaks. The leaks are coming from the cgroup device BPF programs.
With this PR, runc tests pass fine and we can remove the exclude without detecting any leaks. When this is merged and released, I can update the runc PR to remove the exclude and use the new version :)
The commits explain in a little more details, but the fixes are quite simple. Please review commit by commit, as it can be tricky otherwise to understand the diff.