Caught by golangci-lint when enabling golint:
libcontainer/cgroups/ebpf/ebpf.go:35:12: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead. (staticcheck)
if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
^
libcontainer/cgroups/ebpf/ebpf.go:39:13: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead. (staticcheck)
if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
^
Worth noting that we currently call prog.Detach() with unix.BPF_F_ALLOW_MULTI;
https://github.com/golang/sys/blob/22da62e12c0cd9c1da93581e1113ca4d82a5be14/unix/zerrors_linux.go#L178
BPF_F_ALLOW_MULTI = 0x2
Looking at the source code for prog.Detach(); https://github.com/cilium/ebpf/blob/v0.4.0/prog.go#L579-L581,
this would _always_ produce an error:
if flags != 0 {
return errors.New("flags must be zero")
}
Note that the flags parameter is not used (except for that validation)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Since the previous commit, some strings logged by write_log() contain a
literal newline, which leads to errors like this one:
> # time="2020-06-07T15:41:37Z" level=error msg="failed to decode \"{\\\"level\\\":\\\"debug\\\", \\\"msg\\\": \\\"nsexec-0[2265]: update /proc/2266/uid_map to '0 1000 1\\n\" to json: invalid character '\\n' in string literal"
The fix is to escape such characters.
Add a simple (as much as it can be) routine which implements JSON string
escaping as required by RFC4627, section 2.5, plus escaping of DEL (0x7f)
character (not required, but allowed by the standard, and usually done
by tools such as jq).
As much as I hate to code something like this, I was not able to find
a ready to consume and decent C implementation (not using glib).
Added a test case (and some additional asserts in C code, conditionally
enabled by the test case) to make sure the implementation is correct.
The test case have to live in a separate directory so we can use
different C flags to compile the test, and use C from go test.
[v2: try to simplify the code, add more tests]
[v3: don't do exit(1), try returning an error instead]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is somewhat radical approach to deal with kernel memory.
Per-cgroup kernel memory limiting was always problematic. A few
examples:
- older kernels had bugs and were even oopsing sometimes (best example
is RHEL7 kernel);
- kernel is unable to reclaim the kernel memory so once the limit is
hit a cgroup is toasted;
- some kernel memory allocations don't allow failing.
In addition to that,
- users don't have a clue about how to set kernel memory limits
(as the concept is much more complicated than e.g. [user] memory);
- different kernels might have different kernel memory usage,
which is sort of unexpected;
- cgroup v2 do not have a [dedicated] kmem limit knob, and thus
runc silently ignores kernel memory limits for v2;
- kernel v5.4 made cgroup v1 kmem.limit obsoleted (see
https://github.com/torvalds/linux/commit/0158115f702b).
In view of all this, and as the runtime-spec lists memory.kernel
and memory.kernelTCP as OPTIONAL, let's ignore kernel memory
limits (for cgroup v1, same as we're already doing for v2).
This should result in less bugs and better user experience.
The only bad side effect from it might be that stat can show kernel
memory usage as 0 (since the accounting is not enabled).
[v2: add a warning in specconv that limits are ignored]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In order to make 'runc --debug' actually useful for debugging nsexec
bugs, provide information about all the internal operations when in
debug mode.
[@kolyshkin: rebasing; fix formatting via indent for make validate to pass]
Signed-off-by: Aleksa Sarai <asarai@suse.de>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Alas, the EPERM on chdir saga continues...
Unfortunately, the there were two releases between when https://github.com/opencontainers/runc/commit/5e0e67d76cc99d76c8228d48f38f37034503f315 was released
and when the workaround https://github.com/opencontainers/runc/pull/2712 was added.
Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to.
Since this case was previously valid, we should continue support for it.
Now, we retry the chdir:
Once at the top of the function (to catch cases where the runc user has access, but container user does not)
and once after we setup user (to catch cases where the container user has access, and the runc user does not)
Add a test case for this as well.
Signed-off-by: Peter Hunt <pehunt@redhat.com>
Moving these utilities to a separate package, so that consumers of this
package don't have to pull in the whole "system" package.
Looking at uses of these utilities (outside of runc itself);
`RunningInUserNS()` is used by [various external consumers][1],
so adding a "Deprecated" alias for this.
[1]: https://grep.app/search?current=2&q=.RunningInUserNS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Move the unix-specific code to a file that's not compiled on
Windows.
Some of the errors (ErrUnsupported, ErrNoPasswdEntries, ErrNoGroupEntries)
are used in other parts of the code, so are moved to a non-platform
specific file.
Most of "user" is probably not useful on Windows, although it's possible
that Windows code may have to parse a passwd file, so leaving that code
for now.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit bf749516 added these two functions, but they are only used from
Windows code. The v1 of this patch moved these functions to _windows.go
file, but after some discussion we decided to drop windows code
altogether, so this is what this patch now does.
This fixes
> libcontainer/user/user.go:64:6: func `groupFromOS` is unused (unused)
> libcontainer/user/user.go:35:6: func `userFromOS` is unused (unused)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit ccdd75760c introduced the HookName type
for hooks, but only set this type on the Prestart const, but not for the
other hooks.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
c.Parent is only used by systemd cgroup drivers, and both v1 and v2
drivers do have code to set the default if it is empty, so setting
it here is redundant.
In addition, in case of cgroup v2 rootless container setting it here
is harmful as the default should be user.slice not system.slice.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The stars can be aligned in a way that results in runc to leave a stale
bind mount in container's state directory, which manifests itself later,
while trying to remove the container, in an error like this:
> remove /run/runc/test2: unlinkat /run/runc/test2/runc.W24K2t: device or resource busy
The stale mount happens because runc start/run/exec kills runc init
while it is inside ensure_cloned_binary(). One such scenario is when
a unified cgroup resource is specified for cgroup v1, a cgroup manager's
Apply returns an error (as of commit b006f4a180), and when
(*initProcess).start() kills runc init just after it was started.
One solution is NOT to kill runc init too early. To achieve that,
amend the libcontainer/nsenter code to send a \0 byte to signal
that it is past the initial setup, and make start() (for both
run/start and exec) wait for this byte before proceeding with
kill on an error path.
While at it, improve some error messages.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add some minimal validation for cgroups. The following checks
are implemented:
- cgroup name and/or prefix (or path) is set;
- for cgroup v1, unified resources are not set;
- for cgroup v2, if memorySwap is set, memory is also set,
and memorySwap > memory.
This makes some invalid configurations fail earlier (before runc init
is started), which is better.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a very simple test that checks that container.Run do not leak
opened file descriptors.
In fact it does, so we have to add two exclusions:
1. /sys/fs/cgroup is opened once per lifetime in prepareOpenat2(),
provided that cgroupv2 is used and openat2 is available. This
works as intended ("it's not a bug, it's a feature").
2. ebpf program fd is leaked every time we call setDevices() for
cgroupv2 (iow, every container.Run or container.Set leaks 1 fd).
This needs to be fixed, thus FIXME.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, the parent never closes execFifo fd. Not a problem for runc
per se, but can be an issue for a user of libcontainer.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
TestPatchHugeSeccompFilterDoesNotBlock is only testing the
disassembleFilter function. There is no need to invoke PatchAndLoad
which has the side effect of loading a seccomp profile.
Co-authored-by: Danail Branekov <danailster@gmail.com>
Co-authored-by: Kieron Browne <kbrowne@vmware.com>
Signed-off-by: Kieron Browne <kbrowne@vmware.com>
Signed-off-by: Danail Branekov <danailster@gmail.com>
There is a potential deadlock where the ExportBPF method call writes to
a pipe but the pipe is not read until after the method call returns.
ExportBPF might fill the pipe buffer, in which case it will block
waiting for a read on the other side which can't happen until the method
returns.
Here we concurrently read from the pipe into a buffer to ensure
ExportBPF will always return.
Co-authored-by: Kieron Browne <kbrowne@vmware.com>
Co-authored-by: Danail Branekov <danailster@gmail.com>
Signed-off-by: Kieron Browne <kbrowne@vmware.com>
Signed-off-by: Danail Branekov <danailster@gmail.com>
This updates handling of capabilities to match the updated runtime specification,
in https://github.com/opencontainers/runtime-spec/pull/1094.
Prior to that change, the specification required runtimes to produce a (fatal)
error if a container configuration requested capabilities that could not be
granted (either the capability is "unknown" to the runtime, not supported by the
kernel version in use, or not available in the environment that the runtime
operates in).
This caused problems in situations where the runtime was running in a restricted
environment (for example, docker-in-docker), or if there is a mismatch between
the list of capabilities known by higher-level runtimes and the OCI runtime.
Some examples:
- Kernel 5.8 introduced CAP_PERFMON, CAP_BPF, and CAP_CHECKPOINT_RESTORE
capabilities. Docker 20.10.0 ("higher level runtime") shipped with
an updated list of capabilities, and when creating a "privileged" container,
would determine what capabilities are known by the kernel in use, and request
all those capabilities (by including them in the container config).
However, runc did not yet have an updated list of capabilities, and therefore
reject the container specification, producing an error because the new
capabilities were "unknown".
- When running nested containers, for example, when running docker-in-docker,
the "inner" container may be using a more recent version of docker than the
"outer" container. In this situation, the "outer" container may be missing
capabilities that the inner container expects to be supported (based on
kernel version). However, starting the container would fail, because the OCI
runtime could not grant those capabilities (them not being available in the
environment it's running in).
WARN (but otherwise ignore) capabilities that cannot be granted
--------------------------------------------------------------------------------
This patch changes the handling to WARN (but otherwise ignore) capabilities that
are requested in the container config, but cannot be granted, alleviating higher
level runtimes to detect what capabilities are supported (by the kernel, and
in the current environment), as well as avoiding failures in situations where
the higher-level runtime is aware of capabilities that are not (yet) supported
by runc.
Impact on security
--------------------------------------------------------------------------------
Given that `capabilities` is an "allow-list", ignoring unknown capabilities does
not impose a security risk; worst case, a container does not get all requested
capabilities granted and, as a result, some actions may fail.
Backward-compatibility
--------------------------------------------------------------------------------
This change should be fully backward compatible. Higher-level runtimes that
already dynamically adjust the list of requested capabilities can continue to do
so. Runtimes that do not adjust will see an improvement (containers can start
even if some of the requested capabilities are not granted). Container processes
MAY fail (as described in "impact on security"), but users can debug this
situation either by looking at the warnings produces by the OCI runtime, or using
tools such as `capsh` / `libcap` to get the list of actual capabilities in the
container.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
First, add runc --debug exec test cases, very similar to those in
debug.bats but for runc exec (rather than runc run). Do not include json
tests as it is already tested in debug.bats.
Second, add logrus debug to late stages of runc init, and amend the
integration tests to check for those messages. This serves two purposes:
- demonstrate that runc init can be amended with debug logrus which is
properly forwarded to and logged by the parent runc create/run/exec;
- improve the chances to catch the race fixed by the previous commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes debug.bats test cases are failing like this:
> not ok 27 global --debug to --log --log-format 'json'
> # (in test file tests/integration/debug.bats, line 77)
> # `[[ "${output}" == *"child process in init()"* ]]' failed
It happens more when writing to disk.
This issue is caused by the fact that runc spawns log forwarding goroutine
(ForwardLogs) but does not wait for it to finish, resulting in missing
debug lines from nsexec.
ForwardLogs itself, though, never finishes, because it reads from a
reading side of a pipe which writing side is not closed. This is
especially true in case of runc create, which spawns runc init and
exits; meanwhile runc init waits on exec fifo for arbitrarily long
time before doing execve.
So, to fix the failure described above, we need to:
1. Make runc create/run/exec wait for ForwardLogs to finish;
2. Make runc init close its log pipe file descriptor (i.e.
the one which value is passed in _LIBCONTAINER_LOGPIPE
environment variable).
This is exactly what this commit does:
1. Amend ForwardLogs to return a channel, and wait for it in start().
2. In runc init, save the log fd and close it as late as possible.
PS I have to admit I still do not understand why an explicit close of
log pipe fd is required in e.g. (*linuxSetnsInit).Init, right before
the execve which (thanks to CLOEXEC) closes the fd anyway.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
- add check, checkWait, and finish helpers;
- move test cleanup to runLogForwarding;
- introduce and use log struct.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I have noticed that ConfigureLogs do not return an error in case logging
was already configured -- instead it just warns about it. So I went
ahead and changed the warning to the actual error...
... only to discover I broke things badly, because in case of runc init
logging is configured twice. The fix is to not configure logging in case
we are init.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Error handling is slightly cleaner this way.
While at it, do minor refactoring and fix error logging
in processEntry.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sebastiaan van Stijn