Remove runc default devices that overlap with spec devices.#2522
Merged
cyphar merged 2 commits intoopencontainers:masterfrom Aug 19, 2020
Merged
Remove runc default devices that overlap with spec devices.#2522cyphar merged 2 commits intoopencontainers:masterfrom
cyphar merged 2 commits intoopencontainers:masterfrom
Conversation
AkihiroSuda
reviewed
Jul 21, 2020
Contributor
Author
|
Hi @AkihiroSuda, I added a test for Please take a look when you get a chance. Thanks! |
AkihiroSuda
reviewed
Jul 23, 2020
kolyshkin
reviewed
Jul 23, 2020
kolyshkin
reviewed
Jul 23, 2020
kolyshkin
reviewed
Jul 23, 2020
ctalledo
force-pushed
the
2450-fix
branch
2 times, most recently
from
a0d7304 to
7bc65e8
Compare
Contributor
Author
|
Hi @cyphar, @kolyshkin, let me know if this PR looks good to you please. Thanks! |
AkihiroSuda
previously approved these changes
Jul 29, 2020
Contributor
Author
|
Kind ping to @cyphar and @kolyshkin for approval. Thanks! |
Member
|
ping @cyphar @kolyshkin @mrunalp Let's have rc92 with this |
kolyshkin
reviewed
Aug 7, 2020
kolyshkin
reviewed
Aug 7, 2020
kolyshkin
reviewed
Aug 7, 2020
Runc has a set of default devices that it includes in Linux containers (e.g., /dev/null, /dev/random, /dev/tty, etc.) However if the container's OCI spec includes all or a subset of those same devices, runc is currently not detecting the redundancy, causing it to create a lib container config that has redundant device configurations. This causes a failure in rootless mode, in particular when the /dev/tty device has a redundant config: container_linux.go:370: starting container process caused: process_linux.go:459: container init caused: rootfs_linux.go:70: creating device nodes caused: open /tmp/busyboxtest/rootfs/dev/tty: no such device or address" The reason this fails in rootless mode only is that in this case runc sets up /dev/tty not by doing mknod (it's not allowed within a user-ns) but rather by creating a regular file under /dev/tty and bind-mounting the host's /dev/tty to the container's /dev/tty. When this operation is done redundantly, it fails the second time. This change fixes this problem by ensuring runc checks for redundant devices between the OCI spec it receives and the default devices it configures. If a redundant device is detected, the OCI spec takes priority. The change adds both a unit test and an integration test to verify the behavior. Without this fix, this new integration test fails as shown above. Signed-off-by: Cesar Talledo <ctalledo@nestybox.com>
…CI spec. Per the OCI spec, /dev/ptmx is always a symlink to /dev/pts/ptmx. As such, if the OCI spec has an explicit entry for /dev/ptmx, runc shall ignore it. This change ensures this is the case. A integration test was also added (in tests/integration/dev.bats). Signed-off-by: Cesar Talledo <ctalledo@nestybox.com>
AkihiroSuda
approved these changes
Aug 9, 2020
Member
|
ping @kolyshkin PTAL |
Contributor
Author
|
Hi @kolyshkin, would it be possible to get another review? Thanks! |
cyphar
approved these changes
Aug 19, 2020
Member
cyphar
left a comment
cyphar
left a comment
There was a problem hiding this comment.
LGTM. I think the dedup devices code could still be made simpler, but this does solve the issue.
Contributor
Author
|
Thanks @cyphar, @AkihiroSuda, @kolyshkin ! |
7 tasks
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
5 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 has a set of default devices that it includes in Linux containers
(e.g., /dev/null, /dev/random, /dev/tty, etc.)
However if the container's OCI spec includes all or a subset of those same devices,
runc is currently not detecting the redundancy, causing it to create a lib
container config that has redundant device configurations.
This causes a failure in rootless mode, in particular when the /dev/tty device
has a redundant config:
container_linux.go:370: starting container process caused: process_linux.go:459: container init caused: rootfs_linux.go:70: creating device nodes caused: open /tmp/busyboxtest/rootfs/dev/tty: no such device or address"
The reason this fails in rootless mode only is that in this case runc sets up
/dev/tty not by doing mknod (it's not allowed within a user-ns) but rather by
creating a regular file under /dev/tty and bind-mounting the host's /dev/tty to
the container's /dev/tty. When this operation is done redundantly, it fails the
second time.
This change fixes this problem by ensuring runc checks for redundant devices
between the OCI spec it receives and the default devices it configures. If
a redundant device is detected, the OCI spec takes priority.
The change adds both a unit test and an integration test to verify the
behavior. Without this fix, this new integration test fails as shown above.
Signed-off-by: Cesar Talledo ctalledo@nestybox.com