Commit b2f8a74d "clothed" the naked return as inflicted by gofumpt
v0.9.0. Since gofumpt v0.9.2 this rule was moved to "extra" category,
not enabled by default. The only other "extra" rule is to group adjacent
parameters with the same type, which also makes sense.
Enable gofumpt "extra" rules, and reformat the code accordingly.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Introduce and use iobail, xread, and xwrite wrappers so that we can
properly check read/write return value and call either bail or bailx on
error, with proper diagnostics (distinguishing failed read/write from a
short read/write).
This prevents the "Success" prefix in errors like:
failed to sync with stage-1: next state: Success
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a few missing sane_kill calls where they make sense.
Remove one useless sane_kill of stage2_pid, as during SYNC_USERMAP stage2
is not yet started. It is harmless yet it makes the code slightly harder
to read.
Set the child pid to -1 upon receiving SYNC_CHILD_FINISH
to minimize the chances of killing an unrelated process.
When a child sends SYNC_CHILD_FINISH it is about to exit
(although theoretically it could be stuck during debug logging).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We use bail to report fatal errors, and bail always append %m
(aka strerror(errno)). In case an error condition did not set
errno, the log message will end up with ": Success" or an error
from a stale errno value. Either case is confusing for users.
Introduce bailx which is the same as bail except it does not
append %m, and use it where appropriate.
The naming follows libc's err(3) and errx(3).
PS we still use bail in a few cases after read or write, even
if that read/write did not return an error, because the code
does not distinguish between short read/write and error (-1).
This will be addressed by the next commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since sane_kill after a failed read or write, but before reporting the
error from that read or write, it may change the errno value in case
kill(2) fails.
Save and restore the errno around the call to kill.
While at it,
- change the code to return early;
- don't return kill return value as no one is using it, and the errno
value no longer correlates.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As per
- https://github.com/opencontainers/runtime-spec/pull/1253
- https://github.com/opencontainers/runtime-spec/pull/1261
CPU affinity can be set in two ways:
1. When creating/starting a container, in config.json's
Process.ExecCPUAffinity, which is when applied to all execs.
2. When running an exec, in process.json's CPUAffinity, which
applied to a given exec and overrides the value from (1).
Add some basic tests.
Note that older kernels (RHEL8, Ubuntu 20.04) change CPU affinity of a
process to that of a container's cgroup, as soon as it is moved to that
cgroup, while newer kernels (Ubuntu 24.04, Fedora 41) don't do that.
Because of the above,
- it's impossible to really test initial CPU affinity without adding
debug logging to libcontainer/nsenter;
- for older kernels, there can be a brief moment when exec's affinity
is different than either initial or final affinity being set;
- exec's final CPU affinity, if not specified, can be different
depending on the kernel, therefore we don't test it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If we are running with privileges and are asked to join an externally
created user namespaces as well as some other namespace that was *not*
created underneath said user namespace, the approach we added in commit
2cd9c31b99 ("nsenter: guarantee correct user namespace ordering")
doesn't work.
While in theory you would want all externally created namespaces to be
sane, it seems that some tools really do create unrelated namespaces and
ask us to join them. Luckily we can just loosely copy what nsenter(1)
appears to do -- we first try to join any namespaces we can (with host
root privileges), then we join any user namespaces, and then we join any
remaining namespaces (now with the user namespace's privileges).
Note that we *do not* have to try to join namespaces after we create our
own user namespace. Namespace permissions are based purely on the owning
user namespace (not the rootuid) so we will not have access to any extra
namespaces once we unshare(CLONE_NEWUSER) (in fact we will not be able
to setns(2) to anything!).
Fixes: 2cd9c31b99 ("nsenter: guarantee correct user namespace ordering")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This is basically a no-op change because runc already disallows this,
but it will be needed in future patches when we have to track what
namespaces have already been joined.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Containerd pre-creates userns and netns before calling runc, which
results in the current code not working when SELinux is enabled,
resulting in the following error:
> runc create failed: unable to start container process: error during
container init: error mounting "mqueue" to rootfs at "/dev/mqueue":
setxattr /path/to/rootfs/dev/mqueue: operation not permitted
The solution is to become root in the user namespace right after
we join it.
Fixes#4466.
Co-authored-by: Wei Fu <fuweid89@gmail.com>
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-authored-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
This initWaiter logic was introduced by commit 4ecff8d9, but since the logic of
/proc/self/exe was moved out of runc init in commit 0e9a335, this
seems unnecessary to have initWaiter.
Remove it.
This essentially reverts commit 4ecff8d9.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Go 1.23 includes a fix (https://go.dev/cl/587919) so go1.23.x can be
used. This fix is also backported to 1.22.4, so go1.22.x can also be
used (when x >= 4). Finally, for glibc >= 2.32 it doesn't really matter.
Add a note about Go 1.22.x > 1.22.4 to README as well.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We will almost certainly need to eventually rework nsenter to:
1. Figure out a way to make pthread_self() not break after nsenter runs
(probably not possible, because the core issue is likely that we are
ignoring the rules of signal-safety(7)); or
2. Do an other re-exec of /proc/self/exe to execute the Go half of
"runc init" -- after we've done the nsenter setup. This would reset
all of the process state and ensure we have a clean glibc state for
Go, but it would make runc slower...
For now, just block Go 1.22 builds to avoid having broken runcs floating
around until we resolve the issue. It seems possible for musl to also
have an issue, but it appears to work and so for now just block glibc
builds.
Note that this will only block builds for anything that uses nsenter --
so users of our (internal) libcontainer libraries should be fine. Only
users that are starting containers using nsenter to actually start
containers will see the error (which is precisely what we want).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This was added by commit 9c444070 (to use LONG_MAX and INT_MAX) but the
code was later removed by commit ba0b5e26.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.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>
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>
This allow us to remove the amount of C code in runc quite
substantially, as well as removing a whole execve(2) from the nsexec
path because we no longer spawn "runc init" only to re-exec "runc init"
after doing the clone.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The original implementation of cgroupns had additional synchronisation
to "ensure" that the process is in the correct cgroup before unsharing
the cgroupns. This behaviour was actually never necessary, and after
commit 5110bd2fc0 ("nsenter: remove cgroupns sync mechanism") there is
no synchronisation at all, meaning that CLONE_NEWCGROUP should not get
any special treatment.
Fixes: 5110bd2fc0 ("nsenter: remove cgroupns sync mechanism")
Fixes: df3fa115f9 ("Add support for cgroup namespace")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Fix up a few things that were flagged in the review of the original
timens PR, namely around error handling and validation.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
With the new vm.memfd_noexec sysctl, we need to make sure we explicitly
request MFD_EXEC, otherwise an admin could inadvertently break
containers in a somewhat-annoying-to-debug fashion.
It should be noted that vm.memfd_noexec=2 is broken on Linux 6.4
(MFD_EXEC works even in the most restrictive mode) and the most severe
breakage is going to be fixed in Linux 6.6[1].
[1]: https://lore.kernel.org/20230705063315.3680666-2-jeffxu@google.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
While the ro-bind-mount trick did eliminate the memory overhead of
copying the runc binary for each "runc init" invocation, on machines
with very significant container churn, creating a temporary mount
namespace on every container invocation can trigger severe lock
contention on namespace_sem that makes containers fail to spawn.
The only reason we added bindfd in commit 16612d74de ("nsenter:
cloned_binary: try to ro-bind /proc/self/exe before copying") was due to
a Kubernetes e2e test failure where they had a ridiculously small memory
limit. It seems incredibly unlikely that real workloads are running
without 10MB to spare for the very short time that runc is interacting
with the container.
In addition, since the original cloned_binary implementation, cgroupv2
is now almost universally used on modern systems. Unlike cgroupv1, the
cgroupv2 memcg implementation does not migrate memory usage when
processes change cgroups (even cgroupv1 only did this if you had
memory.move_charge_at_immigrate enabled). In addition, because we do the
/proc/self/exe clone before synchronising the bootstrap data read, we
are guaranteed to do the clone before "runc init" is moved into the
container cgroup -- meaning that the memory used by the /proc/self/exe
clone is charged against the root cgroup, and thus container workloads
should not be affected at all with memfd cloning.
The long-term fix for this problem is to block the /proc/self/exe
re-opening attack entirely in-kernel, which is something I'm working
on[1]. Though it should also be noted that because the memfd is
completely separate to the host binary, even attacks like Dirty COW
against the runc binary can be defended against with the memfd approach.
Of course, once we have in-kernel protection against the /proc/self/exe
re-opening attack, we won't have that protection anymore...
[1]: https://lwn.net/Articles/934460/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
"time" namespace was introduced in Linux v5.6
support new time namespace to set boottime and monotonic time offset
Example runtime spec
"timeOffsets": {
"monotonic": {
"secs": 172800,
"nanosecs": 0
},
"boottime": {
"secs": 604800,
"nanosecs": 0
}
}
Signed-off-by: Chethan Suresh <chethan.suresh@sony.com>
While testing this with old kernel versions and kernels that don't
support idmap mounts for some of the filesystems used by a container, I
realized we can throw a more clear errors.
Let's make it clear which syscall we are using, when it is not supported
and when if the fs doesn't support idmap mounts, which path it is.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
This commit adds support for idmap mounts as specified in the runtime-spec.
We open the idmap source paths and call mount_setattr() in runc PARENT,
as we need privileges in the init userns for that, and then sends the
fds to the child process. For this fd passing we use the same mechanism
used in other parts of thecode, the _LIBCONTAINER_ env vars.
The mount is finished (unix.MoveMount) from go code, inside the userns,
so we reuse all the prepareBindMount() security checks and the remount
logic for some flags too.
This commit only supports idmap mounts when userns are used AND the mappings
are the same specified for the userns mapping. This limitation is to
simplify the initial implementation, as all our users so far only need
this, and we can avoid sending over netlink the mappings, creating a
userns with this custom mapping, etc. Future PRs will remove this
limitation.
Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We add idmap.h with the needed includes and defines in case the system
headers don't have the definition for the idmap syscalls we need.
Future patches will use these helpers.
Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Processes can watch /proc/self/mounts or /mountinfo, and the kernel
will notify them whenever the namespace's mount table is modified. The
notified process still needs to read and parse the mountinfo to
determine what changed once notified. Many such processes, including
udisksd and SystemD < v248, make no attempt to rate-limit their
mountinfo notifications. This tends to not be a problem on many systems,
where mount tables are small and mounting and unmounting is uncommon.
Every runC exec which successfully uses the try_bindfd container-escape
mitigation performs two mount()s and one umount() in the host's mount
namespace, causing any mount-watching processes to wake up and parse the
mountinfo file three times in a row. Consequently, using 'exec' health
checks on containers has a larger-than-expected impact on system load
when such mount-watching daemons are running. Furthermore, the size of
the mount table in the host's mount namespace tends to be proportional
to the number of OCI containers as a unique mount is required for the
rootfs of each container. Therefore, on systems with mount-watching
processes, the system load increases *quadratically* with the number of
running containers which use health checks!
Prevent runC from incidentally modifying the host's mount namespace for
container-escape mitigations by setting up the mitigation in a temporary
mount namespace.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Modify receive_fd() and send_fd() so they can be more readily reused in
cloned_binary.c. Change receive_fd() to have a single responsibility:
receiving and returning a single file descriptor over a UNIX domain
socket. Make send_fd() useable in precarious execution contexts such as
a clone(CLONE_VFORK|CLONE_VM) "thread" where allocating heap memory or
calling exit() would be dangerous.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Older kernels may return EINVAL on unshare when a process is reading
runc's /proc/$PID/status or /proc/$PID/maps. This was fixed by kernel
commit 12c641ab8270f ("unshare: Unsharing a thread does not require
unsharing a vm") in Linuxt v4.3.
For CentOS 7, the fix was backported to CentOS 7.7 (kernel 3.10.0-1062).
To work around this kernel bug, let's retry on EINVAL a few times.
Reported-by: zzyyzte <zhang.yu58@zte.com.cn>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
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>
Remove upper bound in integer sanity check
to not restrict the number of socket-activated
sockets passed in.
Closes#3488
Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
After adding some debug info to cloned_binary.c I found out that
is_self_cloned() is not working right when runc binary is on tmpfs,
resulting in one extra re-exec of runc.
With some added debug:
$ mkdir bin
$ sudo mount -t tmpfs tmp bin
$ sudo cp runc bin
$ sudo ./bin/runc --debug exec xxx true
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 1 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[763590]: try_bindfd: 5
DEBU[0000] nsexec[763590]: re-exec itself...
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 1 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[763590]: try_bindfd: -1
DEBU[0000] nsexec[763590]: fallback to make_execfd: 5
DEBU[0000] nsexec[763590]: re-exec itself...
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 15 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 1
From the above, it is seen that
- `is_self_cloned` returns 0,
- `try_bindfd` is called and succeeds,
- runc re-execs itself,
- the second call to `is_self_cloned` returns 0 again (because GET_SEALS returns 1),
- runc falls back to `make_execfd`, and re-execs again,
- finally, the third `is_self_cloned` returns 1.
I guess that the code relied on the following (quoting fcntl(2)):
> Currently, file seals can be applied only to a file descriptor
> returned by memfd_create(2) (if the MFD_ALLOW_SEALING was employed).
> On other filesystems, all fcntl() operations that operate on seals
> will return EINVAL.
It looks like in case of a file on tmpfs it returns 1 (F_SEAL_SEAL).
With the fix:
DEBU[0000] nsexec[768367]: => is_self_cloned
DEBU[0000] nsexec[768367]: got seals 1 (want 15)
DEBU[0000] nsexec[768367]: no CLONED_BINARY_ENV
DEBU[0000] nsexec[768367]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[768367]: try_bindfd: 5
DEBU[0000] nsexec[768367]: re-exec itself...
DEBU[0000] nsexec[768367]: => is_self_cloned
DEBU[0000] nsexec[768367]: got seals 1 (want 15)
DEBU[0000] nsexec[768367]: fstatfs says ro = 1
DEBU[0000] nsexec[768367]: fstat says nlink = 1
DEBU[0000] nsexec[768367]: <= is_self_cloned, is_cloned = 1
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 4d1d6185ab added this
nsenter_unsupported.go file in order for nsenter to be a valid (but
empty, non-functional) Go package on unsupported platforms.
As a result, runc can be build successfully without CGO, which results
in a non-working and hard-to-debug binary (see issue 3330).
As the functionality of being able to compile a package which is
definitely not working is questionable, and I can't think of any use
cases, let's remove the file.
With this, runc can no longer be build without CGO:
[kir@kir-rhat runc]$ CGO_ENABLED=0 make runc
go build -trimpath "-buildmode=pie" -tags "seccomp" -ldflags "-X main.gitCommit=v1.0.0-452-g00f56786-dirty -X main.version=1.1.0-rc.1+dev " -o runc .
go build github.com/opencontainers/runc/libcontainer/nsenter: build constraints exclude all Go files in /home/kir/go/src/github.com/opencontainers/runc/libcontainer/nsenter
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The source of the bind mount might not be accessible in a different user
namespace because a component of the source path might not be traversed
under the users and groups mapped inside the user namespace. This caused
errors such as the following:
# time="2020-06-22T13:48:26Z" level=error msg="container_linux.go:367:
starting container process caused: process_linux.go:459:
container init caused: rootfs_linux.go:58:
mounting \"/tmp/busyboxtest/source-inaccessible/dir\"
to rootfs at \"/tmp/inaccessible\" caused:
stat /tmp/busyboxtest/source-inaccessible/dir: permission denied"
To solve this problem, this patch performs the following:
1. in nsexec.c, it opens the source path in the host userns (so we have
the right permissions to open it) but in the container mntns (so the
kernel cross mntns mount check let us mount it later:
https://github.com/torvalds/linux/blob/v5.8/fs/namespace.c#L2312).
2. in nsexec.c, it passes the file descriptors of the source to the
child process with SCM_RIGHTS.
3. In runc-init in Golang, it finishes the mounts while inside the
userns even without access to the some components of the source
paths.
Passing the fds with SCM_RIGHTS is necessary because once the child
process is in the container mntns, it is already in the container userns
so it cannot temporarily join the host mntns.
This patch uses the existing mechanism with _LIBCONTAINER_* environment
variables to pass the file descriptors from runc to runc init.
This patch uses the existing mechanism with the Netlink-style bootstrap
to pass information about the list of source mounts to nsexec.c.
Rootless containers don't use this bind mount sources fdpassing
mechanism because we can't setns() to the target mntns in a rootless
container (we don't have the privileges when we are in the host userns).
This patch takes care of using O_CLOEXEC on mount fds, and close them
early.
Fixes: #2484.
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
Currently, if the log level is not set to e.g. "debug", runc init sends
some debug logs to the parent, which parses and discards it.
It is better to not send those in the first place.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin