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>
With the rework of nsexec.c to handle MOUNT_ATTR_IDMAP in our Go code we
can now handle arbitrary mappings without issue, so remove the primary
artificial limit of mappings (must use the same mapping as the
container's userns) and add some tests.
We still only support idmap mounts for bind-mounts because configuring
mappings for other filesystems would require switching our entire mount
machinery to the new mount API. The current design would easily allow
for this but we would need to convert new mount options entirely to the
fsopen/fsconfig/fsmount API. This can be done in the future.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Previously, all of our userns tests worked around the remapping issue by
creating the paths that runc would attempt to create (like /proc).
However, this isn't really accurate to how real userns containers are
created, so it's much better to actually remap the rootfs.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Given we've had several bugs in this behaviour that have now been fixed,
add an integration test that makes sure that you can start a container
that joins all of the namespaces of a second container.
The only namespace we do not join is the mount namespace, because
joining a namespace that has been pivot_root'd leads to a bunch of
errors. In principle, removing everything from config.json that requires
a mount _should_ work, but the root.path configuration is mandatory and
we cannot just ignore setting up the rootfs in the namespace joining
scenario (if the user has configured a different rootfs, we need to use
it or error out, and there's no reasonable way of checking if if the
rootfs paths are the same that doesn't result in spaghetti logic).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The owner of /proc/self/timens_offsets doesn't change after creating a
userns, meaning that we need to request stage-0 to write our timens
mappings for us. Before this patch, attempting to use timens with a
proper userns resulted in:
FATA[0000] nsexec-1[18564]: failed to update /proc/self/timens_offsets: Permission denied
FATA[0000] nsexec-0[18562]: failed to sync with stage-1: next state: Success
ERRO[0000] runc run failed: unable to start container process: can't get final child's PID from pipe: EOF
Fixes: ebc2e7c435 ("Support time namespace")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Our handling for name space paths with user namespaces has been broken
for a long time. In particular, the need to parse /proc/self/*id_map in
quite a few places meant that we would treat userns configurations that
had a namespace path as if they were a userns configuration without
mappings, resulting in errors.
The primary issue was down to the id translation helper functions, which
could only handle configurations that had explicit mappings. Obviously,
when joining a user namespace we need to map the ids but figuring out
the correct mapping is non-trivial in comparison.
In order to get the mapping, you need to read /proc/<pid>/*id_map of a
process inside the userns -- while most userns paths will be of the form
/proc/<pid>/ns/user (and we have a fast-path for this case), this is not
guaranteed and thus it is necessary to spawn a process inside the
container and read its /proc/<pid>/*id_map files in the general case.
As Go does not allow us spawn a subprocess into a target userns,
we have to use CGo to fork a sub-process which does the setns(2). To be
honest, this is a little dodgy in regards to POSIX signal-safety(7) but
since we do no allocations and we are executing in the forked context
from a Go program (not a C program), it should be okay. The other
alternative would be to do an expensive re-exec (a-la nsexec which would
make several other bits of runc more complicated), or to use nsenter(1)
which might not exist on the system and is less than ideal.
Because we need to logically remap users quite a few times in runc
(including in "runc init", where joining the namespace is not feasable),
we cache the mapping inside the libcontainer config struct. A future
patch will make sure that we stop allow invalid user configurations
where a mapping is specified as well as a userns path to join.
Finally, add an integration test to make sure we don't regress this again.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
We had ignore lines for these warnings, but it turns out this is most
likely a bug in shellcheck and we can work around it by moving the
helper function definition before any of the functions that use the
helper function. Hopefully it'll be fixed soon.
Ref: https://github.com/koalaman/shellcheck/issues/2873
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(For a container with no private PID namespace, that is).
When runc delete (or container.Destroy) is called on a stopped
container without private PID namespace and there are processes
in its cgroup, kill those.
Add a test case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit f8ad20f moved the kill logic from container destroy to container
kill (which is the right thing to do).
Alas, it broke the use case of doing "runc delete -f" for a container
which does not have its own private PID namespace, when its init process
is gone. In this case, some processes may still be running, and runc
delete -f should kill them (the same way as "runc kill" does).
It does not do that because the container status is "stopped" (as runc
considers the container with no init process as stopped), and so we only
call "destroy" (which was doing the killing before).
The fix is easy: if --force is set, call killContainer no matter what.
Add a test case, similar to the one in the previous commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit f8ad20f made it impossible to kill leftover processes in a
stopped container that does not have its own PID namespace. In other
words, if a container init is gone, it is no longer possible to use
`runc kill` to kill the leftover processes.
Fix this by moving the check if container init exists to after the
special case of handling the container without own PID namespace.
While at it, fix the minor issue introduced by commit 9583b3d:
if signalAllProcesses is used, there is no need to thaw the
container (as freeze/thaw is either done in signalAllProcesses already,
or not needed at all).
Also, make signalAllProcesses return an error early if the container
cgroup does not exist (as it relies on it to do its job). This way, the
error message returned is more generic and easier to understand
("container not running" instead of "can't open file").
Finally, add a test case.
Fixes: f8ad20f
Fixes: 9583b3d
Co-authored-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The container manager like containerd-shim can't use cgroup.kill feature or
freeze all the processes in cgroup to terminate the exec init process.
It's unsafe to call kill(2) since the pid can be recycled. It's good to
provide the pidfd of init process through the pidfd-socket. It's similar to
the console-socket. With the pidfd, the container manager like containerd-shim
can send the signal to target process safely.
And for the standard init process, we can have polling support to get
exit event instead of blocking on wait4.
Signed-off-by: Wei Fu <fuweid89@gmail.com>
This is a test case to demonstrate the selinux vs dmz issue.
The issue is, runc calls selinux.SetExecLabel and then execs the
runc-dmz binary, but the execve is denied by selinux:
> type=PROCTITLE msg=audit(10/05/2023 22:54:07.911:10904) : proctitle=/tmp/bats-run-sGk2sn/runc.Ql243q/bundle/runc init
> type=SYSCALL msg=audit(10/05/2023 22:54:07.911:10904) : arch=x86_64 syscall=execveat success=no exit=EACCES(Permission denied) a0=0x6 a1=0xc0000b90fa a2=0xc0000a26a0 a3=0xc000024660 items=0 ppid=105316 pid=105327 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=pts0 ses=8 comm=runc:[2:INIT] exe=/tmp/bats-run-sGk2sn/runc.Ql243q/bundle/runc subj=unconfined_u:unconfined_r:container_runtime_t:s0-s0:c0.c1023 key=(null)
> type=AVC msg=audit(10/05/2023 22:54:07.911:10904) : avc: denied { entrypoint } for pid=105327 comm=runc:[2:INIT] path=/memfd:runc_cloned:runc-dmz (deleted) dev="tmpfs" ino=2341 scontext=system_u:system_r:container_t:s0:c4,c5 tcontext=unconfined_u:object_r:container_runtime_tmpfs_t:s0 tclass=file permissive=0
Once that error is fixed (by adding a selinux rule that enables it), we
see one more error, also related to executing a file on tmpfs.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, sometimes a short-lived "runc run" produces result with \r
and sometimes without. As a result, we have an occasional failure of
"runc run with tmpfs perms" test.
The solution (to the flaky test) is to use the first line of the output
(like many other tests do).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The original reasoning for this option was to avoid having mount options
be overwritten by runc. However, adding command-line arguments has
historically been a bad idea because it forces strict-runc-compatible
OCI runtimes to copy out-of-spec features directly from runc and these
flags are usually quite difficult to enable by users when using runc
through several layers of engines and orchestrators.
A far more preferable solution is to have a heuristic which detects
whether copying the original mount's mount options would override an
explicit mount option specified by the user. In this case, we should
return an error. You only end up in this path in the userns case, if you
have a bind-mount source with locked flags.
During the course of writing this patch, I discovered that several
aspects of our handling of flags for bind-mounts left much to be
desired. We have completely botched the handling of explicitly cleared
flags since commit 97f5ee4e6a ("Only remount if requested flags differ
from current"), with our behaviour only becoming increasingly more weird
with 50105de1d8 ("Fix failure with rw bind mount of a ro fuse") and
da780e4d27 ("Fix bind mounts of filesystems with certain options
set"). In short, we would only clear flags explicitly request by the
user purely by chance, in ways that it really should've been reported to
us by now. The most egregious is that mounts explicitly marked "rw" were
actually mounted "ro" if the bind-mount source was "ro" and no other
special flags were included. In addition, our handling of atime was
completely broken -- mostly due to how subtle the semantics of atime are
on Linux.
Unfortunately, while the runtime-spec requires us to implement
mount(8)'s behaviour, several aspects of the util-linux mount(8)'s
behaviour are broken and thus copying them makes little sense. Since the
runtime-spec behaviour for this case (should mount options for a "bind"
mount use the "mount --bind -o ..." or "mount --bind -o remount,..."
semantics? Is the fallback code we have for userns actually
spec-compliant?) and the mount(8) behaviour (see [1]) are not
well-defined, this commit simply fixes the most obvious aspects of the
behaviour that are broken while keeping the current spirit of the
implementation.
NOTE: The handling of atime in the base case is left for a future PR to
deal with. This means that the atime of the source mount will be
silently left alone unless the fallback path needs to be taken, and any
flags not explicitly set will be cleared in the base case. Whether we
should always be operating as "mount --bind -o remount,..." (where we
default to the original mount source flags) is a topic for a separate PR
and (probably) associated runtime-spec PR.
So, to resolve this:
* We store which flags were explicitly requested to be cleared by the
user, so that we can detect whether the userns fallback path would end
up setting a flag the user explicitly wished to clear. If so, we
return an error because we couldn't fulfil the configuration settings.
* Revert 97f5ee4e6a ("Only remount if requested flags differ from
current"), as missing flags do not mean we can skip MS_REMOUNT (in
fact, missing flags are how you indicate a flag needs to be cleared
with mount(2)). The original purpose of the patch was to fix the
userns issue, but as mentioned above the correct mechanism is to do a
fallback mount that copies the lockable flags from statfs(2).
* Improve handling of atime in the fallback case by:
- Correctly handling the returned flags in statfs(2).
- Implement the MNT_LOCK_ATIME checks in our code to ensure we
produce errors rather than silently producing incorrect atime
mounts.
* Improve the tests so we correctly detect all of these contingencies,
including a general "bind-mount atime handling" test to ensure that
the behaviour described here is accurate.
This change also inlines the remount() function -- it was only ever used
for the bind-mount remount case, and its behaviour is very bind-mount
specific.
[1]: https://github.com/util-linux/util-linux/issues/2433
Reverts: 97f5ee4e6a ("Only remount if requested flags differ from current")
Fixes: 50105de1d8 ("Fix failure with rw bind mount of a ro fuse")
Fixes: da780e4d27 ("Fix bind mounts of filesystems with certain options set")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Function teardown assumes that every test case will call
setup_sshfs. Currently, this assumption is true, but once
a test case that won't call setup_sshfs is added (say because
it has some extra "requires" or "skip"), it will break bats,
so any bats invocation involving such a test case will end up
hanging after the last test case is run.
The reason is, we have set -u in helpers.bash to help catching the use
of undefined variables. In the above scenario, such a variable is DIR,
which is referenced in teardown but is only defined after a call to
setup_sshfs. As a result, bash that is running the teardown function
will exit upon seeing the first $DIR, and thus teardown_bundle won't be
run. This, in turn, results in a stale recvtty process, which inherits
bats' fd 3. Until that fd is closed, bats waits for test logs.
Long story short, the fix is to
- check if DIR is set before referencing it;
- unset it after unmount.
PS it is still not clear why there is no diagnostics about the failed
teardown.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Commit e1584831b6 did two modifications to check_cgroup_value():
1. It now skips the test if the file is not found.
2. If the comparison failed, a second comparison, with value divided by 1000,
is performed.
These modifications were only needed for cpu.burst, but instead were done
in a generic function used from many cgroup tests. As a result, we can
no longer be sure about the test coverage (item 1) and the check being
correct (item 2) anymore. In fact, part of "update cgroup cpu limits"
test is currently skipped on CentOS 7 and 8 because of item 1.
To fix:
- replace item 1 with a new "cgroups_cpu_burst" argument for "requires",
and move the test to a separate case;
- replace item 2 with a local change in check_cpu_burst.
Fixes: e1584831b6
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This adds support for hugetlb.<pagesize>.rsvd limiting and accounting.
The previous non-rsvd max/limit_in_bytes does not account for reserved
huge page memory, making it possible for a processes to reserve all the
huge page memory, without being able to allocate it (due to cgroup
restrictions).
In practice this makes it possible to successfully mmap more huge page
memory than allowed via the cgroup settings, but when using the memory
the process will get a SIGBUS and crash. This is bad for applications
trying to mmap at startup (and it succeeds), but the program crashes
when starting to use the memory. eg. postgres is doing this by default.
This also keeps writing to the old max/limit_in_bytes, for backward
compatibility.
More info can be found here: https://lkml.org/lkml/2020/2/3/1153
(commit message mostly written by Odin Ugedal)
Co-authored-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Bump fileutils to v0.5.1, which fixes permissions of newly created directories
to not depend on the value of umask.
Add a test case which fails like this before the fix:
mounts.bats
✗ runc run [tmpcopyup]
(in test file tests/integration/mounts.bats, line 28)
`[[ "${lines[0]}" == *'drwxrwxrwx'* ]]' failed
runc spec (status=0):
runc run test_busybox (status=0):
drwxr-xr-x 2 root root 40 Oct 4 22:35 /dir1/dir2
Fixes 3991.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Due to the way capabilities have to be set by runc, capabilities need to
be included in the inheritable and ambient sets anyway. Otherwise, the
container process would not have the correct privileges. This test only
functioned because adding CAP_DAC_OVERRIDE to the inherited,
permissible, and bounding sets means that only "runc init" has these
capabilities -- everything other than the bounding set is cleared on the
first execve(). This breaks with runc-dmz, but the behaviour was broken
from the outset.
Docker appears to not handle this properly at all (the logic for
capability sets changed with the introduction of ambient capabilities,
and while Docker was updated it seems the behaviour is still incorrect
for non-root users).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Burstable CFS controller is introduced in Linux 5.14. This helps with
parallel workloads that might be bursty. They can get throttled even
when their average utilization is under quota. And they may be latency
sensitive at the same time so that throttling them is undesired.
This feature borrows time now against the future underrun, at the cost
of increased interference against the other system users, by introducing
cfs_burst_us into CFS bandwidth control to enact the cap on unused
bandwidth accumulation, which will then used additionally for burst.
The patch adds the support/control for CFS bandwidth burst.
runtime-spec: https://github.com/opencontainers/runtime-spec/pull/1120
Co-authored-by: Akihiro Suda <suda.kyoto@gmail.com>
Co-authored-by: Nadeshiko Manju <me@manjusaka.me>
Signed-off-by: Kailun Qin <kailun.qin@intel.com>
This file contains a test that tests .so hooks. It has a complicated
setup and teardown, and has special requirements (root and no_systemd).
Rename it to hooks_so.bats, so we can add hooks.bats for hooks tests
that do not have such complicated setup and requirements.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When a hook has failed, the error message looks like this:
> error running hook: error running hook #1: exit status 1, stdout: ...
The two problems here are:
1. it is impossible to know what kind of hook it was;
2. "error running hook" stuttering;
Change that to
> error running createContainer hook #1: exit status 1, stdout: ...
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These are not exhaustive, but at least confirm that the feature is not
obviously broken (we correctly set the time offsets).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The idea was to make them strict on dest path from the beginning for
idmap mounts, as runc would do that for all mounts in the future. But
that is causing too many problems.
For now, let's just allow relative paths for idmap mounts too. It just
seems safer.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
This test fails in CI sometimes with the following error:
> `testcontainer test_update stopped' failed
Give OOM killer some time to do its job.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently bind mounts of filesystems with nodev, nosuid, noexec,
noatime, relatime, strictatime, nodiratime options set fail in rootless
mode if the same options are not set for the bind mount.
For ro filesystems this was resolved by #2570 by remounting again
with ro set.
Follow the same approach for nodev, nosuid, noexec, noatime, relatime,
strictatime, nodiratime but allow to revert back to the old behaviour
via the new `--no-mount-fallback` command line option.
Add a testcase to verify that bind mounts of filesystems with nodev,
nosuid, noexec, noatime options set work in rootless mode.
Add a testcase that mounts a nodev, nosuid, noexec, noatime filesystem
with a ro flag.
Add two further testcases that ensure that the above testcases would
fail if the `--no-mount-fallback` command line option is set.
* contrib/completions/bash/runc:
Add `--no-mount-fallback` command line option for bash completion.
* create.go:
Add `--no-mount-fallback` command line option.
* restore.go:
Add `--no-mount-fallback` command line option.
* run.go:
Add `--no-mount-fallback` command line option.
* libcontainer/configs/config.go:
Add `NoMountFallback` field to the `Config` struct to store
the command line option value.
* libcontainer/specconv/spec_linux.go:
Add `NoMountFallback` field to the `CreateOpts` struct to store
the command line option value and store it in the libcontainer
config.
* utils_linux.go:
Store the command line option value in the `CreateOpts` struct.
* libcontainer/rootfs_linux.go:
In case that `--no-mount-fallback` is not set try to remount the
bind filesystem again with the options nodev, nosuid, noexec,
noatime, relatime, strictatime or nodiratime if they are set on
the source filesystem.
* tests/integration/mounts_sshfs.bats:
Add testcases and rework sshfs setup to allow specifying
different mount options depending on the test case.
Signed-off-by: Ruediger Pluem <ruediger.pluem@vodafone.com>
runc ps requires cgroup, but all the tests but one required root. Let's
fix this.
1. Add rootless cgroup requirement to setup() to avoid repetition.
2. Add set_cgroups_path to setup() for rootless containers because
there is no default cgroup path.
3. Modify output checks to use $output rather than $lines because in case
of rootless the first line of output contains the following warning:
> runc ps may fail if you don't have the full access to cgroups
4. While at it, move the common part of every test (creating the
container and making sure it's running) to setup().
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The passing run (with the fix) looks like this:
----
delete.bats
✓ runc delete removes failed systemd unit [4556]
runc spec (status=0):
runc run -d --console-socket /tmp/bats-run-B08vu1/runc.lbQwU5/tty/sock test-failed-unit (status=0):
Warning: The unit file, source configuration file or drop-ins of runc-cgroups-integration-test-12869.scope changed on disk. Run 'systemctl daemon-reload' to reload units.
× runc-cgroups-integration-test-12869.scope - libcontainer container integration-test-12869
Loaded: loaded (/run/systemd/transient/runc-cgroups-integration-test-12869.scope; transient)
Transient: yes
Drop-In: /run/systemd/transient/runc-cgroups-integration-test-12869.scope.d
└─50-DevicePolicy.conf, 50-DeviceAllow.conf
Active: failed (Result: timeout) since Tue 2023-06-13 14:41:38 PDT; 751ms ago
Duration: 2.144s
CPU: 8ms
Jun 13 14:41:34 kir-rhat systemd[1]: Started runc-cgroups-integration-test-12869.scope - libcontainer container integration-test-12869.
Jun 13 14:41:37 kir-rhat systemd[1]: runc-cgroups-integration-test-12869.scope: Scope reached runtime time limit. Stopping.
Jun 13 14:41:38 kir-rhat systemd[1]: runc-cgroups-integration-test-12869.scope: Stopping timed out. Killing.
Jun 13 14:41:38 kir-rhat systemd[1]: runc-cgroups-integration-test-12869.scope: Killing process 1107438 (sleep) with signal SIGKILL.
Jun 13 14:41:38 kir-rhat systemd[1]: runc-cgroups-integration-test-12869.scope: Failed with result 'timeout'.
runc delete test-failed-unit (status=0):
Unit runc-cgroups-integration-test-12869.scope could not be found.
----
Before the fix, the test was failing like this:
----
delete.bats
✗ runc delete removes failed systemd unit
(in test file tests/integration/delete.bats, line 194)
`run -4 systemctl status "$SD_UNIT_NAME"' failed, expected exit code 4, got 3
....
----
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is no such thing as linux.resources.memorySwap (the mem+swap is
set as linux.resources.memory.swap).
As it is not used in this test anyway, remove it.
Fixes: 4929c05ad1
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When a directory already exists (or after a container is restarted) the
perms of the directory being mounted to were being used even when a
different permission is set on the tmpfs mount options.
This prepends the original directory perms to the mount options.
If the perms were already set in the mount opts then those perms will
win.
This eliminates the need to perform a chmod after mount entirely.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
We read output from the following files if they exists:
- cpu.pressure
- memory.pressure
- io.pressure
Each are in format:
```
some avg10=0.00 avg60=0.00 avg300=0.00 total=0
full avg10=0.00 avg60=0.00 avg300=0.00 total=0
```
Signed-off-by: Daniel Dao <dqminh89@gmail.com>
Co-authored-by: Sandor Szücs <sandor.szuecs@zalando.de>
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Aleksa Sarai