cgroup,native: ensure we start the cleaner before creating cgroup #93
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
Some callers of bst would send it a SIGKILL as soon as the underlying operation would get canceled. The problem is that it was possible to race the cgroup initialization code of the native driver, such that the SIGKILL would be received after mkdirat of the cgroup directory, but before the cgroup cleaner has any chance at starting. This commit fixes this issue by reordering the operations. The cgroup cleaner is now started before mkdirat, and waits on a blocked pipe read until the parent process (which is the outer helper) dies. This ensures that the cleaner is started first and foremost, and that it waits until the cgroup has been initialized by the helper.
yabberyabber
reviewed
Feb 5, 2024
| switch (read(lock, &ok, sizeof (ok))) { | ||
| case -1: | ||
| warn("run_cgroup_child: read on lock"); | ||
| goto lastDitchEffort; |
There was a problem hiding this comment.
Is there really a preference for goto lastDistchEffort over return fallback_delete_cgroup ? I suppose the former keeps the scope quite a bit tighter since lastDitschEffort is only accessable from inside the function, but it feels anachronistic
There was a problem hiding this comment.
Yeah, it usually ends up being cleaner because the alternative is exfiltrating a bunch of local variables back to the parent, though in this case for one of the two goto labels, the caller has the parentfd.
It's probably one of the two legitimate uses of goto in C code. Thankfully, we have defer in Go 😛
yabberyabber
approved these changes
Feb 5, 2024
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.
Some callers of bst would send it a SIGKILL as soon as the underlying operation would get canceled. The problem is that it was possible to race the cgroup initialization code of the native driver, such that the SIGKILL would be received after mkdirat of the cgroup directory, but before the cgroup cleaner has any chance at starting.
This commit fixes this issue by reordering the operations. The cgroup cleaner is now started before mkdirat, and waits on a blocked pipe read until the parent process (which is the outer helper) dies. This ensures that the cleaner is started first and foremost, and that it waits until the cgroup has been initialized by the helper.