In some cases, container init fails to start because it is killed by
the kernel OOM killer. The errors returned by runc in such cases are
semi-random and rather cryptic. Below are a few examples.
On cgroup v1 + systemd cgroup driver:
> process_linux.go:348: copying bootstrap data to pipe caused: write init-p: broken pipe
> process_linux.go:352: getting the final child's pid from pipe caused: EOF
On cgroup v2:
> process_linux.go:495: container init caused: read init-p: connection reset by peer
> process_linux.go:484: writing syncT 'resume' caused: write init-p: broken pipe
This commits adds the OOM method to cgroup managers, which tells whether
the container was OOM-killed. In case that has happened, the original error
is discarded (unless --debug is set), and the new OOM error is reported
instead:
> ERRO[0000] container_linux.go:367: starting container process caused: container init was OOM-killed (memory limit too low?)
Also, fix the rootless test cases that are failing because they expect
an error in the first line, and we have an additional warning now:
> unable to get oom kill count" error="no directory specified for memory.oom_control
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes the code simpler and more future-proof, in case
any more values will appear in hugetlb.*.events.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Generalize the libct/getValueFromCgroup() as fscommon.GetValueByKey(),
and document it.
No changes other than using fscommon.ParseUint to convert the value.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is to benefit from openat2() implementation, on kernels
that support it. Theoretically this also improves security.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. This is the only function in the package with Get prefix
that does not read a file (but parses a string). Rename
accordingly, and convert the callers.
GetCgroupParamKeyValue -> ParseKeyValue
2. Use strings.Split rather than strings.Fields. Split by a space
is 2x faster, plus we can limit the splitting. The downside is
we have to strip a newline in one of the callers.
3. Improve the doc and the code flow.
4. Fix a test case with invalid data (spaces at BOL).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
EBUSY when trying to set memory limit may mean the new limit is too low
(lower than the current usage, and the kernel can't do anything).
Provide a more specific error for such case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Factor out setMemory and setSwap
2. Pass cgroup.Resources (rather than cgroup) to setMemoryAndSwap().
3. Merge the duplicated "set memory, set swap" case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, we read and parse 5 different files while we only need 1.
Use GetCgroupParamUint() directly to get current limit.
While at it, remove the workaround previously needed for the unit test,
and make it a bit more verbose.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
All cgroup managers has Apply() and Set() methods:
- Apply is used to create a cgroup (and, in case of systemd,
a systemd unit) and/or put a PID into the cgroup (and unit);
- Set is used to set various cgroup resources and limits.
The fs/fs2 cgroup manager implements the functionality as described above.
The systemd v1/v2 manager deviate -- it sets *most* of cgroup limits
(those that can be projected to systemd unit properties) in Apply(),
and then again *all* cgroup limits in Set (first indirectly via systemd
properties -- same as in Apply, then via cgroupfs).
This commit removes setting the cgroup limits from Apply,
so now the systemd manager behaves the same way as the fs manager.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Checking the access mode as bellow
if (R3 & bpfAccess == 0 /* use R1 as a temp var */) goto next
does not handle correctly device file probing with:
access(dev_name, F_OK)
F_OK does not trigger read or write access. Instead the access type in
R3 in that case will be zero and the check will not pass even if "rw" is
allowed for the device file. Comparing the 'masked' access type with the
requested one solves the issue:
if (R3 & bpfAccess != R3 /* use R1 as a temp var */) goto next
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
In the past we incorrectly handled eBPF errors in two ways:
1. We would only ignore errors if there was an allow rule in the list
(this doesn't make sense because for security purposes we only care
if a *deny* rule is being ignored). Arguably this is a security flaw
but you would only get an error from bpf(2) in rare cases, and thus
is not a big enough deal to go through security review.
2. If we were in a rootless container we would still return an error
even though bpf(2) is blocked for rootless containers.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
It appears that briefly thawing the cgroup while freezing
greatly increases its chances to freeze successfully.
The test case I used is doing runc exec in a look parallel with runc
pause/resume in another loop, and the failure to freeze rate reduced
from 40 to 0 per minute (tested inside a VM using a busybox container
running sleep 1h, doing about 1500 pause/resumes and 650 execs per
minute), with max retries being 150 (of 1000).
This is still a game of chances, so failures are possible.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The correct way to do that conversion according to
https://pkg.go.dev/syscall#Errno is:
```
err = nil
if errno != 0 {
err = errno
}
```
In this case the error check will always report a false positive in
unix.RawSyscall(unix.SYS_SECCOMP, ...), probably nobody has faced this
problem because the code takes the other path in most of the cases.
Fixes: 7a8d7162f9 ("seccomp: prepend -ENOSYS stub to all filters")
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
This simplifies and optimizes getting container images used for tests.
Currently, we have three different ways of getting images:
1. (for hello-world) the image is in this repo under tests/integration/testdata.
2. (for busybox) download it from github (the repo that is used for
preparing official Docker image) using curl.
3. (for debian) download from Docker hub, using skopeo and umoci.
To further complicate things, we have to do this downloading in multiple
scenarios (at least 4): locally, in github CI, from Dockefile, inside a
Vagrant VM. For each scenario, we have to install skopeo and umoci, and
those two are not yet universally available for all the distros that we
use.
Yet another complication is those images are used for tests/integration
(bats-driven tests) as well as for libcontainer/integration (go tests).
The tests in libcontainer/integration rely on busybox being available
from /busybox, and the bats tests just download the images to a
temporary location during every run.
It is also hard to support CI for other architectures, because all
the machinery for preparing images is so complicated.
This commit is an attempt to simplify and optimize getting images,
mostly by getting rid of skopeo and umoci dependencies, but also
by moving the download logic into one small shell script, which
is used from all the places.
Benefits:
- images (if not present) are only downloaded once;
- same images are used for both kind of tests (go and bats);
- same images are used for local and inside-docker tests
(because source directory is mounted into container);
- the download logic is located within 1 simple shell script.
[v2: fix eval; more doc to get-images; print URL if curl failed]
[v3: use "slim" debian, twice as small]
[v4: fix not using $image in setup_bundle]
[v5: don't remove TESTDATA from helpers.bash]
[v6: add i386 support]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Before this commit, Set() used GetState() to check the freezer state
and retry the operation if the actual state still differs from requested.
This should help with the situation when a new process (such as one
added by runc exec) is added to the container's cgroup while it's being
freezed by the kernel, but it's not working as it should.
The problem is, GetState() never returns FREEZING state, looping until
the state is either FROZEN or THAWED, so Set() does not have a chance
to repeate the freeze attempt.
As a result, the container might end up stuck in a FREEZING state,
with GetState() never returning (which in turn blocks some other
operations).
One way to fix this would be to have GetState returning FREEZING state
instead of retrying ad infinitum. It would result in changing the public
API, and no callers of GetState expects it to return this.
To fix, let's not use GetState() from Set(). Instead, read the
freezer.state file directly and act accordingly -- return success
on FROZEN, retry on FREEZING, and error out on any other (unexpected)
value.
While at it, further improve the code:
- limit the number of retries;
- if retries are exceeded, thaw and return an error;
- don't retry (or read the state back) on THAW.
I played a lot with various reproducers for this bug, including
- parallel runc execs and runc pause/resumes
- parallel runc execs and runc --systemd-cgroup update
(the latter performs freeze/unfreeze);
- continuously running /bin/printf inside container
in parallel with runc pause/resume;
- running pthread bomb (from criu test suite) in parallel
with runc pause/resume;
and I was not able to make freeze work 100%, meaning sometimes
runc pause fails, or runc --systemd-cgroup update produces a warning.
With that said, it's still a big improvement over the previous
state of affairs where container is stuck in FREEZING state,
and GetState() (and all its users) are also stuck.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This function is called by `InitSeccomp`, but only when compiled
with seccomp (and cgo) enabled, so should not be needed for other
situations.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The merge 6eed6e5795 broke the build because ab27e12ceb ("Implement
GetStat for cpuset cgroup.") dropped the errors import which was used by
c85cd2b325 ("libct/cg/fs/cpuset: don't parse mountinfo") and the CI
wasn't retriggered.
Fix this by just importing "github.com/pkg/errors" again.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Having -EPERM is the default was a fairly significant mistake from a
future-proofing standpoint in that it makes any new syscall return a
non-ignorable error (from glibc's point of view). We need to correct
this now because faccessat2(2) is something glibc critically needs to
have support for, but they're blocked on container runtimes because we
return -EPERM unconditionally (leading to confusion in glibc). This is
also a problem we're probably going to keep running into in the future.
Unfortunately there are several issues which stop us from having a clean
solution to this problem:
1. libseccomp has several limitations which require us to emulate
behaviour we want:
a. We cannot do logic based on syscall number, meaning we cannot
specify a "largest known syscall number";
b. libseccomp doesn't know in which kernel version a syscall was
added, and has no API for "minimum kernel version" so we cannot
simply ask libseccomp to generate sane -ENOSYS rules for us.
c. Additional seccomp rules for the same syscall are not treated as
distinct rules -- if rules overlap, seccomp will merge them. This
means we cannot add per-syscall -EPERM fallbacks;
d. There is no inverse operation for SCMP_CMP_MASKED_EQ;
e. libseccomp does not allow you to specify multiple rules for a
single argument, making it impossible to invert OR rules for
arguments.
2. The runtime-spec does not have any way of specifying:
a. The errno for the default action;
b. The minimum kernel version or "newest syscall at time of profile
creation"; nor
c. Which syscalls were intentionally excluded from the allow list
(weird syscalls that are no longer used were excluded entirely,
but Docker et al expect those syscalls to get EPERM not ENOSYS).
3. Certain syscalls should not return -ENOSYS (especially only for
certain argument combinations) because this could also trigger glibc
confusion. This means we have to return -EPERM for certain syscalls
but not as a global default.
4. There is not an obvious (and reasonable) upper limit to syscall
numbers, so we cannot create a set of rules for each syscall above
the largest syscall number in libseccomp. This means we must handle
inverse rules as described below.
5. Any syscall can be specified multiple times, which can make
generation of hotfix rules much harder.
As a result, we have to work around all of these things by coming up
with a heuristic to stop the bleeding. In the future we could hopefully
improve the situation in the runtime-spec and libseccomp.
The solution applied here is to prepend a "stub" filter which returns
-ENOSYS if the requested syscall has a larger syscall number than any
syscall mentioned in the filter. The reason for this specific rule is
that syscall numbers are (roughly) allocated sequentially and thus newer
syscalls will (usually) have a larger syscall number -- thus causing our
filters to produce -ENOSYS if the filter was written before the syscall
existed.
Sadly this is not a perfect solution because syscalls can be added
out-of-order and the syscall table can contain holes for several
releases. Unfortuntely we do not have a nicer solution at the moment
because there is no library which provides information about which Linux
version a syscall was introduced in. Until that exists, this workaround
will have to be good enough.
The above behaviour only happens if the default action is a blocking
action (in other words it is not SCMP_ACT_LOG or SCMP_ACT_ALLOW). If the
default action is permissive then we don't do any patching.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Kir Kolyshkin (3):
tests/int/mounts.bats: cleanup
tests/int/mount.bats: reformat
runc run: resolve tmpfs mount dest in container scope
LGTMs: @AkihiroSuda @cyphar
Closes#2715
Test that CommandHook actually executes a new process with the given env
variables, parameters and json state.
This commit also solves an issue with the previous approach that was calling
'os.Exit(0)' failing to signal test failures.
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
A recent commit a35cad3b22 added warnings about systemd being too
old. While those warnings are valid, they break some existing tests,
and also don't add much value to a user (IOW no one is going to upgrade
systemd because runc says it's old).
Demote those to warnings.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin