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>
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>
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>
When getting pids stats, instead of checking whether the pids controller
is available, let's use a fall back function in case pids.current does
not exist. This simplifies the logic in fs2.GetStats.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is inefficient to create an associative map for the whole purpose of
counting the number of elements in it, especially if the elements are
all unique. It uses more CPU than necessary and creates some work for
the garbage collector.
The file we read contains PIDs and newlines, and the easiest/fastest way
to get the number of PIDs is just to count the newlines.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Giuseppe found out that cpu.stat for a cgroup is available even if
the cpu controller is not enabled for it. So, let's call statCpu
regradress, and ignore ENOENT.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is no memory.{current,max} for the root node in cgroup v2, and
thus stats for "/sys/fs/cgroup" return an error.
The same thing works with cgroup v1 (as there are
memory.{usage,limit}_in_bytes files in the root node).
Emulate stats for /sys/fs/cgroup by getting numbers from
/proc/self/meminfo.
NOTE that both memory.current (in cgroup v2) and memory.usage_in_bytes
(in cgroup v1) include page cache etc into the number, so we do the same
when calculating memory usage (as opposed to number reported by "free",
which excludes page cache and buffers).
[v2: check for cgroup files first, as future kernels might add it]
[v3: don't subtract cache from mem_used, simplifying the logic]
[Initially, I wanted to avoid parsing yet another /proc file and
instead mock some numbers using data from memory.stat but was
unable to come up with formulae that make sense.]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Existing code ignores ENOENT error in case we're reading data from
controls that might not be enabled. While this is correct, the code
can be improved:
1. Check name != "" instead of moduleName != "memory", as these checks
are equivalent but the new one is faster.
2. It does not make sense to ignore subsequent errors -- if the control
is not available, we won't hit this codepath.
3. Add a comment explaining why we ignore the error.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
cgroup v1 reports combined mem+swap in stats.MemoryStats.SwapUsage.
In cgroup v2, swap is separate.
For the sake of compatibility, make v2 report mem+swap as well.
This also includes Limit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This unconditionally enables TasksAccounting for systemd unified (v2)
cgroup driver, making it work the same way as the legacy (v1) driver.
Practically, it is probably a no-op since DefaultTasksAccounting is
usually true.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
if bfq is not loaded, then io.bfq.weight is not available. io.weight
should always be available and is the next best equivalent thing.
Signed-off-by: Daniel Dao <dqminh89@gmail.com>
Aleksa Sarai