The primary change is a switch to using /proc/thread-self, which is
needed for when we add a CLONE_FS thread to runc.
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>
With open_tree(OPEN_TREE_CLONE), it is possible to implement both the
id-mapped mounts and bind-mount source file descriptor logic entirely in
Go without requiring any complicated handling from nsexec.
However, implementing it the naive way (do the OPEN_TREE_CLONE in the
host namespace before the rootfs is set up -- which is what the existing
implementation did) exposes issues in how mount ordering (in particular
when handling mount sources from inside the container rootfs, but also
in relation to mount propagation) was handled for idmapped mounts and
bind-mount sources. In order to solve this problem completely, it is
necessary to spawn a thread which joins the container mount namespace
and provides mountfds when requested by the rootfs setup code (ensuring
that the mount order and mount propagation of the source of the
bind-mount are handled correctly). While the need to join the mount
namespace leads to other complicated (such as with the usage of
/proc/self -- fixed in a later patch) the resulting code is still
reasonable and is the only real way to solve the issue.
This allows us to reduce the amount of C code we have in nsexec, as well
as simplifying a whole host of places that were made more complicated
with the addition of id-mapped mounts and the bind sourcefd logic.
Because we join the container namespace, we can continue to use regular
O_PATH file descriptors for non-id-mapped bind-mount sources (which
means we don't have to raise the kernel requirement for that case).
In addition, we can easily add support for id-mappings that don't match
the container's user namespace. The approach taken here is to use Go's
officially supported mechanism for spawning a process in a user
namespace, but (ab)use PTRACE_TRACEME to avoid actually having to exec a
different process. The most efficient way to implement this would be to
do clone() in cgo directly to run a function that just does
kill(getpid(), SIGSTOP) -- we can always switch to that if it turns out
this approach is too slow. It should be noted that the included
micro-benchmark seems to indicate this is Fast Enough(TM):
goos: linux
goarch: amd64
pkg: github.com/opencontainers/runc/libcontainer/userns
cpu: Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz
BenchmarkSpawnProc
BenchmarkSpawnProc-8 1670 770065 ns/op
Fixes: fda12ab101 ("Support idmap mounts on volumes")
Fixes: 9c444070ec ("Open bind mount sources from the host userns")
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>
For userns and timens, the mappings (and offsets, respectively) cannot
be changed after the namespace is first configured. Thus, configuring a
container with a namespace path to join means that you cannot also
provide configuration for said namespace. Previously we would silently
ignore the configuration (and just join the provided path), but we
really should be returning an error (especially when you consider that
the configuration userns mappings are used quite a bit in runc with the
assumption that they are the correct mapping for the userns -- but in
this case they are not).
In the case of userns, the mappings are also required if you _do not_
specify a path, while in the case of the time namespace you can have a
container with a timens but no mappings specified.
It should be noted that the case checking that the user has not
specified a userns path and a userns mapping needs to be handled in
specconv (as opposed to the configuration validator) because with this
patchset we now cache the mappings of path-based userns configurations
and thus the validator can't be sure whether the mapping is a cached
mapping or a user-specified one. So we do the validation in specconv,
and thus the test for this needs to be an integration test.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
If a user has misconfigured their userns mappings, they need to know
which id specifically is not mapped. There's no need to be vague.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
While we do cache the mappings when using userns paths, there's no need
to do this in this particular case, since we are in the namespace and
set[ug]id() give unambiguous EINVAL error codes if the id is unmapped.
This appears to also be the only code which does Host[UG]ID calculations
from inside "runc init".
Ref: 1a5fdc1c5f ("init: support setting -u with rootless containers")
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>
Kir Kolyshkin (8):
libct: replace runType with hasInit
libct: Signal: slight refactor
runc kill: fix sending KILL to non-pidns container
runc delete -f: fix for no pidns + no init case
libct/cg: improve cgroup removal logic
runc delete: do not ignore error from destroy
runc delete, container.Destroy: kill all processes
libct: Destroy: don't proceed in case of errors
LGTMs: lifubang cyphar
For some reason, container destroy operation removes container's state
directory even if cgroup removal fails (and then still returns an
error). It has been that way since commit 5c246d038f, which added
cgroup removal.
This is problematic because once the container state dir is removed, we
no longer know container's cgroup and thus can't remove it.
Let's return the error early and fail if cgroup can't be removed.
Same for other operations: do not proceed if we fail.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.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>
If container.Destroy() has failed, runc destroy still return 0, which is
wrong and can result in other issues down the line.
Let's always return error from destroy in runc delete.
For runc checkpoint and runc run, we still treat it as a warning.
Co-authored-by: Zhang Tianyang <burning9699@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The current code is only doing retries in RemovePaths, which is only
used for cgroup v1 (cgroup v2 uses RemovePath, which makes no retries).
Let's remove all retry logic and logging from RemovePaths, together
with:
- os.Stat check from RemovePaths (its usage probably made sense before
commit 19be8e5ba5 but not after);
- error/warning logging from RemovePaths (this was added by commit
19be8e5ba5 in 2020 and so far we've seen no errors other
than EBUSY, so reporting the actual error proved to be useless).
Add the retry logic to rmdir, and the second retry bool argument.
Decrease the initial delay and increase the number of retries from the
old implementation so it can take up to ~1 sec before returning EBUSY
(was about 0.3 sec).
Hopefully, as a result, we'll have less "failed to remove cgroup paths"
errors.
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 semantics of runType is slightly complicated, and the only place
where we need to distinguish between Created and Running is
refreshState.
Replace runType with simpler hasInit, simplifying its users (except the
refreshState, which now figures out on its own whether the container is
Created or Running).
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>
It now comes with container-selinux 2:2.224.0-1.module_el8+712+4cd1bd69,
so we only need the kludge for CentOS 7 (which, I guess, is the sole
reason why we have this kludge at all).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Because runc-dmz is not checked into Git, go get will end up creating a
copy of libcontainer/dmz with no runc-dmz binary, which causes external
libcontainer users to have compilation errors.
Unfortunately, we cannot get go:embed to just ignore that there are no
files matching the provided pattern, so instead we need to create a
dummy file that matches the go:embed (which we check into Git and so go
get _will_ copy) and switch to embed.FS.
This is a little bit uglier, but at least it will fix external
libcontainer users.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Kir Kolyshkin (3):
Add dmz-vs-selinux kludge and a way to disable it
README: fix reference to memfd-bind
tests/int: add selinux test case
LGTMs: AkihiroSuda cyphar
Bind-mounts cannot have any filesystem-specific "data" arguments,
because the kernel ignores the data argument for MS_BIND and
MS_BIND|MS_REMOUNT and we cannot safely try to override the flags
because those would affect mounts on the host (these flags affect the
superblock).
It should be noted that there are cases where the filesystem-specified
flags will also be ignored for non-bind-mounts but those are kernel
quirks and there's no real way for us to work around them. And users
wouldn't get any real benefit from us adding guardrails to existing
kernel behaviour.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Add a workaround for a problem of older container-selinux not allowing
runc to use dmz feature. If runc sees that SELinux is in enforced mode
and the container's SELinux label is set, it disables dmz.
Add a build tag, runc_dmz_selinux_nocompat, which disables the workaround.
Newer distros that ship container-selinux >= 2.224.0 (currently CentOS
Stream 8 and 9, RHEL 8 and 9, and Fedora 38+) may build runc with this
build tag set to benefit from dmz working with SELinux.
Document the build tag in the top-level and libct/dmz READMEs.
Use the build tag in our CI builds for CentOS Stream 9 and Fedora 38,
as they already has container-selinux 2.224.0 available in updates.
Add a TODO to use the build tag for CentOS Stream 8 once it has
container-selinux updated.
Signed-off-by: Kir Kolyshkin <kolyshkin@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>
CentOS 7 is showing its age and we'd rather skip some tests on it than
find out why they are flaky.
Add internal/testutil package, and move the generalized version of
SkipOnCentOS7 from libcontainer/cgroups/devices to there.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Aleksa Sarai