Commit b999376fb2 ("nsenter: cloned_binary: remove bindfd logic
entirely") removed the read-only bind-mount logic from our cloned binary
code because it wasn't really safe because a container with
CAP_SYS_ADMIN could remove the MS_RDONLY bit and get write access to
/proc/self/exe (even with user namespaces this could've been an issue
because it's not clear if the flags are locked).
However, copying a binary does seem to have a minor performance impact.
The only way to have no performance impact would be for the kernel to
block these write attempts, but barring that we could try to reduce the
overhead by coming up with a mount that cannot have it's read-only bits
cleared.
The "simplest" solution is to create a temporary overlayfs using
fsopen(2) which uses the directory where runc exists as a lowerdir,
ensuring that the container cannot access the underlying file -- and we
don't have to do any copies.
While fsopen(2) is not free because mount namespace cloning is usually
expensive (and so it seems like the difference would be marginal), some
basic performance testing seems to indicate there is a ~60% improvement
doing it this way and that it has effectively no overhead even when
compared to just using /proc/self/exe directly:
% hyperfine --warmup 50 \
> "./runc-noclone run -b bundle ctr" \
> "./runc-overlayfs run -b bundle ctr" \
> "./runc-memfd run -b bundle ctr"
Benchmark 1: ./runc-noclone run -b bundle ctr
Time (mean ± σ): 13.7 ms ± 0.9 ms [User: 6.0 ms, System: 10.9 ms]
Range (min … max): 11.3 ms … 16.1 ms 184 runs
Benchmark 2: ./runc-overlayfs run -b bundle ctr
Time (mean ± σ): 13.9 ms ± 0.9 ms [User: 6.2 ms, System: 10.8 ms]
Range (min … max): 11.8 ms … 16.0 ms 180 runs
Benchmark 3: ./runc-memfd run -b bundle ctr
Time (mean ± σ): 22.6 ms ± 1.3 ms [User: 5.7 ms, System: 20.7 ms]
Range (min … max): 19.9 ms … 26.5 ms 114 runs
Summary
./runc-noclone run -b bundle ctr ran
1.01 ± 0.09 times faster than ./runc-overlayfs run -b bundle ctr
1.65 ± 0.15 times faster than ./runc-memfd run -b bundle ctr
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This test case is frequently hanging recently. Might be caused
by a recent kernel update from 5.14.0-427.33.1.el9_4.x86_64 to
5.14.0-427.37.1.el9_4.x86_64.
Could not reproduce locally.
Let's skip it for now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The issue quoted is now fixed, so add some information about the fixed
kernel version, and remove links to older discussions about idmapped
mounts security.
We can actually remove all of it for now, but let's keep it. Change
the skip message to say which kernel is required.
Amends commit b460dc39.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 98fe566c removed setting inheritable capabilities from runc exec
--cap, but neglected to also remove ambient capabilities.
An ambient capability could only be set if the same inheritable
capability is set, so as a result of the above change ambient
capabilities were not set (but due to a bug in gocapability package,
those errors are never reported).
Once we start using a library with the fix [1], that bug will become
apparent. Alas, we do not have any tests for runc exec --cap, so add
one.
Yet, if some inheritable bits are already set from spec, let's set
ambient to avoid a possible regression. Add a test case for that, too.
[1]: https://github.com/kolyshkin/capability/pull/3
Fixes: 98fe566c ("runc: do not set inheritable capabilities")
Co-authored-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Shared pid namespace means `runc kill` (or `runc delete -f`) have to
kill all container processes, not just init. To do so, it needs a cgroup
to read the PIDs from.
If there is no cgroup, processes will be leaked, and so such
configuration is bad and should not be allowed. To keep backward
compatibility, though, let's merely warn about this for now.
Alas, the only way to know if cgroup access is available is by returning
an error from Manager.Apply. Amend fs cgroup managers to do so (systemd
doesn't need it, since v1 can't work with rootless, and cgroup v2 does
not have a special rootless case).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This aids in failed test analysis by allowing to distinguish the output
of various commands being run as part of the test case from the output
of teardown command like runc delete.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We reintroduced this once already because it is quite easy to miss this
subtle aspect of proc mounting. The recent migration to
securejoin.MkdirAllInRoot could have also inadvertently reintroduced
this (though it didn't).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
`signalAllProcesses()` depends on the cgroup and is expected to fail
when runc is running in rootless without an access to the cgroup.
When `RootlessCgroups` is set to `true`, runc just ignores the error
from `signalAllProcesses` and may leak some processes running.
(See the comments in PR 4395)
In the future, runc should walk the process tree to avoid such a leak.
Note that `RootlessCgroups` is a misnomer; it is set to `false` despite
the name when cgroup v2 delegation is configured.
This is expected to be renamed in a separate commit.
Fix issue 4394
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
The following commands are moved from `contrib/cmd` to `tests/cmd`:
- fs-idmap
- pidfd-kill
- recvtty
- remap-rootfs
- sd-helper
- seccompagent
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
1. Rename current -> got, expected -> want.
2. check_cgroup_value: add file name to output.
3. Improve functions description.
This is mostly to simplify debugging test failures.
Example output before:
current 500000 !? 500
After:
cpu.max.burst: got 500000, want 500
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There's too much logic here figuring out which CPUs to use. Runc is a
low level tool and is not supposed to be that "smart". What's worse,
this logic is executed on every exec, making it slower. Some of the
logic in (*setnsProcess).start is executed even if no annotation is set,
thus making ALL execs slow.
Also, this should be a property of a process, rather than annotation.
The plan is to rework this.
This reverts commit afc23e3397.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It has been pointed out that some controllers can not accept multiple
lines of output at once. In particular, io.max can only set one device
at a time.
Practically, the only multi-line resource values we can get come from
unified.* -- let's write those line by line.
Add a test case.
Reported-by: Tao Shen <shentaoskyking@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There are cgroup v2 systems out there that do not have cgroup swap enabled,
and this test will probably fail in there.
Move it to a separate case, guarded with requires cgroups_swap.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Add "-maxdepth 2" to not dive too deep into cgroup hierarchy.
2. Add "-type f" to look for a regular file.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test case fails when there's a single CPU. Fix this by adding
"require smp".
While at it, document the test case and add a FIXME to maybe remove
this test later.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The issue is the same as in commit 1b2adcf but for RT scheduler;
the fix is also the same.
Test case by ls-ggg.
Co-authored-by: ls-ggg <335814617@qq.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Earlier, commit fce8dd4d already increased this timeout from 1 to 5 seconds.
Yet, I just saw this timeout being hit in actuated-arm CI.
Increase the timeout again, this time from 5 to 50 (100 * 0.5) seconds.
Also, use wait_pids_gone, and improve some comments.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. A few tests use "criu check --feature" to check for a specific
feature. Let's generalize it.
2. Fix "checkpoint --pre-dump and restore" test to require memory
tracking (which is missing on ARM).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit c77aaa3f the tail of criu.log is printed by runc, so
there's no need to do the same thing in tests.
Related to 3711, 3816.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kcmp is often missing: https://man7.org/linux/man-pages/man2/kcmp.2.html
> Before Linux 5.12, this system call is available only if the
> kernel is configured with CONFIG_CHECKPOINT_RESTORE, since the
> original purpose of the system call was for the
> checkpoint/restore in user space (CRIU) feature. (The
> alternative to this system call would have been to expose
> suitable process information via the proc(5) filesystem; this was
> deemed to be unsuitable for security reasons.) Since Linux 5.12,
> this system call is also available if the kernel is configured
> with CONFIG_KCMP.
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
This handles a corner case when joining a container having all
the processes running exclusively on isolated CPU cores to force
the kernel to schedule runc process on the first CPU core within the
cgroups cpuset.
The introduction of the kernel commit
46a87b3851f0d6eb05e6d83d5c5a30df0eca8f76 has affected this deterministic
scheduling behavior by distributing tasks across CPU cores within the
cgroups cpuset. Some intensive real-time application are relying on this
deterministic behavior and use the first CPU core to run a slow thread
while other CPU cores are fully used by real-time threads with SCHED_FIFO
policy. Such applications prevents runc process from joining a container
when the runc process is randomly scheduled on a CPU core owned by a
real-time thread.
Introduces isolated CPU affinity transition OCI runtime annotation
org.opencontainers.runc.exec.isolated-cpu-affinity-transition to restore
the behavior during runc exec.
Fix issue with kernel >= 6.2 not resetting CPU affinity for container processes.
Signed-off-by: Cédric Clerget <cedric.clerget@gmail.com>
When we run this:
mount --bind -o remount,diratime,strictatime "$DIR"
It fails in debian testing, when it is the second time we call this
function in the same bats test (i.e. when $DIR is defined already).
strace shows this syscall failing:
mount_setattr(3, "", AT_EMPTY_PATH, {attr_set=MOUNT_ATTR_NOSUID|MOUNT_ATTR_NODEV|MOUNT_ATTR_NOEXEC|MOUNT_ATTR_NOATIME|MOUNT_ATTR_STRICTATIME, attr_clr=MOUNT_ATTR_RDONLY|MOUNT_ATTR_NOATIME|MOUNT_ATTR_STRICTATIME|MOUNT_ATTR_NODIRATIME|0x40, propagation=0 /* MS_??? */, userns_fd=0}, 32) = -1 EINVAL (Invalid argument)
Note it has `MOUNT_ATTR_NOATIME` and `MOUNT_ATTR_STRICTATIME` which
probably causes it to return EINVAL.
This patch simply adds atime to the options, so the mount command now
works and fixes most of the tests in debian testing.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
relatime is not shown on some debian systems. Let's check that no other
setting that removes the relatime effect is set, as that should be
enough too.
For more info, see the issue linked in the comments.
Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar>
In issue #4210, if we don't provide `--cpu-burst` in `runc update`,
the value of cpu burst will always set to 0.
Signed-off-by: lifubang <lifubang@acmcoder.com>
Now that runc-dmz is opt-in, we no longer need to try to detect whether
SELinux would cause issues for us. We can also remove the
special-purpose build-tag we added.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
If it is compiled, the user needs to opt-in with this env variable to
use it.
While we are there, remove the RUNC_DMZ=legacy as that is now the
default.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
It takes some time for the kernel to kill the process (and remove its
PID from cgroup.procs). To ensure we don't have flakes from reading
cgroup.procs right after the kill, check and wait for processes to
actually be gone.
Fixes: 4163
Reported-by: lifubang@acmcoder.com
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This expression is specific to GNU awk (gawk), so if someone has other version
of awk installed, this won't work and it's not easy to see why.
Explicitly requiring gawk here is better.
Revert "tests/int/helpers: gawk -> awk"
This reverts commit 4e65118d02.
Reported-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Our previous implementation of idmapped mounts and bind-mount sources
would open all of the source paths before we did any mounts, meaning
that mounts using sources from inside the container rootfs would not be
correct.
This has been fixed with the new on-demand system, and so add some
regression tests.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
ridmap indicates that the id mapping should be applied recursively (only
really relevant for rbind mount entries), and idmap indicates that it
should not be applied recursively (the default). If no mappings are
specified for the mount, we use the userns configuration of the
container. This matches the behaviour in the currently-unreleased
runtime-spec.
This includes a minor change to the state.json serialisation format, but
because there has been no released version of runc with commit
fbf183c6f8 ("Add uid and gid mappings to mounts"), we can safely make
this change without affecting running containers. Doing it this way
makes it much easier to handle m.IsIDMapped() and indicating that a
mapping has been specified.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Aleksa Sarai