Commit Graph

2409 Commits

Author SHA1 Message Date
lifubang 83ecd11c29 runc exec: setupRlimits after syscall.rlimit.init() completed
Issue: https://github.com/opencontainers/runc/issues/4195
Since https://go-review.googlesource.com/c/go/+/476097, there is
a get/set race between runc exec and syscall.rlimit.init, so we
need to call setupRlimits after syscall.rlimit.init() completed.

Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit a853a82677)
Signed-off-by: lifubang <lifubang@acmcoder.com>
2024-05-16 21:48:34 +08:00
Kir Kolyshkin fbddb715ed libct: fix a comment
Do not refer to the function which was removed.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit bac506463d)
Signed-off-by: lifubang <lifubang@acmcoder.com>
2024-05-16 21:48:34 +08:00
Kir Kolyshkin debf52aa5b deprecate libct.system.Execv
This is not used since commit dac41717.
It will be removed in v1.2.0

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit dbd0c3349f)
Signed-off-by: lifubang <lifubang@acmcoder.com>
2024-05-16 21:47:26 +08:00
lfbzhm 6379b58d97 libcontainer: force apps to think fips is enabled/disabled for testing
The motivation behind this change is to provide a flexible mechanism for
containers within a Kubernetes cluster to opt out of FIPS mode when necessary.
This change enables apps to simulate FIPS mode being enabled or disabled for testing
purposes. Users can control whether apps believe FIPS mode is on or off by manipulating
`/proc/sys/crypto/fips_enabled`.

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
2024-04-26 11:23:49 -04:00
Aleksa Sarai e9665f4d60 init: don't special-case logrus fds
We close the logfd before execve so there's no need to special case it.
In addition, it turns out that (*os.File).Fd() doesn't handle the case
where the file was closed and so it seems suspect to use that kind of
check.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:59 +11:00
Aleksa Sarai 683ad2ff3b libcontainer: mark all non-stdio fds O_CLOEXEC before spawning init
Given the core issue in GHSA-xr7r-f8xq-vfvv was that we were unknowingly
leaking file descriptors to "runc init", it seems prudent to make sure
we proactively prevent this in the future. The solution is to simply
mark all non-stdio file descriptors as O_CLOEXEC before we spawn "runc
init".

For libcontainer library users, this could result in unrelated files
being marked as O_CLOEXEC -- however (for the same reason we are doing
this for runc), for security reasons those files should've been marked
as O_CLOEXEC anyway.

Fixes: GHSA-xr7r-f8xq-vfvv CVE-2024-21626
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:59 +11:00
Aleksa Sarai b6633f48a8 cgroup: plug leaks of /sys/fs/cgroup handle
We auto-close this file descriptor in the final exec step, but it's
probably a good idea to not possibly leak the file descriptor to "runc
init" (we've had issues like this in the past) especially since it is a
directory handle from the host mount namespace.

In practice, on runc 1.1 this does leak to "runc init" but on main the
handle has a low enough file descriptor that it gets clobbered by the
ForkExec of "runc init".

OPEN_TREE_CLONE would let us protect this handle even further, but the
performance impact of creating an anonymous mount namespace is probably
not worth it.

Also, switch to using an *os.File for the handle so if it goes out of
scope during setup (i.e. an error occurs during setup) it will get
cleaned up by the GC.

Fixes: GHSA-xr7r-f8xq-vfvv CVE-2024-21626
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:58 +11:00
Aleksa Sarai 284ba3057e init: close internal fds before execve
If we leak a file descriptor referencing the host filesystem, an
attacker could use a /proc/self/fd magic-link as the source for execve
to execute a host binary in the container. This would allow the binary
itself (or a process inside the container in the 'runc exec' case) to
write to a host binary, leading to a container escape.

The simple solution is to make sure we close all file descriptors
immediately before the execve(2) step. Doing this earlier can lead to very
serious issues in Go (as file descriptors can be reused, any (*os.File)
reference could start silently operating on a different file) so we have
to do it as late as possible.

Unfortunately, there are some Go runtime file descriptors that we must
not close (otherwise the Go scheduler panics randomly). The only way of
being sure which file descriptors cannot be closed is to sneakily
go:linkname the runtime internal "internal/poll.IsPollDescriptor"
function. This is almost certainly not recommended but there isn't any
other way to be absolutely sure, while also closing any other possible
files.

