Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcontainer: default mount propagation correctly #1598

Merged
merged 1 commit into from
Sep 25, 2017

Conversation

euank
Copy link
Contributor

@euank euank commented Sep 22, 2017

The code in prepareRoot (

flag := unix.MS_SLAVE | unix.MS_REC
if config.RootPropagation != 0 {
flag = config.RootPropagation
}
if err := unix.Mount("", "/", "", uintptr(flag), ""); err != nil {
return err
}
)
attempts to default the rootfs mount to rslave. However, since the spec
conversion has already defaulted it to rprivate, that code doesn't
actually ever do anything.

This changes the spec conversion code to accept "" and treat it as 0.

Implicitly, this makes rootfs propagation default to rslave, which is
a part of fixing the moby bug moby/moby#34672

Alternate implementatoins include changing this defaulting to be
rslave and removing the defaulting code in prepareRoot, or skipping
the mapping entirely for "", but I think this change is the cleanest of
those options.

cc @cyphar, this is somewhat similar to #1500 and fixes a similar problem.

The code in prepareRoot (https://github.com/opencontainers/runc/blob/e385f67a0e45fa1d8ef8154e2aea5128ea1d331b/libcontainer/rootfs_linux.go#L599-L605)
attempts to default the rootfs mount to `rslave`. However, since the spec
conversion has already defaulted it to `rprivate`, that code doesn't
actually ever do anything.

This changes the spec conversion code to accept "" and treat it as 0.

Implicitly, this makes rootfs propagation default to `rslave`, which is
a part of fixing the moby bug moby/moby#34672

Alternate implementatoins include changing this defaulting to be
`rslave` and removing the defaulting code in prepareRoot, or skipping
the mapping entirely for "", but I think this change is the cleanest of
those options.

Signed-off-by: Euan Kemp <euan.kemp@coreos.com>
euank added a commit to euank/coreos-overlay that referenced this pull request Sep 22, 2017
Upstream as opencontainers/runc#1598

See discussion there and in linked issues for the full details.
euank added a commit to euank/coreos-overlay that referenced this pull request Sep 22, 2017
Upstream as opencontainers/runc#1598

See discussion there and in linked issues for the full details.
@cyphar
Copy link
Member

cyphar commented Sep 22, 2017

#1500 fixes a related but not identical race that you can hit with devicemapper.

LGTM, but there are bigger problems than just this piece of code. These issues are caused by an architectural issue with containerd (which came from Docker's design). The solution should be to make containerd spawn each runc in a separate mount namespace (with the rootfs mounts done inside the namespace) which will avoid leakage into other containers. Of course, this is not really possible at the moment due to the way the Docker/containerd split were set up.

Approved with PullApprove

@hqhq
Copy link
Contributor

hqhq commented Sep 25, 2017

LGTM

Approved with PullApprove

@hqhq hqhq merged commit 79ad714 into opencontainers:master Sep 25, 2017
@euank euank deleted the ragent branch September 25, 2017 07:02
ChrisMcKenzie pushed a commit to ChrisMcKenzie/coreos-overlay that referenced this pull request Dec 9, 2017
Upstream as opencontainers/runc#1598

See discussion there and in linked issues for the full details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants