[1.1] Fix failed exec after systemctl daemon-reload (regression in 1.1.3) #3554
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
ff7cdd1 to
98adcb5
Compare
|
CI failure is to be fixed by #3558 Still no regression test :( |
|
Tried a build from this PR, and that fixes the issue reported in moby/moby#43960 (reply in thread) |
98adcb5 to
e6de45d
Compare
|
Thanks, but I don't see the main PR, isn't the main branch affected? |
This comment was marked as outdated.
This comment was marked as outdated.
thaJeztah
previously approved these changes
Aug 17, 2022
good point; PR for main is in #3559, but indeed still in draft; do we want the one in main first? |
This comment was marked as outdated.
This comment was marked as outdated.
e6de45d to
7054801
Compare
AkihiroSuda
approved these changes
Aug 17, 2022
This comment was marked as outdated.
This comment was marked as outdated.
7054801 to
8630d3f
Compare
AkihiroSuda
previously approved these changes
Aug 18, 2022
|
Please run git cherry-pick with -x for tracking the main commit |
That is what I usually do, although not in this case. The reasons are:
I can certainly add a reference to commit 58b1374 to the commit message. |
A regression reported for runc v1.1.3 says that "runc exec -t" fails after doing "systemctl daemon-reload": > exec failed: unable to start container process: open /dev/pts/0: operation not permitted: unknown Apparently, with commit 7219387 we are no longer adding "DeviceAllow=char-pts rwm" rule (because os.Stat("char-pts") returns ENOENT). The bug can only be seen after "systemctl daemon-reload" because runc also applies the same rules manually (by writing to devices.allow for cgroup v1), and apparently reloading systemd leads to re-applying the rules that systemd has (thus removing the char-pts access). The fix is to do os.Stat only for "/dev" paths. Also, emit a warning that the path was skipped. Since the original idea was to emit less warnings, demote the level to debug. Note this also fixes the issue of not adding "m" permission for block-* and char-* devices. A test case is added, which reliably fails before the fix on both cgroup v1 and v2. This is a backport of commit 58b1374 to release-1.1 branch. Fixes: opencontainers#3551 Fixes: 7219387 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
8630d3f to
204c673
Compare
Done; PTAL @AkihiroSuda |
AkihiroSuda
approved these changes
Aug 18, 2022
|
@kolyshkin @AkihiroSuda @cyphar should we do a |
2 tasks
This was referenced Nov 8, 2022
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.
A regression reported for runc v1.1.3 says that after systemctl
daemon-reload runc exec fails:
Apparently, with commit 7219387 we are no longer adding
"DeviceAllow=char-pts rwm" rule (because os.Stat("char-pts") returns
ENOENT).
The bug can only be seen after "systemctl daemon-reload" because runc
also applies the same rules manually (by writing to devices.allow for
cgroup v1), and apparently reloading systemd leads to re-applying the
rules that systemd has (thus removing the char-pts access).
The fix is to do os.Stat only for "/dev" paths.
Also, emit a warning that the path was skipped. Since the original idea
was to emit less warnings, demote the level to debug.
Note this also fixes the issue of not adding "m" permission for block-*
and char-* devices.
A test case is added, which reliably fails before the fix
on both cgroup v1 and v2.
Fixes: #3551
Fixes: 7219387