In addition, we can keep the logrus forwarding logfd open because you
cannot execve a pipe and the contents of the pipe are so restricted
(JSON-encoded in a format we pick) that it seems unlikely you could even
construct shellcode. Closing the logfd causes issues if there is an
error returned from execve.

In mainline runc, runc-dmz protects us against this attack because the
intermediate execve(2) closes all of the O_CLOEXEC internal runc file
descriptors and thus runc-dmz cannot access them to attack the host.

Fixes: GHSA-xr7r-f8xq-vfvv CVE-2024-21626
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:58 +11:00
Aleksa Sarai fbe3eed1e5 setns init: do explicit lookup of execve argument early
(This is a partial backport of a minor change included in commit
dac41717465462b21fab5b5942fe4cb3f47d7e53.)

This mirrors the logic in standard_init_linux.go, and also ensures that
we do not call exec.LookPath in the final execve step.

While this is okay for regular binaries, it seems exec.LookPath calls
os.Getenv which tries to emit a log entry to the test harness when
running in "go test" mode. In a future patch (in order to fix
CVE-2024-21626), we will close all of the file descriptors immediately
before execve, which would mean the file descriptor for test harness
logging would be closed at execve time. So, moving exec.LookPath earlier
is necessary.

Ref: dac4171746 ("runc-dmz: reduce memfd binary cloning cost with small C binary")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:57 +11:00
Aleksa Sarai 0994249a5e init: verify after chdir that cwd is inside the container
If a file descriptor of a directory in the host's mount namespace is
leaked to runc init, a malicious config.json could use /proc/self/fd/...
as a working directory to allow for host filesystem access after the
container runs. This can also be exploited by a container process if it
knows that an administrator will use "runc exec --cwd" and the target
--cwd (the attacker can change that cwd to be a symlink pointing to
/proc/self/fd/... and wait for the process to exec and then snoop on
/proc/$pid/cwd to get access to the host). The former issue can lead to
a critical vulnerability in Docker and Kubernetes, while the latter is a
container breakout.

We can (ab)use the fact that getcwd(2) on Linux detects this exact case,
and getcwd(3) and Go's Getwd() return an error as a result. Thus, if we
just do os.Getwd() after chdir we can easily detect this case and error
out.

In runc 1.1, a /sys/fs/cgroup handle happens to be leaked to "runc
init", making this exploitable. On runc main it just so happens that the
leaked /sys/fs/cgroup gets clobbered and thus this is only consistently
exploitable for runc 1.1.

Fixes: GHSA-xr7r-f8xq-vfvv CVE-2024-21626
Co-developed-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
[refactored the implementation and added more comments]
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:57 +11:00
hang.jiang 506552a88b Fix File to Close
(This is a cherry-pick of 937ca107c3d22da77eb8e8030f2342253b980980.)

Signed-off-by: hang.jiang <hang.jiang@daocloud.io>
Fixes: GHSA-xr7r-f8xq-vfvv CVE-2024-21626
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2024-01-24 00:10:57 +11:00
Aleksa Sarai 617db78515 configs: make id mappings int64 to better handle 32-bit
(This is a cherry-pick of 482e56379a1c2f8b2573f07c06013510e59f9eb8.)

