stage_2_pid is not yet assigned, so this kills the PID -1, but as
the sane_kill() wrapper is just a nop in that case. Just remove these
calls to kill stage_2_pid before it is cloned/assigned.
I've checked by executing the error paths that no binary is left by mistake.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
centos-9 unit test sometimes fails with:
=== RUN TestPodSkipDevicesUpdate
systemd_test.go:114: container stderr not empty: basename: missing operand
Try 'basename --help' for more information.
--- FAIL: TestPodSkipDevicesUpdate (0.11s)
I'm not sure why the container output is an error in basename. It seems
likely that the bashrc in that distro is kind of broken. Let's just run
a sleep command and forget about bash.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
In 18c4760a (libct: fixStdioPermissions: skip chown if not needed)
the check whether the STDIO file descriptors point to /dev/null was
removed which can cause /dev/null to change ownership e.g. when using
docker exec on a running container:
$ ls -l /dev/null
crw-rw-rw- 1 root root 1, 3 Aug 1 14:12 /dev/null
$ docker exec -u test 0ad6d3064e9d ls
$ ls -l /dev/null
crw-rw-rw- 1 test root 1, 3 Aug 1 14:12 /dev/null
Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
First, check if strdup() fails and error out.
While we are there, the else case was missing brackets, as we only need
to check ret in the else case. Fix that too
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
configs package can no longer be built on non-Linux OS, such as Darwin.
When running `GOOS=darwin go build` on the packge, we had the following
errors:
```
./configs/mount.go:34:16: undefined: unix.MountAttr
./configs/mount.go:47:22: undefined: unix.MS_BIND
```
Let's ensure that the linux specific bits are handled in mount_linux.go,
and introduce a _unsupported file, similar to how cgroups file is
handled within the package. This'll facilitate utilization of the pkg
for other projects that care about Darwin.
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Rewrite systemdVersionAtoi to not use regexp, and fix two issues:
1. It was returning 0 (rather than -1) for some errors.
2. The comment was saying that the input string is without quotes,
while in fact it is.
Note the new function, similar to the old one, works on input either
with or without quotes. Amend the test to add test cases without quotes.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I don't want to implement it now, because this might result in some
new issues, but this is definitely something that is worth implementing.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test (initially added by commit 58ea21daef and later amended in
commit 26dc55ef1a) currently has two major deficiencies:
1. All possible flag combinations, and their respective numeric values,
have to be explicitly listed. Currently we support 3 flags, so
there is only 2^3 - 1 = 7 combinations, but adding more flags will
become increasingly difficult (for example, 5 flags will result in
31 combinations).
2. The test requires kernel 4.17 (for SECCOMP_FILTER_FLAG_SPEC_ALLOW),
and not doing any tests when running on an older kernel. This, too,
will make it more difficult to add extra flags in the future.
Both issues can be solved by using runc features which now prints all
known and supported runc flags. We still have to hardcode the numeric
values of all flags, but most of the other work is coded now.
In particular:
* The test only uses supported flags, meaning it can be used with
older kernels, removing the limitation (2) above.
* The test calculates the powerset (all possible combinations) of
flags and their numeric values. This makes it easier to add more
flags, removing the limitation (1) above.
* The test will fail (in flags_value) if any new flags will be added
to runc but the test itself is not amended.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If no seccomps flags are set in OCI runtime spec (not even the empty
set), set SPEC_ALLOW as the default (if it's supported).
Otherwise, use the flags as they are set (that includes no flags for
empty seccomp.Flags array).
This mimics the crun behavior, and makes runc seccomp performance on par
with crun.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Amend runc features to print seccomp flags. Two set of flags are added:
* known flags are those that this version of runc is aware of;
* supported flags are those that can be set; normally, this is the same
set as known flags, but due to older version of kernel and/or
libseccomp, some known flags might be unsupported.
This commit also consolidates three different switch statements dealing
with flags into one, in func setFlag. A note is added to this function
telling what else to look for when adding new flags.
Unfortunately, it also adds a list of known flags, that should be
kept in sync with the switch statement.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Init State Error message was using the err variable instead of uerr, which has been fixed now.
The error message should not show "nil" now.
Signed-off-by: Vipul Newaskar <vipulnewaskar7@gmail.com>
This is aimed at solving the problem of cgroup v2 memory controller
behavior which is not compatible with that of cgroup v1.
In cgroup v1, if the new memory limit being set is lower than the
current usage, setting the new limit fails.
In cgroup v2, same operation succeeds, and the container is OOM killed.
Introduce a new setting, memory.checkBeforeUpdate, and use it to mimic
cgroup v1 behavior.
Note that this is not 100% reliable because of TOCTOU, but this is the
best we can do.
Add some test cases.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In findLastSyscalls, we convert libseccomp.ArchNative to the real
libseccomp architecture, but archToNative already does that, so
this code is redundant.
Remove the redundant code, and move its comment to archToNative.
Fixes: 7a8d7162f
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes libseccomp produce a BPF which uses a binary tree for
syscalls (instead of linear set of if statements).
It does not make sense to enable binary tree for small set of rules,
so don't do that if we have less than 8 syscalls (the number is chosen
arbitrarily).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Runc parses cpuset range to bits in the case of cgroup v2 + systemd as cgroup driver.
The byte order representation differs from systemd expectation, which will set
different cpuset range in systemd transient unit if the length of parsed byte array exceeds one.
# cat config.json
...
"resources": {
...
"cpu": {
"cpus": "10-23"
}
},
...
# runc --systemd-cgroup run test
# cat /run/systemd/transient/runc-test.scope.d/50-AllowedCPUs.conf
# This is a drop-in unit file extension, created via "systemctl set-property"
# or an equivalent operation. Do not edit.
[Scope]
AllowedCPUs=0-7 10-15
The cpuset.cpus in cgroup will also be set to wrong value after reloading systemd manager configuration.
# systemctl daemon-reload
# cat /sys/fs/cgroup/system.slice/runc-test.scope/cpuset.cpus
0-7,10-15
Signed-off-by: seyeongkim <seyeong.kim@canonical.com>
Signed-off-by: Chengen, Du <chengen.du@canonical.com>
This is a forward port of https://github.com/opencontainers/runc/pull/3620
The original code depended on the origin filesystem to have
/dev/{block,char} populated. This is done by udev normally and while is
very common non-containerized systemd installs, it's very easy to start
systemd in a container created by runc itself and not have
/dev/{block,char} populated. When this occurs, the following error
output is observed:
$ docker run hello-world
docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error reopening /dev/null inside container: open /dev/null: operation not permitted: unknown.
/dev/null can't be opened because it was not added to the
deviceAllowList, as there was no /dev/char directory. The change here
utilizes the fact that when sysfs in in use, there is a
/sys/dev/{block,char} that is kernel maintained that we can check.
Signed-off-by: Evan Phoenix <evan@phx.io>
It's more idiomatic Go to define interfaces on the receiver, and constructors to
return concrete types.
This patch changes various constructors to return a concrete type, with the
exceptions of NewWithPaths, which needs the abstraction as it switches between
implementations.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The v6.0.0 release of go-criu has deprecated the `rpc` package in favour
of the `crit` package. This commit provides the changes required to use
this version in runc.
Signed-off-by: Prajwal S N <prajwalnadig21@gmail.com>
Add a debug print of seccomp flags value, so the test can check
those (without using something like strace, that is).
Amend the flags setting test with the numeric values expected, and the
logic to check those.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 58ea21daef added support for seccomp flags such as
SPEC_ALLOW, but it does not work as expected, because since commit
7a8d7162f9 we do not use libseccomp-golang's Load(), but
handle flags separately in patchbfp.
This fixes setting SPEC_ALLOW flag.
Add a comment to not forget to amend filterFlags when adding new flags.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A regression reported for runc v1.1.3 says that "runc exec -t" fails
after doing "systemctl daemon-reload":
> exec failed: unable to start container process: open /dev/pts/0: operation not permitted: unknown
Apparently, with commit 7219387eb7 we are no longer adding
"DeviceAllow=char-pts rwm" rule (because os.Stat("char-pts") returns
ENOENT).
The bug can only be seen after "systemctl daemon-reload" because runc
also applies the same rules manually (by writing to devices.allow for
cgroup v1), and apparently reloading systemd leads to re-applying the
rules that systemd has (thus removing the char-pts access).
The fix is to do os.Stat only for "/dev" paths.
Also, emit a warning that the path was skipped. Since the original idea
was to emit less warnings, demote the level to debug.
Note this also fixes the issue of not adding "m" permission for block-*
and char-* devices.
A test case is added, which reliably fails before the fix
on both cgroup v1 and v2.
Fixes: https://github.com/opencontainers/runc/issues/3551
Fixes: 7219387eb7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since Go 1.19, godoc recognizes lists, code blocks, headings etc. It
also reformats the sources making it more apparent that these features
are used.
Fix a few places where it misinterpreted the formatting (such as
indented vs unindented), and format the result using the gofumpt
from HEAD, which already incorporates gofmt 1.19 changes.
Some more fixes (and enhancements) might be required.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Unless the container's runtime config has intelRdt configuration set,
any checks for whether Intel RDT is supported or the resctrl filesystem
is mounted are a waste of time as, per the OCI Runtime Spec, "the
runtime MUST NOT manipulate any resctrl pseudo-filesystems." And in the
likely case where Intel RDT is supported by both the hardware and
kernel but the resctrl filesystem is not mounted, these checks can get
expensive as the intelrdt package needs to parse mountinfo to check
whether the filesystem has been mounted to a non-standard path.
Optimize for the common case of containers with no intelRdt
configuration by only performing the checks when the container has opted
in.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The OCI runtime spec mandates "[i]f intelRdt is not set, the runtime
MUST NOT manipulate any resctrl pseudo-filesystems." Attempting to
delete files counts as manipulating, so stop doing that when the
container's RDT configuration is nil.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The intelrdt package only needs to parse mountinfo to find the mount
point of the resctrl filesystem. Users are generally going to mount the
resctrl filesystem to the pre-created /sys/fs/resctrl directory, so
there is a common case where mountinfo parsing is not required. Optimize
for the common case with a fast path which checks both for the existence
of the /sys/fs/resctrl directory and whether the resctrl filesystem was
mounted to that path using a single statfs syscall.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Reading /proc/cpuinfo is a surprisingly expensive operation. Since
kernel version 4.12 [1], opening /proc/cpuinfo on an x86 system can
block for around 20 milliseconds while the kernel samples the current
CPU frequency. There is a very recent patch [2] which gets rid of the
delay, but has yet to make it into the mainline kenel. Regardless,
kernels for which opening /proc/cpuinfo takes 20ms will continue to be
run in production for years to come. libcontainer only opens
/proc/cpuinfo to read the processor feature flags so all the delays to
get an accurate snapshot of the CPU frequency are just wasted time.
If we wanted to, we could interrogate the CPU features directly from
userspace using the `CPUID` instruction. However, Intel and AMD CPUs
have flags in different positions for their analogous sub-features and
there are CPU quirks [3] which would need to be accounted for. Some
Haswell server CPUs support RDT/CAT but are missing the `CPUID` flags
advertising their support; the kernel checks for support on that
processor family by probing the the hardware using privileged
RDMSR/WRMSR instructions [4]. This sort of probing could not be
implemented in userspace so it would not be possible to check for RDT
feature support in userspace without false negatives on some hardware
configurations.
It looks like libcontainer reads the CPU feature flags as a kind of
optimization so that it can skip checking whether the kernel supports an
RDT sub-feature if the hardware support is missing. As the kernel only
exposes subtrees in the `resctrl` filesystem for RDT sub-features with
hardware and kernel support, checking the CPU feature flags is redundant
from a correctness point of view. Remove the /proc/cpuinfo check as it
is an optimization which actually hurts performance.
[1]: https://unix.stackexchange.com/a/526679
[2]: https://lore.kernel.org/all/20220415161206.875029458@linutronix.de/
[3]: https://github.com/torvalds/linux/blob/7cf6a8a17f5b134b7e783c2d45c53298faef82a7/arch/x86/kernel/cpu/resctrl/core.c#L834-L851
[4]: https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/x86/kernel/cpu/resctrl/core.c#L111-L153
Signed-off-by: Cory Snider <csnider@mirantis.com>
When starting a new container, and the very last step of executing of a
user process fails (last lines of (*linuxStandardInit).Init), it is too
late to print a proper error since both the log pipe and the init pipe
are closed.
This is partially mitigated by using exec.LookPath() which is supposed
to say whether we will be able to execute or not. Alas, it fails to do
so when the binary to be executed resides on a filesystem mounted with
noexec flag.
A workaround would be to use access(2) with X_OK flag. Alas, it is not
working when runc itself is a setuid (or setgid) binary. In this case,
faccessat2(2) with AT_EACCESS can be used, but it is only available
since Linux v5.8.
So, use faccessat2(2) with AT_EACCESS if available. If not, fall back to
access(2) for non-setuid runc, and do nothing for setuid runc (as there
is nothing we can do). Note that this check if in addition to whatever
exec.LookPath does.
Fixes https://github.com/opencontainers/runc/issues/3520
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin