When doing a lazy checkpoint/restore, we should not restore into the
same cgroup, otherwise there is a race which result in occasional
killing of the restored container (GH #2760, #2924).
The fix is to use --manage-cgroup-mode=ignore, which allows to restore
into a different cgroup.
Note that since cgroupsPath is not set in config.json, the cgroup is
derived from the container name, so calling set_cgroups_path is not
needed.
For the previous (unsuccessful) attempt to fix this, as well as detailed
(and apparently correct) analysis, see commit 36fe3cc28c.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test checks that the container is restored into a different cgroup.
To do so, a user should
- use --manage-cgroups-mode ignore on both checkpoint and restore;
- change the cgroupsPath value in config.json before restoring.
The test does some checks to ensure that its logic is correct, and that
after the restore the old (original) cgroup does not exist, the new one
exists, and the container's init is in that new cgroup.
Signed-off-by: Kir Kolyshkin <kolyshkin@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>
On ARM, mkdirat(2) is used instead of mkdir(2), thus the seccomp rules
needs to be amended accordingly.
This is a change similar to one in commit e119db7a23, but but it
evaded the test case added in commit 58ea21dae as it took a long time to
merge, and we don't have ARM CI.
Fixes: 58ea21dae ("seccomp: add support for flags")
Reported-by: Ryan Phillips <rphillips@redhat.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test case for an issue fixed by the previous commit.
The env should has more than 8 core CPU to meet the test requirement.
Signed-off-by: Chengen, Du <chengen.du@canonical.com>
We use awk in other 9 or so places, and here it's gawk.
Since this is on Linux, most probably awk is gawk.
So s/gawk/awk/.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The situation when /sys/fs/cgroup/unified is not present normal and
should not result in anything on stderr. Suppress it.
Fixes: cc15b887a0
Signed-off-by: Kir Kolyshkin <kolyshkin@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>
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>
Recently released codespell 2.2 adds some more false positives,
such as:
./Makefile:78: ro ==> to, row, rob, rod, roe, rot
./Makefile:88: ro ==> to, row, rob, rod, roe, rot
./notify_socket.go:51: ro ==> to, row, rob, rod, roe, rot
./LICENSE:128: complies ==> compiles
./go.sum:59: BU ==> BY
./types/features/features.go:17: ro ==> to, row, rob, rod, roe, rot
./libcontainer/rootfs_linux.go:52: ro ==> to, row, rob, rod, roe, rot
./libcontainer/rootfs_linux.go:166: ro ==> to, row, rob, rod, roe, rot
....
./tests/integration/cgroup_delegation.bats:38: inh ==> in
...
To fix:
- exclude go.sum;
- add ro and complies to the list of ignored words;
- s/inh/inherit in cgroup_delegation.bats.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently only amd64 and arm64v8 tarball have been checked in testdata,
while busybox bundle is downloaded on fly, and supports multiple architectures.
To enable integration tests for more architectures, the hello world
bundle is replaced by busybox one.
Signed-off-by: Shengjing Zhu <zhsj@debian.org>
Due to a bug in commit 9c444070ec, when the user and mount namespaces
are used, and the bind mount is followed by the cgroup mount in the
spec, the cgroup is mounted using the bind mount's mount fd.
This can be reproduced with podman 4.1 (when configured to use runc):
$ podman run --uidmap 0:100:10000 quay.io/libpod/testimage:20210610 mount
Error: /home/kir/git/runc/runc: runc create failed: unable to start container process: error during container init: error mounting "cgroup" to rootfs at "/sys/fs/cgroup": mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted: OCI permission denied
or manually with the spec mounts containing something like this:
{
"destination": "/etc/resolv.conf",
"type": "bind",
"source": "/userdata/resolv.conf",
"options": [
"bind"
]
},
{
"destination": "/sys/fs/cgroup",
"type": "cgroup",
"source": "cgroup",
"options": [
"rprivate",
"nosuid",
"noexec",
"nodev",
"relatime",
"ro"
]
}
The issue was not found earlier since it requires using userns, and even then
mount fd is ignored by mountToRootfs, except for bind mounts, and all the bind
mounts have mountfd set, except for the case of cgroup v1's /sys/fs/cgroup
which is internally transformed into a bunch of bind mounts.
This is a minimal fix for the issue, suitable for backporting.
A test case is added which reproduces the issue without the fix applied.
Fixes: 9c444070ec ("Open bind mount sources from the host userns")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The variable $ROOTLESS, as set by helpers.bash and used in many places,
provides the same value as $EUID which is always set by bash. Since we
are using bash, we can rely on $EUID being omnipresent.
Modify all uses accordingly, and since the value is known to be a
number, omit the quoting.
Similarly, replace all uses of $(id -u) to $EUID.
Do some trivial cleanups along the way, such as
- simplify some if A; then B; to A && B;
- do not use [[ instead of [ where not necessary.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test requires both rootless and root, which does not make sense.
Remove the rootless part.
Fixes: d41a273da
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes it work similar to all the other variables we use as binary
flags.
The new 'shellcheck disable' is due to a bug in shellcheck (basically,
it does not track the scope of variables or execution order, assuming
everything is executed as soon as it is seen).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Strictly speaking, == is for [[ only, not for [ / test,
and, unlike =, the right side is a pattern.
To avoid confusion, use =. In cases where we compare with empty string,
use -z instead.
Keep using [[ in some cases since it does not require quoting the left
and right side of comparison (I trust shellcheck on that one).
This should have no effect (other than the code being a tad more
strict).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
While doing the previous fix, I went over all the tests in this file and
made sure they were named correctly. This patch just adds a small
sentence to clarify the intent, and does some minor improvements to some
other test names.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
There was a typo and instead of "empty" we should have used "non-empty".
Let's add a small sentence explaining the intent (like other tests in
this file) and let's highlight what we expect to happen in this test (to
ignore the listenerPath).
Fixes: #3415
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
The following failure was observed in CI (on centos-stream-8 in
integration-cgroup suite):
not ok 42 runc delete
(from function `fail' in file tests/integration/helpers.bash, line 338,
in test file tests/integration/delete.bats, line 30)
`[ "$output" = "" ] || fail "cgroup not cleaned up correctly: $output"' failed
....
cgroup not cleaned up correctly: /sys/fs/cgroup/pids/system.slice/tmp-bats\x2drun\x2d68012-runc.IPOypI-state-testbusyboxdelete-runc.zriC8C.mount
/sys/fs/cgroup/cpu,cpuacct/system.slice/tmp-bats\x2drun\x2d68012-runc.IPOypI-state-testbusyboxdelete-runc.zriC8C.mount
...
Apparently, this is a cgroup systemd creates for a mount unit which
appears then runc does internal /proc/self/exe bind-mount. The test
case should not take it into account.
The second problem with this test is it does not check that cgroup
actually exists when the container is running (so checking that it
was removed after makes less sense). For example, in rootless mode
the cgroup might not have been created.
Fix the find arguments to look for a specific cgroup name, and add
a check that these arguments are correct (i.e. the cgroup is found
when the container is running).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a way to prevent the code doing something really bad when a
variable it uses is not set. Good to have since it helps to catch some
logical errors etc.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Audit all checks for non-empty variables (i.e. ' -z ', ' -n ',
' != ""' and '= ""'), and fix those cases where a variable might be
unset. Those variables (that might not be set) are
- RUNC_USE_SYSTEMD
- BATS_RUN_TMPDIR
- AUX_UID
- AUX_DIR
- SD_PARENT_NAME
- REL_PARENT_PATH
- ROOT
- HAVE_CRIU
- ROOTLESS_FEATURES
- and a few test-specific or file-specific variables
This should allow us to enable set -u.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Add "unset ALT_ROOT" since it should not be used after teardown is
called.
2. Remove "rm -rf $ALT_ROOT". It is not needed, because ALT_ROOT is a
subdirectory of ROOT, which is removed in teardown_bundle.
3. Checking for ALT_ROOT being non-empty is a leftover from the era when
teardown() was called as the first step from setup(). Since commit
41670e21f0 this is no longer the case, so the condition
is no longer needed (plus, the `set -u` which is about to be added
should catch any possible use of unset ALT_ROOT).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Some tests (those in help.bats and version.bats) do not use setup_bundle
(as they do not need to start any containers), and thus they do not set
$ROOT. As a consequence, these tests now call "runc --root /state" which
is not nice.
Make adding --root conditional (only if $ROOT is set).
Amazingly, this change breaks help.bats tests under rootless, because
"sudo rootless" does not change the value of XDG_RUNTIME_DIR which still
points to root-owned directory, and as a result we have this:
> runc foo -h (status=1):
> the path in $XDG_RUNTIME_DIR must be writable by the user
> time="2022-02-08T07:04:57Z" level=error msg="mkdir /run/user/0/runc: permission denied"
This could be fixed by adding proper $ROOT, but it's easier just to skip
those tests under non-root.
NOTE that version.bats is not broken because -v is handled by urfave/cli
very early, so app.Before function is not run.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Older bash versions treats variable as unset if nothing has been
assigned to it. Here is an example from CentOS 7 system:
[kir@localhost ~]$ bash -u -c 'x() { local args=(); echo "${args[@]}"; }; x'
bash: args[@]: unbound variable
[kir@localhost ~]$ echo $BASH_VERSION
4.2.46(2)-release
Rewrite to work around this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. This valid warning is reported by shellcheck v0.8.0:
In tests/integration/helpers.bash line 38:
KERNEL_MINOR="${KERNEL_VERSION#$KERNEL_MAJOR.}"
^-----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
Did you mean:
KERNEL_MINOR="${KERNEL_VERSION#"$KERNEL_MAJOR".}"
Fix this.
2. These (invalid) warnings are also reported by the new version:
In tests/integration/events.bats line 13:
@test "events --stats" {
^-- SC2030 (info): Modification of status is local (to subshell caused by @bats test).
In tests/integration/events.bats line 41:
[ "$status" -eq 0 ]
^-----^ SC2031 (info): status was modified in a subshell. That change might be lost.
Basically, this is happening because shellcheck do not really track
the call tree and/or local variables. This is a known (and reported)
deficiency, and the alternative to disabling these warnings is moving
the code around, which is worse due to more changes in git history.
So we have to silence/disable these.
3. Update shellcheck to 0.8.0.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was introduced in an initial commit, back in the day when criu was
a highly experimental thing. Today it's not; most users who need it have
it packaged by their distro vendor.
The usual way to run a binary is to look it up in directories listed in
$PATH. This is flexible enough and allows for multiple scenarios (custom
binaries, extra binaries, etc.). This is the way criu should be run.
Make --criu a hidden option (thus removing it from help). Remove the
option from man pages, integration tests, etc. Remove all traces of
CriuPath from data structures.
Add a warning that --criu is ignored and will be removed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using "$@" instead of $1 in update_config() allows us to use it from
hooks.bats, where jq is used with more options than usual.
We need to disable SC2016 as otherwise shellcheck sees $something inside
single quotes and think we are losing the shell expansion (we are not).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This can be used to specify a different runc binary, for example:
sudo -E RUNC=$PWD/runc.mine tests/integration/cwd.bats
A different (but compatible enough) runtime also works:
sudo -E RUNC=/usr/local/bin/crun tests/integration/cwd.bats
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new mount option "rro" makes the mount point recursively read-only,
by calling `mount_setattr(2)` with `MOUNT_ATTR_RDONLY` and `AT_RECURSIVE`.
https://man7.org/linux/man-pages/man2/mount_setattr.2.html
Requires kernel >= 5.12.
The "rro" option string conforms to the proposal in util-linux/util-linux Issue 1501.
Fix issue 2823
Similary, this commit also adds the following mount options:
- rrw
- r[no]{suid,dev,exec,relatime,atime,strictatime,diratime,symfollow}
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Delegating cgroups to the container enables more complex workloads,
including systemd-based workloads. The OCI runtime-spec was
recently updated to explicitly admit such delegation, through
specification of cgroup ownership semantics:
https://github.com/opencontainers/runtime-spec/pull/1123
Pursuant to the updated OCI runtime-spec, change the ownership of
the container's cgroup directory and particular files therein, when
using cgroups v2 and when the cgroupfs is to be mounted read/write.
As a result of this change, systemd workloads can run in isolated
user namespaces on OpenShift when the sandbox's cgroupfs is mounted
read/write.
It might be possible to implement this feature in other cgroup
managers, but that work is deferred.
Signed-off-by: Fraser Tweedale <ftweedal@redhat.com>
As reported in [1], in a case where read-only fuse (sshfs) mount
is used as a volume without specifying ro flag, the kernel fails
to remount it (when adding various flags such as nosuid and nodev),
returning EPERM.
Here's the relevant strace line:
> [pid 333966] mount("/tmp/bats-run-PRVfWc/runc.RbNv8g/bundle/mnt", "/proc/self/fd/7", 0xc0001e9164, MS_NOSUID|MS_NODEV|MS_REMOUNT|MS_BIND|MS_REC, NULL) = -1 EPERM (Operation not permitted)
I was not able to reproduce it with other read-only mounts as the source
(tried tmpfs, read-only bind mount, and an ext2 mount), so somehow this
might be specific to fuse.
The fix is to check whether the source has RDONLY flag, and retry the
remount with this flag added.
A test case (which was kind of hard to write) is added, and it fails
without the fix. Note that rootless user need to be able to ssh to
rootless@localhost in order to sshfs to work -- amend setup scripts
to make it work, and skip the test if the setup is not working.
[1] https://github.com/containers/podman/issues/12205
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit fb4c27c4b7 (went into v1.0.0-rc93) fixed a bug with
read-only tmpfs, but introduced a bug with read-only /dev.
This happens because /dev is a tmpfs mount and is therefore remounted
read-only a bit earlier than before.
To fix,
1. Revert the part of the above commit which remounts all tmpfs mounts
as read-only in mountToRootfs.
2. Reuse finalizeRootfs (which is already used to remount /dev
read-only) to also remount all ro tmpfs mounts that were previously
mounted rw in mountPropagate.
3. Remove the break in finalizeRootfs, as now we have more than one
mount to care about.
4. Reorder the if statements in finalizeRootfs to perform the fast check
(for ro flag) first, and compare the strings second. Since /dev is
most probably also a tmpfs mount, do the m.Device check first.
Add a test case to validate the fix and prevent future regressions;
make sure it fails before the fix:
✗ runc run [ro /dev mount]
(in test file tests/integration/mounts.bats, line 45)
`[ "$status" -eq 0 ]' failed
runc spec (status=0):
runc run test_busybox (status=1):
time="2021-11-12T12:19:48-08:00" level=error msg="runc run failed: unable to start container process: error during container init: error mounting \"devpts\" to rootfs at \"/dev/pts\": mkdir /tmp/bats-run-VJXQk7/runc.0Fj70w/bundle/rootfs/dev/pts: read-only file system"
Fixes: fb4c27c4b7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes a container cgroup already exists but is frozen.
When this happens, runc init hangs, and it's not clear what is going on.
Refuse to run in a frozen cgroup; add a test case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently runc allows multiple containers to share the same cgroup (for
example, by having the same cgroupPath in config.json). While such
shared configuration might be OK, there are some issues:
- When each container has its own resource limits, the order of
containers start determines whose limits will be effectively applied.
- When one of containers is paused, all others are paused, too.
- When a container is paused, any attempt to do runc create/run/exec
end up with runc init stuck inside a frozen cgroup.
- When a systemd cgroup manager is used, this becomes even worse -- such
as, stop (or even failed start) of any container results in
"stopTransientUnit" command being sent to systemd, and so (depending on
unit properties) other containers can receive SIGTERM, be killed after a
timeout etc.
Any of the above may lead to various hard-to-debug situations in production
(runc init stuck, cgroup removal error, wrong resource limits, init not
reaping zombies etc.).
One obvious solution is to refuse a non-empty cgroup when starting a new
container. This would be a breaking change though, so let's make it in
steps, with the first step is issue a warning and a deprecated notice
about a non-empty cgroup.
Later (in runc 1.2) we will replace this warning with an error.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, if a container is paused (i.e. its cgroup is frozen), runc exec
just hangs, and it is not obvious why.
Refuse to exec in a paused container. Add a test case.
In case runc exec in a paused container is a legit use case,
add --ignore-paused option to override the check. Document it,
add a test case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Some test directories are created using mktemp -d, and so they have
permissions set to 0700 and are thus inaccessible to a user inside
userns. This was workarounded for $ROOT in userns.bats before.
Now, when we have updated Cirrus CI config to use Fedora 35 (rather than
34), userns tests fail:
> runc run failed: unable to start container process: error during
> container init: error preparing rootfs: mount
> /tmp/bats-run-4pCERd/runc.f66gCC/bundle/rootfs:/tmp/bats-run-4pCERd/runc.f66gCC/bundle/rootfs,
> flags: 0x5000: permission denied
Fedora 34 image used kernel v5.11, while Fedora 35 has v5.15.
Apparently, the newer kernel also checks that the parent directories
are accessible by the user before doing mount.
Move the old workaround from userns.bats to helpers.bats, drop the r bit
(not needed), and add $BATS_RUN_TMPDIR (also created by mktemp -d) to
fix userns.bats test failures under Fedora 35.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The misc cgroup controller, introduced in Linux 5.13, is still unknown
to systemd, and thus it cannot delegate it. Add an appropriate fixup
to the test case, similar to an earlier commit 601cf5825f.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The previous commit fixed an issue opening bind mount sources. This
commit just adds integration tests to make sure we don't regress on this
in the future.
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
Kir Kolyshkin