Using ints for all of our mapping structures means that a 32-bit binary
errors out when trying to parse /proc/self/*id_map:

  failed to cache mappings for userns: failed to parse uid_map of userns /proc/1/ns/user:
  parsing id map failed: invalid format in line "         0          0 4294967295": integer overflow on token 4294967295

This issue was unearthed by commit 1912d5988b ("*: actually support
joining a userns with a new container") but the underlying issue has
been present since the docker/libcontainer days.

In theory, switching to uint32 (to match the spec) instead of int64
would also work, but keeping everything signed seems much less
error-prone. It's also important to note that a mapping might be too
large for an int on 32-bit, so we detect this during the mapping.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2023-12-14 12:19:49 +11:00
Aleksa Sarai e65d4cac66 specconv: temporarily allow userns path and mapping if they match
(This is a cherry-pick of ebcef3e651e61aeee96546301d8db9e92b505ce6.)

It turns out that the error added in commit 09822c3da8 ("configs:
disallow ambiguous userns and timens configurations") causes issues with
containerd and CRIO because they pass both userns mappings and a userns
path.

These configurations are broken, but to avoid the regression in this one
case, output a warning to tell the user that the configuration is
incorrect but we will continue to use it if and only if the configured
mappings are identical to the mappings of the provided namespace.

Fixes: 09822c3da8 ("configs: disallow ambiguous userns and timens configurations")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
2023-12-14 12:17:17 +11:00
Aleksa Sarai 8f8cb45572 configs: disallow ambiguous userns and timens configurations
(This is a cherry-pick of 09822c3da8ad8fa91b8796c5abf27ef06814a0c3.)

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>
2023-12-14 11:56:43 +11:00
Aleksa Sarai 0c8e2cc602 *: actually support joining a userns with a new container
(This is a cherry-pick of 1912d5988bbb379189ea9ceb2e03945738c513dc.)

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>
2023-12-14 11:56:33 +11:00
Heran Yang 87792ce02c libct/cg: add swapOnlyUsage in MemoryStats
This field reports swap-only usage. For cgroupv1, `Usage` and `Failcnt`
are set by subtracting memory usage from memory+swap usage. For cgroupv2,
`Usage`, `Limit`, and `MaxUsage` are set. This commit also export `MaxUsage`
of memory under cgroupv2 mode, using `memory.peak` introduced in kernel 5.19.

Signed-off-by: Heran Yang <heran55@126.com>
(cherry picked from commit 104b8dc951)
Signed-off-by: Harshal Patil <harpatil@redhat.com>
2023-12-12 11:42:09 -05:00
Kir Kolyshkin b426e9b76b libct/cgroups.OpenFile: clean "file" argument
This prevents potential exploit of using "../" in cgroups.OpenFile
(as well as other methods that use OpenFile) to read or write to
other cgroups.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 2c9598c886)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-10-27 10:47:20 -07:00
Kir Kolyshkin 8214e63474 libct/cg: support hugetlb rsvd
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)

[1.1 backport: check for CGROUP_UNIFIED in integration test]

Co-authored-by: Odin Ugedal <odin@ugedal.com>
(cherry picked from commit 4a7d3ae5cd)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-10-20 11:44:36 -07:00
lifubang b365458f40 fix a typo in cloned_binary.c: re-use -> reuse
Signed-off-by: lifubang <lifubang@acmcoder.com>
2023-10-04 13:24:36 +08:00
lifubang 8f66c9fbd0 fix two typos
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit 109dcadd9d)
Signed-off-by: lifubang <lifubang@acmcoder.com>
2023-10-04 13:17:41 +08:00
Jordan Rife 016b2b4233 Handle kmem.limit_in_bytes removal
kmem.limit_in_bytes has been removed in upstream linux and this patch
is queued to be backported to linux 6.1 stable:

- https://lore.kernel.org/linux-mm/20230705134434.GA156754@cmpxchg.org/T/
- https://www.spinics.net/lists/stable-commits/msg316619.html

Without this change to libcontainerd, GetStats() will return an error
on the latest kernel(s). A downstream effect is that Kubernetes's
kubelet does not start up. This fix was tested by ensuring that it
unblocks kubelet startup when running on the latest kernel.

Signed-off-by: Jordan Rife <jrife0@gmail.com>
(cherry picked from commit 99469eba3e)
Signed-off-by: Jordan Rife <jrife0@gmail.com>
2023-09-26 16:16:16 +00:00
Cory Snider f44190e076 libct/intelrdt: check if available iff configured
Unless the container's runtime config has intelRdt configuration set,
any checks for whether Intel RDT is supported or the resctrl filesystem
is mounted are a waste of time as, per the OCI Runtime Spec, "the
runtime MUST NOT manipulate any resctrl pseudo-filesystems." And in the
likely case where Intel RDT is supported by both the hardware and
kernel but the resctrl filesystem is not mounted, these checks can get
expensive as the intelrdt package needs to parse mountinfo to check
whether the filesystem has been mounted to a non-standard path.
Optimize for the common case of containers with no intelRdt
configuration by only performing the checks when the container has opted
in.

Signed-off-by: Cory Snider <csnider@mirantis.com>
(cherry picked from commit ea0bd78268)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Cory Snider 6cf9ac159e libct/intelrdt: skip remove unless configured
The OCI runtime spec mandates "[i]f intelRdt is not set, the runtime
MUST NOT manipulate any resctrl pseudo-filesystems." Attempting to
delete files counts as manipulating, so stop doing that when the
container's RDT configuration is nil.

Signed-off-by: Cory Snider <csnider@mirantis.com>
(cherry picked from commit 56daf36be2)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Cory Snider 4796f49ca3 libct/intelrdt: elide parsing mountinfo
The intelrdt package only needs to parse mountinfo to find the mount
point of the resctrl filesystem. Users are generally going to mount the
resctrl filesystem to the pre-created /sys/fs/resctrl directory, so
there is a common case where mountinfo parsing is not required. Optimize
for the common case with a fast path which checks both for the existence
of the /sys/fs/resctrl directory and whether the resctrl filesystem was
mounted to that path using a single statfs syscall.

Signed-off-by: Cory Snider <csnider@mirantis.com>
(cherry picked from commit c156bde7cc)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Cory Snider 6a7a6a5741 libct/intelrdt: skip reading /proc/cpuinfo
Reading /proc/cpuinfo is a surprisingly expensive operation. Since
kernel version 4.12 [1], opening /proc/cpuinfo on an x86 system can
block for around 20 milliseconds while the kernel samples the current
CPU frequency. There is a very recent patch [2] which gets rid of the
delay, but has yet to make it into the mainline kenel. Regardless,
kernels for which opening /proc/cpuinfo takes 20ms will continue to be
run in production for years to come. libcontainer only opens
/proc/cpuinfo to read the processor feature flags so all the delays to
get an accurate snapshot of the CPU frequency are just wasted time.

If we wanted to, we could interrogate the CPU features directly from
userspace using the `CPUID` instruction. However, Intel and AMD CPUs
have flags in different positions for their analogous sub-features and
there are CPU quirks [3] which would need to be accounted for. Some
Haswell server CPUs support RDT/CAT but are missing the `CPUID` flags
advertising their support; the kernel checks for support on that
processor family by probing the the hardware using privileged
RDMSR/WRMSR instructions [4]. This sort of probing could not be
implemented in userspace so it would not be possible to check for RDT
feature support in userspace without false negatives on some hardware
configurations.

It looks like libcontainer reads the CPU feature flags as a kind of
optimization so that it can skip checking whether the kernel supports an
RDT sub-feature if the hardware support is missing. As the kernel only
exposes subtrees in the `resctrl` filesystem for RDT sub-features with
hardware and kernel support, checking the CPU feature flags is redundant
from a correctness point of view. Remove the /proc/cpuinfo check as it
is an optimization which actually hurts performance.

[1]: https://unix.stackexchange.com/a/526679
[2]: https://lore.kernel.org/all/20220415161206.875029458@linutronix.de/
[3]: https://github.com/torvalds/linux/blob/7cf6a8a17f5b134b7e783c2d45c53298faef82a7/arch/x86/kernel/cpu/resctrl/core.c#L834-L851
[4]: https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/x86/kernel/cpu/resctrl/core.c#L111-L153

Signed-off-by: Cory Snider <csnider@mirantis.com>
(cherry picked from commit 9f107489b0)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Cory Snider 7c83dbe6ec libct/intelrdt: delete IsMBAScEnabled()
This function is unused, and removing it simplifies the changes which
follow this commit.

Signed-off-by: Cory Snider <csnider@mirantis.com>
(cherry picked from commit 13674f43d3)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Kir Kolyshkin 5ebcfa6281 [1.1] libct: rm intelrtd.Manager interface, NewIntelRdtManager
[This is a manual port of commit dbd990d555 to release-1.1
branch. Original description follows.]

Remove intelrtd.Manager interface, since we only have a single
implementation, and do not expect another one.

Rename intelRdtManager to Manager, and modify its users accordingly.

Remove NewIntelRdtManager from factory.

Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the
feature is not available.

Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew.

Add internal function newManager to be used for tests (to make sure
some testing is done even when the feature is not available in
kernel/hardware).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Kir Kolyshkin 69473d0af9 libct: rm TestGetContainerStats, mockIntelRdtManager
TestGetContainerStats test a function that is smaller than the test
itself, and only calls a couple of other functions (which are
represented by mocks). It does not make sense to have it.

mockIntelRdtManager is only needed for TestGetContainerStats
and TestGetContainerState, which basically tests that Path
is called. Also, it does not make much sense to have it.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 85932850ec)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Kir Kolyshkin dfdc7d078f libct/intelrdt: explain why mountinfo is required
For the Nth time I wanted to replace parsing mountinfo with
statfs and the check for superblock magic, but it is not possible
since some code relies of mount options check which can only
be obtained via mountinfo.

Add a note about it.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 5e201e7ce2)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Kir Kolyshkin 5ba1b8ec43 libct/intelrdt: faster init if rdt is unsupported
In a (quite common) case RDT is not supported by the kernel/hardware,
it does not make sense to parse /proc/cpuinfo and /proc/self/mountinfo,
and yet the current code does it (on every runc exec, for example).

Fortunately, there is a quick way to check whether RDT is available --
if so, kernel creates /sys/fs/resctrl directory. Check its existence,
and skip all the other initialization if it's not present.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit edeb3b376c)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Kir Kolyshkin a5407b9a36 libct/intelrdt: remove findMountpointDir test
This test was written back in the day when findIntelRdtMountpointDir
was using its own mountinfo parser. Commit f1c1fdf911 changed that,
and thus this test is actually testing moby/sys/mountinfo parser, which
does not make much sense.

Remove the test, and drop the io.Reader argument since we no longer need
to parse a custom file.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 6c6b14e075)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Kir Kolyshkin dc8d0cc172 libct/intelrdt: wrap Root in sync.Once
In case resctrl filesystem can not be found in /proc/self/mountinfo
(which is pretty common on non-server or non-x86 hardware), subsequent
calls to Root() will result in parsing it again and again.

Use sync.Once to avoid it. Make unit tests call it so that Root() won't
actually parse mountinfo in tests.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 02e961bcf9)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-10 17:09:53 +08:00
Alexander Eldeib 929d04fc8b libct/cg/fs2: use file + anon + swap for usage
This aligns v2 usage calculations more closely with v1.
Current node-level reporting for v1 vs v2 on the same
machine under similar load may differ by ~250-750Mi.

Also return usage as combined swap + memory usage, aligned
with v1 and non-root v2 cgroups.

`mem_cgroup_usage` in the kernel counts NR_FILE_PAGES
+ NR_ANON_MAPPED + `nr_swap_pages` (if swap enabled) [^0].

Using total - free results in higher "usage" numbers.
This is likely due to various types of reclaimable
memory technically counted as in use (e.g. inactive anon).

See also https://github.com/kubernetes/kubernetes/issues/118916 for more context

[^0]: https://github.com/torvalds/linux/blob/06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5/mm/memcontrol.c#L3673-L3680

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>
2023-08-09 19:20:15 -07:00
Kir Kolyshkin bdbfe0425f ci: bump golangci-lint, remove fixed exception
The exception was fixed by https://github.com/polyfloyd/go-errorlint/pull/12
which eventually made its way into golangci-lint.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 98317c16ed)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-09 16:44:50 -07:00
Kir Kolyshkin 44a53f088c ci: fix TestOpenat2 when no systemd is used
A few cases relied on the fact that systemd is used, and thus
/sys/fs/cgroup/user.slice is available.

Guess what, in case of "make unittest" it might not be.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 5c6b334c88)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-08 18:47:03 -07:00
Kir Kolyshkin cff41a893c ci: fix TestNilResources when systemd not available
Split the test into two -- for fs and systemd cgroup managers, and only
run the second one if systemd is available.

Prevents the following failure during `make unittest`:

> === RUN   TestNilResources
>     manager_test.go:27: systemd not running on this host, cannot use systemd cgroups manager
> --- FAIL: TestNilResources (0.22s)

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 962019d64e)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-08-08 18:46:40 -07:00
lifubang 3d3a2b38b4 fix some file mode bits missing when doing mount syscall
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit 6092a4b42d)
2023-08-03 11:31:03 +08:00
lfbzhm 096277c761 Merge pull request #3921 from jiusanzhou/1.1-bugfix/skip-update-while-frozen-faield
[1.1] libct/cg/sd/v1: do not update non-frozen cgroup after frozen failed.
2023-07-16 08:16:43 +08:00
Kir Kolyshkin 1188c5a192 runc delete: call systemd's reset-failed
runc delete is supposed to remove all the container's artefacts.
In case systemd cgroup driver is used, and the systemd unit has failed
(e.g. oom-killed), systemd won't remove the unit (that is, unless the
"CollectMode: inactive-or-failed" property is set).

Call reset-failed from manager.Destroy so the failed unit will be
removed during "runc delete".

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 43564a7b55)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-07-08 11:54:41 -07:00
Kir Kolyshkin 71e7600768 libct/cg/sd: remove logging from resetFailedUnit
Sometimes we call resetFailedUnit as a cleanup measure, and we don't
care if it fails or not. So, move error reporting to its callers, and
ignore error in cases we don't really expect it to succeed.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 91b4cd25b7)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-07-08 11:54:20 -07:00
Zoe 6bc3f22a31 libct/cg/sd/v1: do not update non-frozen cgroup after frozen failed.
In code we have frozen the cgroup to avoid the processes get
an occasional "permission denied" error, while the systemd's application of device
rules is done disruptively. When the processes in the container can not
be frozen over 2 seconds (which defined in fs/freezer.go),
we still update the cgroup which resulting the container get an occasional
"permission denied" error in some cases.

Return error directly without updating cgroup, when freeze fails.

Fixes: #3803

Signed-off-by: Zoe <hi@zoe.im>
2023-06-30 19:23:36 +08:00
Akihiro Suda 9164fe17a7 libct/seccomp: add riscv64
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 1d7b297128)
2023-06-28 22:34:45 +03:00
Brian Goff 9af462e4a7 Fix tmpfs mode opts when dir already exists
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>
(cherry picked from commit 9fa8b9de3e)
Resolved conflicts:
	tests/integration/run.bats
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
2023-06-28 08:21:04 -06:00
Sebastiaan van Stijn bcf234b897 Merge pull request #3877 from kolyshkin/1.1-sd-rm-race
[1.1] libct: fix a race with systemd removal
2023-05-30 14:34:20 +02:00
Aleksa Sarai 63af8b0069 init: do not print environment variable value
When given an environment variable that is invalid, it's not a good idea
to output the contents in case they are supposed to be private (though
such a container wouldn't start anyway so it seems unlikely there's a
real way to use this to exfiltrate environment variables you didn't
already know).

Reported-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 20e38fb2b1)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-05-22 14:52:57 -07:00
Kir Kolyshkin 404ea7ab0f libct: fix a race with systemd removal
For a previous attempt to fix that (and added test cases), see commit
9087f2e827.

Alas, it's not always working because of cgroup directory TOCTOU.

To solve this and avoid the race, add an error _after_ the operation.
Implement it as a method that ignores the error that should be ignored.
Instead of currentStatus(), use faster runType(), since we are not
interested in Paused status here.

For Processes(), remove the pre-op check, and only use it after getting
an error, making the non-error path more straightforward.

For Signal(), add a second check after getting an error. The first check
is left as is because signalAllProcesses might print a warning if the
cgroup does not exist, and we'd like to avoid that.

This should fix an occasional failure like this one:

	not ok 84 kill detached busybox
	# (in test file tests/integration/kill.bats, line 27)
	#   `[ "$status" -eq 0 ]' failed
	....
	# runc kill test_busybox KILL (status=0):
	# runc kill -a test_busybox 0 (status=1):
	# time="2023-04-04T18:24:27Z" level=error msg="lstat /sys/fs/cgroup/devices/system.slice/runc-test_busybox.scope: no such file or directory"

(cherry picked from commit fe278b9caa)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-05-22 14:06:04 -07:00
Kir Kolyshkin 79a52b4349 libct/cg/sd: use systemd version when generating dev props
Commit 343951a22b added a call to os.Stat for the device path
when generating systemd device properties, to avoid systemd warning for
non-existing devices. The idea was, since systemd uses stat(2) to look
up device properties for a given path, it will fail anyway. In addition,
this allowed to suppress a warning like this from systemd:

> Couldn't stat device /dev/char/10:200

NOTE that this was done because:
 - systemd could not add the rule anyway;
 - runs puts its own set of rules on top of what systemd does.

Apparently, the above change broke some setups, resulting in inability
to use e.g. /dev/null inside a container. My guess is this is because
in cgroup v2 we add a second eBPF program, which is not used if the
first one (added by systemd) returns "access denied".

Next, commit 3b9582895b fixed that by adding a call to os.Stat for
"/sys/"+path (meaning, if "/dev/char/10:200" does not exist, we retry
with "/sys/dev/char/10:200", and if it exists, proceed with adding a
device rule with the original (non-"/sys") path).

How that second fix ever worked was a mystery, because the path we gave
to systemd still doesn't exist.

Well, I think now I know.

Since systemd v240 (commit 74c48bf5a8005f20) device access rules
specified as /dev/{block|char}/MM:mm are no longer looked up on the
filesystem, instead, if possible, those are parsed from the string.

So, we need to do different things, depending on systemd version:

 - for systemd >= v240, use the /dev/{char,block}/MM:mm as is, without
   doing stat() -- since systemd doesn't do stat() either;
 - for older version, check if the path exists, and skip passing it on
   to systemd otherwise.
 - the check for /sys/dev/{block,char}/MM:mm is not needed in either
   case.

Pass the systemd version to the function that generates the rules, and
fix it accordingly.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit d7208f5910)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-04-25 09:38:05 -07:00
Kir Kolyshkin e4ce94e291 libct/cg: add misc controller to v1 drivers
This is just so that the container can join the misc controller.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 611bbacb3b)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-04-10 15:34:51 -07:00
Peter Hunt~ 10cfd81631 libctr/cgroups: don't take init's cgroup into account
Sometimes, the init process is not in the root cgroup.
This can be noted by GetInitPath, which already scrubs the path of `init.scope`.

This was encountered when trying to patch the Kubelet to handle systemd being in a separate cpuset
from root (to allow load balance disabling for containers). At present, there's no way to have libcontainer or runc
manage cgroups in a hierarchy outside of the one init is in (unless the path contains `init.scope`, which is limiting)

Signed-off-by: Peter Hunt <pehunt@redhat.com>
(cherry picked from commit 54e20217a8)
2023-04-07 09:41:30 -04:00
Kir Kolyshkin 840b95394b Fix runc run "permission denied" when rootless
Since commit 957d97bcf4 was made to fix issue [7],
a few things happened:

- a similar functionality appeared in go 1.20 [1], so the issue
  mentioned in the comment (being removed) is no longer true;
- a bug in runc was found [2], which also affects go [3];
- the bug was fixed in go 1.21 [4] and 1.20.2 [5];
- a similar fix was made to x/sys/unix.Faccessat [6].

The essense of [2] is, even if a (non-root) user that the container is
run as does not have execute permission bit set for the executable, it
should still work in case runc has the CAP_DAC_OVERRIDE capability set.

To fix this [2] without reintroducing the older bug [7]:
- drop own Eaccess implementation;
- use the one from x/sys/unix for Go 1.19 (depends on [6]);
- do not use anything when Go 1.20+ is used.

NOTE it is virtually impossible to fix the bug [2] when Go 1.20 or Go
1.20.1 is used because of [3].

A test case is added by a separate commit.

Fixes: #3715.

[1] https://go-review.googlesource.com/c/go/+/414824
[2] https://github.com/opencontainers/runc/issues/3715
[3] https://go.dev/issue/58552
[4] https://go-review.googlesource.com/c/go/+/468735
[5] https://go-review.googlesource.com/c/go/+/469956
[6] https://go-review.googlesource.com/c/sys/+/468877
[7] https://github.com/opencontainers/runc/issues/3520

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 8491d33482)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2023-04-06 18:07:03 -07:00