gofumpt (mvdan.cc/gofumpt) is a fork of gofmt with stricter rules.
Brought to you by
git ls-files \*.go | grep -v ^vendor/ | xargs gofumpt -s -w
Looking at the diff, all these changes make sense.
Also, replace gofmt with gofumpt in golangci.yml.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
strconv.ParseUint(..., 0) is not really safe, because on 32-bit
architectures it will trigger runtime errors when trying to parse large
numbers (which in the case of the cgroupv2 io controller, is almost
certainly going to happen).
Fixes: 1932917b71 ("libcontainer: add initial support for cgroups v2")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Kubelet and cAdvisor depend on the metrics having the same values as in
cgroupv1, but we didn't correctly map the number of read and write IOs
to the correct cgroupv1 stats table (blkio.io_serviced).
In addition, don't leak any extra stats in our output -- if users need
that information we can always add a new field for it.
Reported-by: Yashpal Choudhary <yashpal.c1995@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The idea is to mimic what kubelet is doing, with minimum amount of code.
First, create a slice with SkipDevices=true. It should have access to
all devices.
Next, create a scope within the above slice, allowing access to /dev/full
only.
Check that within that scope we can only access /dev/full and not other
devices (such as /dev/null).
Repeat the test with SkipDevices=false, make sure we can not access any
devices (as they are disallowed by a parent cgroup). This is done only
to assess the test correctness.
NOTE that cgroup v1 and v2 behave differently for SkipDevices=false
case, and thus the check is different. Cgroup v1 returns EPERM on
writing to devices.allow, so cgroup manager's Set() fails, and we check
for a particular error from m.Set(). Cgroup v2 allows to create a child
cgroup, but denies access to any device (despite access being enabled)
-- so we check the error from the shell script running in that cgroup.
Again, this is only about SkipDevices=false case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most of these were false positives or cases where we want to ignore the
lint, but the change to the BPF generation is actually useful.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Commit 108ee85b82 adds SkipDevices flag, which is used by kubernetes
to create cgroups for pods.
Unfortunately the above commit falls short, and systemd DevicePolicy and
DeviceAllow properties are still set, which requires kubernetes to set
"allow everything" rule.
This commit fixes this: if SkipDevices flag is set, we return
Device* properties to allow all devices.
Fixes: 108ee85b82
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In the normal cases (only one existing filter or no existing filters),
just make use of BPF_F_REPLACE if there is one existing filter. However
if there is more than one filter applied, we should probably remove all
other filters since the alternative is that we will never remove our old
filters.
The only two other viable ways of solving this problem would be to use
BPF pins to either pin the eBPF program using a predictable name (so we
can always only replace *our* programs) or to switch away from custom
programs and instead use eBPF maps (which are pinned) and thus we just
update the map conntents to update the ruleset. Unfortunately these both
would add a hard requirement of bpffs and would require at least a minor
rewrite of the eBPF filtering code -- which is better left for another
time.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
There were several issues with the previous cgroupv2 devices filter
generator implementation, stemming from the previous implementation
using a few too many tricks to implement the correct cgroup behaviour
(rules were handled in reverse order, with wildcards having particularly
special interpretations). As a result, some slightly odd configurations
with rules in specific orders could result in incorrect filters being
generated.
By switching to the emulator which is already used by cgroupv1, we can
guarantee that the behaviour of filters in both cgroup versions will be
identical, as well as making use of the hardenings in the emulator (not
allowing users to add deny rules the kernel will ignore).
(Note that because the ordering of the devices emulator rules is
deterministic and based on the rule value, the existing test rules had
to be reordered slightly.)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The devices cgroup emulator is also useful for removing unneeded rules
as well as computing what the final default-allow state of the filter
will be (allow-list or deny-list).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
According to cgroup v2 documentation [1]:
> Freezing of the cgroup may take some time; when this action is
> completed, the “frozen” value in the cgroup.events control file will
> be updated to “1” and the corresponding notification will be issued.
Implement polling of cgroup.events, waiting for "frozen 1" to appear.
In case something goes wrong, limit the maximum number of retries and
return "undefined" after some time (currently 10s).
[1] https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Before this patch, setFreezer does
- open/read/close (to check if the freezer is supported)
- open/write/close (to set the value)
- open/read/close (to check the value)
Three opens is a bit excessive. Refactor to only open the file once:
- open (to check if the freezer is supported)
- write (to set the value)
- seek/read (to check the value)
- close
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case configs.Undefined or any wrong value is passed, there is no need
to check whether the freezer is supported.
Move arguments check to the beginning to avoid an unnecessary call to
supportFreezer().
While at it, simplify the "whether to return an error if freezer is not
supported" check.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Historically, we never returned an error from failed startUnit
or stopUnit. The startUnit case was fixed by commit 3844789.
It is time to fix stopUnit, too. The reasons are:
1. Ignoring an error from stopUnit means an unexpected trouble down the
road, for example a failure to create a container with the same name:
> time="2021-05-07T19:51:27Z" level=error msg="container_linux.go:380: starting container process caused: process_linux.go:385: applying cgroup configuration for process caused: Unit runc-test_busybox.scope already exists."
2. A somewhat short timeout of 1 second means the cgroup might
actually be removed a few seconds later but we might have a
race between removing the cgroup and creating another one
with the same name, resulting in the same error as amove.
So, return an error if removal failed, and increase the timeout.
Now, modify the systemd cgroup v1 manager to not mask the error from
stopUnit (stopErr) with the subsequent one from cgroups.RemovePath,
as stopErr is most probably the reason why RemovePath failed.
Note that for v1 we do want to remove the paths even in case of a
failure from stopUnit, as some were not created by systemd.
There's no need to do that for v2, thanks to unified hierarchy,
so no changes there.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes Path() is called before m.path is initialized (in particular,
this happens from (*linuxContainer).newInitConfig), so we do need to
make sure to call initPath.
This fixes the following integration tests (for cgroup v2 + systemd case,
currently not enabled -- to be enabled by further commits):
* runc run (blkio weight)
* runc run (cgroupv2 mount inside container)
Fixes: ff692f289b ("Fix cgroup2 mount for rootless case")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using per cgroup manager dbus connection instances means
that every cgroup manager instance gets a new connection,
and those connections are never closed, ultimately resulting
in file descriptors limit being hit.
Revert back to using a single global dbus connection for everything,
without changing the callers.
NOTE that it is assumed a runtime can't use both root and rootless
dbus at the same time. If this happens, we panic.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 47ef9a104f forgot to wrap GetManagerProperty("ControlGroup")
into retryOnDisconnect. Since there's one other user of
GetManagerProperty, add getManagerProperty wrapper and use it.
Fixes: 47ef9a104f
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I hate to keep adding those kludges, but lately TestFreeze (and
TestSystemdFreeze) from libcontainer/integration fails a lot. The
failure comes and goes, and is probably this is caused by a slow host
allocated for the test, and a slow VM on top of it.
To remediate, add a small sleep on every 25th iteration in between
asking the kernel to freeze and checking its status.
In the worst case scenario (failure to freeze), this adds about 0.4 ms
(40 x 10 us) to the duration of the call.
It is hard to measure how this affects CI as GHA plays a roulette when
allocating a node to run the test on, but it seems to help. With
additional debug info, I saw somewhat frequent "frozen after 24 retries"
or "frozen after 49 retries", meaning it succeeded right after the added
sleep.
While at it, rewrite/improve the comments.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A cgroup manager's Set method sets cgroup resources, but historically
it was accepting configs.Cgroups.
Refactor it to accept resources only. This is an improvement from the
API point of view, as the method can not change cgroup configuration
(such as path to the cgroup etc), it can only set (modify) its
resources/limits.
This also lays the foundation for complicated resource updates, as now
Set has two sets of resources -- the one that was previously specified
during cgroup manager creation (or the previous Set), and the one passed
in the argument, so it could deduce the difference between these. This
is a long term goal though.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For some reason, systemd cgroup v2 driver's Set is not using its
container argument when generating systemd unit properties.
This bug is not detected by our update tests as we run a new binary
every time and thus a new instance of a cgroup manager.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 88e8350de2, among the other things, replaced filepath.Join with
securejoin.SecureJoin for both reads and writes to cgroupfs.
Commits e76ac1c054 and 31f0f5b7e0 switched more code to use
fscommon.ReadFile (and thus securejoin). Commit 0228226e6d introduced
fscommon.OpenFile (which uses securejoin as the fallback if openat2(2)
is not available, which is the case for older kernels), and commit
c95e69007c switched most of cgroup/fs[2] code to use it.
As a result, fs.GetStats() method became noticeable slower, mostly due
to securejoin calling os.Lstat and filepath.Clean.
Using securejoin as a security measure for cgroupfs files is
not well justified, as cgroupfs do not contain symlinks, and none of the
code using it have uncleaned paths. In particular, fs/fs2/systemd
managers do check and sanitize their paths.
This commit modifies the code to not use securejoin. Instead, it checks
that the opened file is indeed on cgroupfs.
Using BenchmarkGetStats on a CentOS 8 VM, I see the following
improvement:
Before:
> BenchmarkGetStats-8 8376 625135 ns/op
After:
> BenchmarkGetStats-8 12226 485015 ns/op
An intermediate version, with no fstatfs to check fstype:
> BenchmarkGetStats-8 13162 452281 ns/op
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of reconnecting to dbus after some failed operations, and
returning an error (so a caller has to retry), reconnect AND retry
in place for all such operations.
This should fix issues caused by a stale dbus connection after e.g.
a dbus daemon restart.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
[@kolyshkin: doc nits, use dbus.ErrClosed and isDbusError]
Signed-off-by: Shiming Zhang <wzshiming@foxmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Generalize isUnitExists as isDbusError, and use errors.As while at it
(which can handle wrapped errors as well).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case openat2() is not available, it does not make sense to calculate
relpath (and check if path has /sys/fs/cgroup prefix).
Reverse the order of checks to not do that in case openat2 is not
available.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Function (*FreezerGroup).Set has a few paths where in can return an
error. In any case, if an error is returned, we failed to freeze,
and we need to thaw to avoid leaving the cgroup in a stuck state.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 5d0ffbf9c8 added OOM kill count checking and better container
start/run/exec error reporting in case we hit OOM.
It also introduced warnings like these:
> level=warning msg="unable to get oom kill count" error="openat2
> /sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/test_hello/memory.events:
> no such file or directory"
In case of rootless containers, unless cgroup is delegated or systemd is
used, runc can not create a cgroup and thus it fails to get OOM kill
count. This is expected, and the warning should not be shown in this
case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For fs, commit fc620fdf81 made rootless field private,
and for fs2, it was always private, and yet comments in both
mention it as m.Rootless.
Fix it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Caught by golangci-lint when enabling golint:
libcontainer/cgroups/ebpf/ebpf.go:35:12: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead. (staticcheck)
if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
^
libcontainer/cgroups/ebpf/ebpf.go:39:13: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead. (staticcheck)
if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
^
Worth noting that we currently call prog.Detach() with unix.BPF_F_ALLOW_MULTI;
https://github.com/golang/sys/blob/22da62e12c0cd9c1da93581e1113ca4d82a5be14/unix/zerrors_linux.go#L178
BPF_F_ALLOW_MULTI = 0x2
Looking at the source code for prog.Detach(); https://github.com/cilium/ebpf/blob/v0.4.0/prog.go#L579-L581,
this would _always_ produce an error:
if flags != 0 {
return errors.New("flags must be zero")
}
Note that the flags parameter is not used (except for that validation)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This is somewhat radical approach to deal with kernel memory.
Per-cgroup kernel memory limiting was always problematic. A few
examples:
- older kernels had bugs and were even oopsing sometimes (best example
is RHEL7 kernel);
- kernel is unable to reclaim the kernel memory so once the limit is
hit a cgroup is toasted;
- some kernel memory allocations don't allow failing.
In addition to that,
- users don't have a clue about how to set kernel memory limits
(as the concept is much more complicated than e.g. [user] memory);
- different kernels might have different kernel memory usage,
which is sort of unexpected;
- cgroup v2 do not have a [dedicated] kmem limit knob, and thus
runc silently ignores kernel memory limits for v2;
- kernel v5.4 made cgroup v1 kmem.limit obsoleted (see
https://github.com/torvalds/linux/commit/0158115f702b).
In view of all this, and as the runtime-spec lists memory.kernel
and memory.kernelTCP as OPTIONAL, let's ignore kernel memory
limits (for cgroup v1, same as we're already doing for v2).
This should result in less bugs and better user experience.
The only bad side effect from it might be that stat can show kernel
memory usage as 0 (since the accounting is not enabled).
[v2: add a warning in specconv that limits are ignored]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Moving these utilities to a separate package, so that consumers of this
package don't have to pull in the whole "system" package.
Looking at uses of these utilities (outside of runc itself);
`RunningInUserNS()` is used by [various external consumers][1],
so adding a "Deprecated" alias for this.
[1]: https://grep.app/search?current=2&q=.RunningInUserNS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Some controllers might still have stats available even if they are
disabled (this is definitely so for cpu.stat -- see earlier commit).
Some stat methods might implement sensible fallbacks (see previous
commit for statPids.
In the view of all that, it makes sense to not check if a particular
controller is available, but rather ignore ENOENT from it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin