Using host's / as the container rootfs does not work: with a private
mount namespace, pivot_root (or MS_MOVE in case of no-pivot) fails,
and without it, container mounts (such as proc, dev etc.) are mounted
on top of the host ones, effectively breaking the host.
Disallow it.
Add an integration test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When the container does not have its own mount namespace, using
--no-pivot results in msMoveRoot being run in the host mount namespace.
It unmounts host's procfs and sysfs mounts, and then moves the container
rootfs on top of the host's /, effectively breaking the host.
Similarly, rootfs propagation is applied to / and to the parent mount of
the container rootfs. Without a private mount namespace, this changes
the propagation of host mounts.
Disallow both configurations.
Add integration tests (the --no-pivot one is marked as unsafe, since
without the fix it breaks the host).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Split security into mountns and selinuxCheck, as these are unrelated.
In mountns, return early if there is a private mount namespace.
No functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The order of checks is not important (none of them modify the config),
so sort the list alphabetically, making it easier to add new entries.
The only visible effect is which error is reported first in case
the configuration has multiple issues.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was added by commit 4fd4af5b ("CI: workaround CentOS Stream 9
criu issue") back in 2022 to skip checkpoint/restore tests with broken
criu-3.17-[123].el9 packages (see #3532). These are long gone from
CentOS Stream 9 repositories, so the workaround is no longer needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The "CRIU options" and "Feature check says" debug messages, as well as
two error messages, use text marshaling of CRIU protobuf messages
(either directly via MarshalProtoText/String, or indirectly via fmt,
which uses the String method). This pulls in about 200 KB of generated
text marshaling code.
The information is not that valuable: the options are derived from
runc checkpoint/restore flags, config.json and the org.criu.config
annotation, CRIU itself logs what it does into dump.log/restore.log
(with log level 4, which runc always sets), and missing CRIU features
are reported via ErrCriuMissingFeatures.
Remove the debug messages, and only print the response type in errors.
This makes runc binary about 196 KB (1.7%) smaller on linux/amd64.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Both tests run ps in the container and check that its output contains
the container's init process (cat). When container.Run returns, though,
runc init may not yet have executed cat, so ps sometimes shows runc init
instead:
execin_test.go:63: unexpected running process, output "PID USER TIME COMMAND\n 1 root 0:00 [runc:[2:INIT]]\n 7 root 0:00 ps\n"
The race can be reliably reproduced by adding a StartContainer hook
which runs "sleep 0.2", as such hooks are run by runc init right before
executing the container's binary.
To fix, add waitExec helper, which waits for the given process to
execute a binary with the given name, and use it in both tests.
Fixes: https://github.com/opencontainers/runc/issues/4437
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Network interface statistics were only collected for "veth" networks,
but the veth strategy was removed back in 2018 by commit fe3d5c4c
("Remove unused veth setup code"). Since then, a container with a veth
network can not even be started ("unknown strategy type"), so this code
is never reached, and the network interface stats are always empty.
Remove the dead code, and deprecate libcontainer.Stats.Interfaces and
types.Stats.NetworkInterfaces, which are always empty.
Network statistics are better obtained by whoever sets up the container
network (which runc does not do).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The "defaults" mount option is present in the mountFlags table, but its
flag value is zero, and parseMountOptions only consumed the options
with a non-zero flag. As a result, "defaults" ended up in the
filesystem-specific data string, and the mount failed for filesystems
that do not accept it (such as tmpfs).
Consume all known options from mountFlags, regardless of the flag
value. The "or the flag is not supported on the platform" case is
obsolete, as modern golang.org/x/sys/unix defines all the flags.
Add a test case.
Fixes: #5500
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using net.Dial makes the DNS resolver from the net package (including
vendor/golang.org/x/net/dns/dnsmessage) reachable for the linker, since
net.Dial can be used with a host name. All the runc callers only ever
connect to a unix socket, so use net.DialUnix, which does not do any
name resolution. As a bonus, there is no need for a type assertion to
call File().
On its own, this change does not make runc binary smaller, since
godbus/dbus also uses net.Dial in its tcp transports. Once godbus gets
a way to disable those (see https://github.com/godbus/dbus/pull/444),
the two changes together make runc binary about 380 KB (3.7%) smaller.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Read-only tmpfs mounts, as well as a read-only /dev, are first mounted
read-write (as we need to modify their contents afterwards), and are only
made read-only later, by remounting them from finalizeRootfs. Same as with a
read-only rootfs, this is only done for containers having their own mount
namespace, so such mounts were silently left writable otherwise.
Refuse those configurations, too. Note that all other read-only mounts get
MS_RDONLY set right away, and thus are not affected.
To avoid the condition drifting apart between the two (now three) places
which need it, add configs.Mount.IsReadonlyDeferred and use it everywhere.
Note that filepath.Clean is used instead of pathrs.LexicallyCleanPath, as
the two only differ for relative paths (which can never be equal to "/dev"
either way), and libcontainer/configs should not depend on internal/pathrs.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Setting the umask has nothing to do with the rootfs setup, yet it was done
in finalizeRootfs, which is only called when the container has its own mount
namespace. Because of that, a container without a mount namespace ignored
process.user.umask from the spec, and did not even set the default 0o022.
Move it to standard_init, keeping the original ordering (i.e. after the
rootfs is finalized), so that it is applied unconditionally.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A read-only rootfs (root.readonly in the OCI spec, config.Readonlyfs in
libcontainer) is implemented by remounting / read-only from finalizeRootfs,
which is only called for containers having their own mount namespace (doing
it otherwise would remount the host's / read-only).
As a result, a container configured with root.readonly: true and no mount
namespace was silently started with a writable rootfs -- no error, no
warning -- leaving whoever relies on that hardening unprotected.
Fail closed instead, the same way we already do for maskedPaths and
readonlyPaths, which have the very same requirement.
Note this requires amending two tests which use a configuration with a
read-only rootfs and no mount namespace (in the same manner as they already
amend hostname, maskedPaths and readonlyPaths, for the very same reason).
Fixes#5371
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
RecvFile, SendFile, and SendRawFd were moved to an internal package
in runc 1.5, and the wrappers left in libcontainer/utils were
deprecated, with a note that they will be removed in runc 1.6.
Remove them.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This package was deprecated in runc 1.5 (in favor of
github.com/moby/sys/devices), with a note that it will be removed
in runc 1.6.
Remove it. As a result, github.com/moby/sys/devices is no longer
a runc dependency.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Now the cgroups dependency has been upgraded and it doesn't cause these
leaks anymore. Let's remove the exclude.
Signed-off-by: Rodrigo Campos <rodrigo@amutable.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Calling reflect.Value.MethodByName with a non-constant argument
prevents the Go linker from doing dead code elimination for
exported methods.
Use CriuOpts.MarshalProtoText instead, which is also more concise
as it only prints the options that are set.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Two things have to change together with the version, as neither is valid
for v2.12:
- The gofumpt extra-rules setting is deprecated in favor of the
individual extra.* options; v2.12 rejects the latter (its config
schema does not know them). Enable all three, which is what
extra-rules: true used to mean, so the formatting stays the same.
- The newer staticcheck flags the deprecated Hooks.Prestart and
LinuxMemory.Kernel fields in the specconv tests. These exercise those
fields on purpose, since runc still supports them, so silence it the
same way as in the code under test. v2.12 does not report these, so
nolintlint would call the directives unused there.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The gofumpt version bundled with golangci-lint v2.13 formats these
slightly differently. Do it now, in a separate commit, so that the
golangci-lint bump itself does not have to carry unrelated churn.
No functional change; this is the result of "golangci-lint fmt".
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Both linuxStandardInit.Init and linuxSetnsInit.Init deferred a
selinux.SetKeyLabel("") / selinux.SetExecLabel("") call to reset the
labels on return. These deferred calls are useless: on success the
function ends with execve(2) and never returns, and on failure runc init
exits right after, so there is nothing left to reset.
Worse, they are actively harmful. They run after utils.UnsafeCloseFrom,
which closes every file descriptor that must not leak into the
container, including the procfs handle cached internally by libpathrs.
Writing the label then re-opens /proc, and libpathrs aborts the whole
process:
thread '<unnamed>' (1) panicked at src/procfs.rs:349:22:
cached procfs handle should be valid: [...] Bad file descriptor
SIGABRT: abort
So a plain execve failure (a bad ELF, or an exec denied by policy) was
reported as a Rust panic plus a Go stack dump instead of the actual
error.
Drop the deferred calls, and add a test case.
Fixes#5438
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
AppArmor returns ENOENT from the write to attr/apparmor/exec when it
does not know the profile we asked for. runc passed that through as-is,
producing
unable to apply apparmor profile: apparmor failed to apply profile: write /proc/thread-self/attr/apparmor/exec: no such file or directory
which reads like a problem with the procfs file we have just written to.
Name the profile and say what the kernel means:
unable to apply apparmor profile "crio-default": profile not loaded
While at it, move the error wrapping from linuxStandardInit.Init
to applyProfile for more uniform and clear errors.
Add an integration test, along with a way to skip it where AppArmor is
not available.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
MaxCPU was documented and enforced as the highest CPU/NUMA node ID that
ToCPUSet accepts, i.e. an inclusive bound, while unix.NewCPUSet takes an
exclusive one. As a result, tryResetCPUAffinity's NewCPUSet(MaxCPU) mask
was one ID short of what ToCPUSet can parse.
This is harmless in practice, as sched_setaffinity(2) truncates the mask
to cpumask_size() anyway, and no kernel is configured with anywhere near
64K CPUs. Still, having a single constant mean two different things in
two different files is asking for a real off-by-one later on.
Make MaxCPU exclusive to match NewCPUSet, so that NewCPUSet(MaxCPU) is
correct as written, and ToCPUSet's NewCPUSet(maxID+1) can never exceed
it. This restores the "ret >= max" check and the "max-1" wording used
before MaxCPU was introduced, and shifts the test boundary cases by one.
Fixes: daf934fefc ("libct: use CPUSetDynamic for affinity and mempolicy masks")
Fixes: c55649b75e ("libct: reuse configs.MaxCPU")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since CPUAffinity.Initial/Final and LinuxMemoryPolicy.Nodes became
unix.CPUSetDynamic (a slice) rather than *unix.CPUSet (a pointer),
a nil check is no longer a complete "unset" test: a non-nil but empty
slice is now representable, and can be produced by unmarshalling a
"[]" from state.json or by a libcontainer user setting the field
directly.
Such a value used to pass the "!= nil" check and get handed to
sched_setaffinity(2) as a NULL pointer with a zero size, which the
kernel interprets as an empty mask and rejects with EINVAL, rather
than resetting the affinity as intended.
Use len() instead, which is both idiomatic for slices and treats nil
and empty alike. While at it, drop the now-redundant nil guards in the
memory policy validator (Count on a nil slice is 0).
Fixes: daf934fefc ("libct: use CPUSetDynamic for affinity and mempolicy masks")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 4d84a3403 (PR #5347) added a workaround to ignore EINVAL from
filter.GetWaitKill(), which happens when the run-time libseccomp is
older than the compile-time one and thus does not support the wait-kill
flag.
libseccomp-golang v0.12.0 handles compile-time vs. run-time version
mismatches itself, so the workaround is no longer needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These messages date back to 4776b4326 ("libcontainer: refactor syncT
handling"), which added libcontainer/sync.go. Its readSync was used by
"runc init" only, to read what its parent had sent, so saying "parent"
was accurate.
f81ef1493 ("libcontainer: sync: cleanup synchronisation code") split
readSync into doReadSync plus a few wrappers, and doReadSync now serves
both sides: the child reaches it through readSync/readSyncFull, and the
parent through parseSync. The wording was carried over unchanged, so
ever since, a read error on the parent side claims runc failed to read
from its parent, while it was in fact reading from "runc init".
Both messages are equally clear without naming a side.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check that a sub-cgroup path (as set by runc exec --cgroup, or
libcontainer's Process.SubCgroupPaths) does not escape the container's
cgroup was too lax: a plain prefix check also accepts a sibling cgroup
whose name starts with the container cgroup name (say, "../foo-suffix"
for a container cgroup named "foo").
For addIntoCgroupV1 (used for per-controller sub-cgroup paths, which
AddPid does not support) the resulting path is passed to WriteCgroupProc
as is, so the pid did end up in the sibling cgroup. For prepareCgroupFD
the subsequent cgroups.OpenFile call cleans the path, so nothing escaped
in practice, but the check was wrong nevertheless.
Use the same check as opencontainers/cgroups v0.0.9 does in AddPid, and
add test cases for both code paths.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Now that the minimum Go version is 1.26, new(expr) is available, so the
local mkPtr/ptrInt helpers are no longer needed. This is what the TODOs
next to them asked for, and the modernize linter now flags them:
update.go:22:6: newexpr: mkPtr can be an inlinable wrapper around new(expr) (modernize)
Remove all three copies of mkPtr (update.go, libcontainer/criu_linux.go,
libcontainer/integration/exec_test.go) and ptrInt
(libcontainer/integration/utils_test.go), converting their callers to
new(v). No functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Go 1.26 vet is stricter about the %q verb applied to non-rune integer
types, and rightfully complains about these two:
libcontainer/container_linux_test.go:278:53: (*testing.common).Fatalf format %q has arg state.Config.Cgroups.Memory of wrong type int64
libcontainer/container_linux_test.go:294:53: (*testing.common).Fatalf format %q has arg state.Config.Cgroups.Memory of wrong type int64
Use %d, which is what was meant here in the first place.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ToCPUSet was limited to unix.CPUSet (a fixed-size mask capped at 1024
CPUs). Switch CPUAffinity, LinuxMemoryPolicy.Nodes, ToCPUSet, and
SetMempolicy to unix.CPUSetDynamic, which can represent arbitrary CPU
and NUMA node IDs.
ToCPUSet now parses the input in a first pass to find the maximum value,
then allocates a mask large enough to hold it (out-of-range Set calls on
a dynamic mask are silently ignored). An arbitrary sanity cap is kept to
avoid huge allocations on bogus input.
This is backward compatible with reading older state.json: both the old
*unix.CPUSet (a fixed array) and the new CPUSetDynamic (a slice)
marshal to and from the same JSON array-of-numbers representation.
Update TestToCPUSet accordingly, adding coverage for values beyond the
old non-dynamic limit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Given that the majority of reviewers favor the 'nr_inodes=2' retry logic,
we propose reverting #5353 and implementing the new approach.
1. Revert "libct: add a fallback for nr_inodes=2"
This reverts commit 79ac57770f.
2. Revert "libct: Enforce nr_inodes=2 to fix Focal mount errors"
This reverts commit feea25820e.
3. The new approach:
On most kernels `nr_inodes=1` works fine. However, Ubuntu 20.04 (Focal) with
the official 5.4 kernel carries a private patch in "mm/shmem.c" that rejects
`nr_inodes<2`, so retry with `nr_inodes=2` here.
For reference, search for "case Opt_nr_inodes" in:
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/plain/mm/shmem.c?h=Ubuntu-5.4.0-216.236
Signed-off-by: lifubang <lifubang@acmcoder.com>
We don't know whether some kernels will fail with "nr_inodes=2",
so let's fall back to mount a tmpfs without "nr_inodes".
Signed-off-by: lifubang <lifubang@acmcoder.com>
On most kernels `nr_inodes=1` works fine. However, Ubuntu 20.04 (Focal) with
the official 5.4 kernel carries a private patch in mm/shmem.c that rejects
"nr_inodes<2", so let's keep `nr_inodes=2` here!
Signed-off-by: lifubang <lifubang@acmcoder.com>
