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

drop runc-dmz solution according to overlay solution #4482

Merged
merged 1 commit into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ jobs:
rootless: ["rootless", ""]
race: ["-race", ""]
criu: ["", "criu-dev"]
dmz: ["", "runc_nodmz"]
exclude:
# Disable most of criu-dev jobs, as they are expensive
# (need to compile criu) and don't add much value/coverage.
Expand All @@ -38,26 +37,12 @@ jobs:
rootless: rootless
- criu: criu-dev
race: -race
- criu: criu-dev
dmz: runc_nodmz
# Disable most of runc_nodmz jobs, as they don't add much value
# (as dmz is disabled by default anyway).
- dmz: runc_nodmz
os: ubuntu-20.04
- dmz: runc_nodmz
go-version: 1.22.x
- dmz: runc_nodmz
rootless: rootless
- dmz: runc_nodmz
race: -race
- go-version: 1.22.x
os: actuated-arm64-6cpu-8gb
- race: "-race"
os: actuated-arm64-6cpu-8gb
- criu: criu-dev
os: actuated-arm64-6cpu-8gb
- dmz: runc_nodmz
os: actuated-arm64-6cpu-8gb

runs-on: ${{ matrix.os }}

Expand Down Expand Up @@ -150,8 +135,6 @@ jobs:
check-latest: true

- name: build
env:
EXTRA_BUILDTAGS: ${{ matrix.dmz }}
run: sudo -E PATH="$PATH" make EXTRA_FLAGS="${{ matrix.race }}" all

- name: Setup Bats and bats libs
Expand All @@ -171,8 +154,6 @@ jobs:

- name: unit test
if: matrix.rootless != 'rootless'
env:
EXTRA_BUILDTAGS: ${{ matrix.dmz }}
run: sudo -E PATH="$PATH" -- make TESTFLAGS="${{ matrix.race }}" localunittest

- name: add rootless user
Expand Down Expand Up @@ -209,8 +190,6 @@ jobs:
timeout-minutes: 15
strategy:
fail-fast: false
matrix:
dmz: ["", "runc_nodmz"]
runs-on: ubuntu-22.04

steps:
Expand All @@ -234,8 +213,6 @@ jobs:
check-latest: true

- name: unit test
env:
EXTRA_BUILDTAGS: ${{ matrix.dmz }}
run: sudo -E PATH="$PATH" -- make GOARCH=386 localunittest

all-done:
Expand Down
1 change: 0 additions & 1 deletion .golangci-extra.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
run:
build-tags:
- seccomp
- runc_nodmz

linters:
disable-all: true
Expand Down
1 change: 0 additions & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
run:
build-tags:
- seccomp
- runc_nodmz

linters:
enable:
Expand Down
28 changes: 5 additions & 23 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ SHELL = /bin/bash
CONTAINER_ENGINE := docker
GO ?= go

# Get CC values for cross-compilation.
include cc_platform.mk

PREFIX ?= /usr/local
BINDIR := $(PREFIX)/sbin
MANDIR := $(PREFIX)/share/man
Expand Down Expand Up @@ -73,10 +70,10 @@ endif
.DEFAULT: runc

.PHONY: runc
runc: runc-bin verify-dmz-arch
runc: runc-bin

.PHONY: runc-bin
runc-bin: runc-dmz
runc-bin:
$(GO_BUILD) -o runc .

.PHONY: all
Expand All @@ -92,7 +89,7 @@ recvtty sd-helper seccompagent fs-idmap pidfd-kill remap-rootfs:

.PHONY: clean
clean:
rm -f runc runc-* libcontainer/dmz/binary/runc-dmz
rm -f runc runc-*
rm -f contrib/cmd/memfd-bind/memfd-bind
rm -f tests/cmd/recvtty/recvtty
rm -f tests/cmd/sd-helper/sd-helper
Expand All @@ -104,17 +101,12 @@ clean:
rm -rf man/man8

.PHONY: static
static: static-bin verify-dmz-arch
static: static-bin

.PHONY: static-bin
static-bin: runc-dmz
static-bin:
$(GO_BUILD_STATIC) -o runc .

.PHONY: runc-dmz
runc-dmz:
rm -f libcontainer/dmz/binary/runc-dmz
$(GO) generate -tags "$(BUILDTAGS)" ./libcontainer/dmz

.PHONY: releaseall
releaseall: RELEASE_ARGS := "-a 386 -a amd64 -a arm64 -a armel -a armhf -a ppc64le -a riscv64 -a s390x"
releaseall: release
Expand Down Expand Up @@ -253,16 +245,6 @@ verify-dependencies: vendor
|| (echo -e "git status:\n $$(git status -- go.mod go.sum vendor/)\nerror: vendor/, go.mod and/or go.sum not up to date. Run \"make vendor\" to update"; exit 1) \
&& echo "all vendor files are up to date."

.PHONY: verify-dmz-arch
verify-dmz-arch:
@if test -s libcontainer/dmz/binary/runc-dmz; then \
set -Eeuo pipefail; \
export LC_ALL=C; \
diff -u \
<(readelf -h runc | grep -E "(Machine|Flags):") \
<(readelf -h libcontainer/dmz/binary/runc-dmz | grep -E "(Machine|Flags):"); \
fi

.PHONY: validate-keyring
validate-keyring:
script/keyring_validate.sh
3 changes: 1 addition & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,14 @@ make BUILDTAGS=""
| Build Tag | Feature | Enabled by Default | Dependencies |
|---------------|---------------------------------------|--------------------|---------------------|
| `seccomp` | Syscall filtering using `libseccomp`. | yes | `libseccomp` |
| `!runc_nodmz` | Reduce memory usage for CVE-2019-5736 protection by using a small C binary, [see `memfd-bind` for more details][contrib-memfd-bind]. `runc_nodmz` disables this **experimental feature** and causes runc to use a different protection mechanism which will further increases memory usage temporarily during container startup. To enable this feature you also need to set the `RUNC_DMZ=true` environment variable. | yes ||

The following build tags were used earlier, but are now obsoleted:
- **runc_nodmz** (since runc v1.2.1 runc dmz binary is dropped)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Considering the amount of the changes I was wondering if this is going to be v1.3.0, but probably safe to cherrypick to v1.2.1, as dmz was experimental and opt-in)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably https://github.com/opencontainers/runc/blob/main/docs/experimental.md should be updated to reflect the history

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to drop runc-dmz now since no one is using runc_dmz (yet). Adding a backport label.

- **nokmem** (since runc v1.0.0-rc94 kernel memory settings are ignored)
- **apparmor** (since runc v1.0.0-rc93 the feature is always enabled)
- **selinux** (since runc v1.0.0-rc93 the feature is always enabled)
lifubang marked this conversation as resolved.
Show resolved Hide resolved

[contrib-memfd-bind]: /contrib/cmd/memfd-bind/README.md
[dmz README]: /libcontainer/dmz/README.md

### Running the test suite

Expand Down
61 changes: 0 additions & 61 deletions cc_platform.mk

This file was deleted.

15 changes: 3 additions & 12 deletions contrib/cmd/memfd-bind/README.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
## memfd-bind ##

`runc` normally has to make a binary copy of itself (or of a smaller helper
binary called `runc-dmz`) when constructing a container process in order to
defend against certain container runtime attacks such as CVE-2019-5736.
`runc` normally has to make a binary copy of itself when constructing a
container process in order to defend against certain container runtime attacks
such as CVE-2019-5736.
Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot to document how the new overlay mode works in #4448. I'll open a separate PR for that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is more of a discussion, so with the overlay change i guess memfd-bind is no longer needed? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being discussed in #4450 (comment). Short answer: it has fewer upsides now and there is a fairly strong case for removing it.


This cloned binary only exists until the container process starts (this means
for `runc run` and `runc exec`, it only exists for a few hundred milliseconds
Expand Down Expand Up @@ -34,15 +34,6 @@ much memory usage they can use:
* `memfd-bind` only creates a single in-memory copy of the `runc` binary (about
10MB), regardless of how many containers are running.

* `runc-dmz` is (depending on which libc it was compiled with) between 10kB and
1MB in size, and a copy is created once per process spawned inside a
container by runc (both the pid1 and every `runc exec`). The `RUNC_DMZ=true`
environment variable needs to be set to opt-in. There are circumstances where
using `runc-dmz` will fail in ways that runc cannot predict ahead of time (such
as restrictive LSMs applied to containers). `runc-dmz` also requires an
additional `execve` over the other options, though since the binary is so small
the cost is probably not even noticeable.

* The classic method of making a copy of the entire `runc` binary during
container process setup takes up about 10MB per process spawned inside the
container by runc (both pid1 and `runc exec`).
Expand Down
1 change: 1 addition & 0 deletions docs/experimental.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ Feature | Experimental release | Graduation rel
---------------------------------------- | -------------------- | ------------------
cgroup v2 | v1.0.0-rc91 | v1.0.0-rc93
The `runc features` command | v1.1.0 | v1.2.0
runc-dmz | v1.2.0-rc1 | Dropped in v1.2.1
Loading
Loading