TestPodSkipDevicesUpdate checks that updating a pod having SkipDevices: true
does not result in spurious "permission denied" errors in a container
running under the pod. The test is somewhat similar in nature to the
@test "update devices [minimal transition rules]" in tests/integration,
but uses a pod.
This tests the validity of freezeBeforeSet in v1.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was initially added by commit 3e5c199708 because Set (with
r.Freezer = Frozen) was not able to freeze a container.
Now (see a few previous commits) Set can do the freeze, so the explicit
Freeze is no longer needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Introduce freezeBeforeSet, which contains the logic of figuring out
whether we need to freeze/thaw around setting systemd unit properties.
In particular, if SkipDevices is set, and the current unit properties
allow all devices, there is no need to freeze and thaw, as systemd
won't write any device rules in this case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
m.Freeze method changes m.cgroups.Resources.Freezer field, which should
not be done while we're temporarily freezing the cgroup in Set. If this
field is changed, and r == m.cgroups.Resources (as it often happens),
this results in inability to freeze the container using Set().
To fix, add and use a method which does not change r.Freezer field.
A test case for the bug will be added separately.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is necessary in order for runc to be able to configure device
cgroups with --systemd-cgroup on distributions that have very strict
SELinux policies such as openSUSE MicroOS[1].
The core issue here is that systemd is adding its own BPF policy that
has an SELinux label such that runc cannot interact with it. In order to
work around this, we can just ignore the policy -- in theory this
behaviour is not correct but given that the most obvious case
(--systemd-cgroup) will still handle updates correctly, this logic is
reasonable.
[1]: https://bugzilla.suse.com/show_bug.cgi?id=1182428
Fixes: d0f2c25f52 ("cgroup2: devices: replace all existing filters when attaching")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This test the issues fixed by the two preceding commits.
Co-Authored-By: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Odin Ugedal <odin@uged.al>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If a control group is frozen, all its descendants will report FROZEN
in freezer.state cgroup file.
OTOH cgroup v2 cgroup.freeze is not reporting the cgroup as frozen
unless it is frozen directly (i.e. not via an ancestor).
Fix the discrepancy between v1 and v2 drivers behavior by
looking into freezer.self_freezing cgroup file, which, according
to kernel documentation, will show 1 iff the cgroup was frozen directly.
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Odin Ugedal <odin@uged.al>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since device updates in cgroup v2 are atomic for systemd, there is no
need to freeze the processes before running the updates.
Signed-off-by: Odin Ugedal <odin@uged.al>
Errors from unix.* are always bare and thus can be used directly.
Add //nolint:errorlint annotation to ignore errors such as these:
libcontainer/system/xattrs_linux.go:18:7: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
case errno == unix.ERANGE:
^
libcontainer/container_linux.go:1259:9: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
if e != unix.EINVAL {
^
libcontainer/rootfs_linux.go:919:7: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
if err != unix.EINVAL && err != unix.EPERM {
^
libcontainer/rootfs_linux.go:1002:4: switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors (errorlint)
switch err {
^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Do this for all errors except one from unix.*.
This fixes a bunch of errorlint warnings, like these
libcontainer/generic_error.go:25:15: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
if le, ok := err.(Error); ok {
^
libcontainer/factory_linux_test.go:145:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
lerr, ok := err.(Error)
^
libcontainer/state_linux_test.go:28:11: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
_, ok := err.(*stateTransitionError)
^
libcontainer/seccomp/patchbpf/enosys_linux.go:88:4: switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors (errorlint)
switch err {
^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This one is tough as errorlint insists on using errors.Is, and the
latter is known to not work for Go 1.13 which we still support.
So, add a nolint annotation to suppress the warning, and a TODO to
address it later.
For intelrdt, we can do the same, but it is easier to reuse the very
same function from fscommon (note we can't use fscommon for other stuff
as it expects cgroupfs).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This should result in no change when the error is printed, but make the
errors returned unwrappable, meaning errors.As and errors.Is will work.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This builds on top of recently introduced fscommon.ParseError.
Errors returned from parsers (mostly ones used by GetStats()) are all
different, and many are incomplete. For example, in many cases errors
from strconv.ParseUint are returned as is, meaning there is no context
telling which file we were reading. Similarly, errors from
fscommon.ParseKeyValue should be wrapped to add more context.
Same is true for scanner.Err().
OTOH, errors from fscommon.GetCgroup* do have enough context and there
is no need to wrap them.
Fix all the above.
While at it, add missing scanner.Err() checks.
[v2: use parseError, not ParseError]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This builds on top of recently introduced fscommon.ParseError.
Errors returned from parsers (mostly ones used by GetStats()) are all
different, and many are incomplete. For example, in many cases errors
from strconv.ParseUint are returned as is, meaning there is no context
telling which file we were reading. Similarly, errors from
fscommon.ParseKeyValue should be wrapped to add more context.
Same is true for scanner.Err().
One special case that repeats a few times is "malformed line: xxx".
Add and use a helper for that to simplify things.
OTOH, errors from fscommon.GetCgroup* do have enough context and there
is no need to wrap them.
Fix all the above.
While at it, add a missing scanner.Err() check.
[v2: use parseError not ParseError]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Introduce ParseError type as a way to unify error messages related to
file parsing. Use it from GetCgroup* functions.
2. Do not discard the error from strconv.Parse{Int,Uint} -- it contains
the value being parsed, and the details about the error.
2. As the error above already contains the value, drop it from format.
[v2: use path.Join in Error]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Do not wrap errors returned from fscommon.GetCgroupParamUint -- those
errors already have enough context.
2. Instead of parsing "max" ourselves, use GetCgroupParamUint which does
it, and then convert MaxUint64 to 0 (we do it historically since
commit 087b953dc5, and while using MaxUint64 as is seems fine,
there may be some existing users who rely on the old behavior).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The error from fscommon.GetCgroup* already contains the file name and so
on, so there's no need to wrap it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The error returned from strconv.ParseUint is already pretty descriptive,
something like:
strconv.ParseUint: parsing "000d": invalid syntax
So, there is no need to add more context to it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using fmt.Errorf for errors that do not have %-style formatting
directives is an overkill. Switch to errors.New.
Found by
git grep fmt.Errorf | grep -v ^vendor | grep -v '%'
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Per-device weight is supported since kernel v5.4 (kernel commit
795fe54c2a8), so let's set those if supplied.
[v2: implement a more relaxed check in bfqDeviceWeightSupported]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For per-device weight, you can set weight and/or leaf weight.
The problem is, with the recent fix to use BFQ on cgroup v1,
if per-device weights are set, the code tries to set device
weight to blkio.bfq.weight, and the leaf weight to
blkio.leaf_weight_device. The latter file does not exist on
kernels v5.0, meaning one can not set any per-device weights
at all.
The fix is to only set weights if they are non-zero (i.e. set).
The test case will come in a following commit.
Fixes: 6339d8a0dd
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a better place as cgroups itself is using these.
Should help with moving more stuff common in between fs and fs2 to
fscommon.
Looks big, but this is just moving the code around:
fscommon/{fscommon,open}.go -> cgroups/file.go
fscommon/fscommon_test.go -> cgroups/file_test.go
and fixes for TestMode moved to a different package.
There's no functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The `errors.Is(err, unix.EINVAL)` check in `haveBpfProgReplace()` was
broken because the `cilium/ebpf` library did not "wrap" errors.
https://github.com/cilium/ebpf/blob/v0.6.0/link/program.go#L72
So the eBPF support of runc was broken for kernel prior to 5.6.
This commit bumps up cilium/ebpf to contain cilium/ebpf PR 320.
Fix opencontainers/runc issue 3008
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
- Update the blkio cgroup to support the BFQ I/O Scheduler, that has
replaced CFQ in the Linux kernel.
- BFQ is controlled through blkio.bfq.weight[_device] instead of
CFQ's blkio.weight[_device] in cgroups v1.
- BFQ does not support blkio.leaf_weight[_device], so that behavior
remains untouched.
- Do not change behavior on legacy CFQ systems.
- Enable using blkio weights on BFQ systems.
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
1. The meaning of SkipDevices is what it is -- do not set any
device-related options.
2. Reverts the part of commit 108ee85b82 which skipped the freeze
when the SkipDevices is set. Apparently, the freeze is needed on
update even if no Device* properties are being set.
3. Add "runc update" to "runc run [device cgroup deny]" test.
Fixes: 752e7a8249
Fixes: 108ee85b82
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It seems that we are triggering the mutli-attach fallback in the fedora
CI, but we don't have enough debugging information to really know what's
going on, so add some. Unfortunately the amount of information we have
available with eBPF programs in general is fairly limited (we can't get
their bytecode for instance).
We also demote the "more than one filter" warning to an info message
because it happens very often under the systemd cgroup driver (likely
when systemd configures the cgroup it isn't deleting our old program, so
when our apply code runs after the systemd one there are two running
programs).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
It turns out that the cilium eBPF library doesn't degrade gracefully if
BPF_F_REPLACE is not supported, so we need to work around it by treating
that case as we treat the more-than-one program case.
It also turns out that we weren't passing BPF_F_REPLACE explicitly, but
this is required by the cilium library (causing EINVALs).
Fixes: d0f2c25f52 ("cgroup2: devices: replace all existing filters when attaching")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Noticed that the check of trying to use both rootful and rootless
in NewDbusConnManager never worked, as we never set dbusInited to true.
Do that. While at it, protect this with the mutex (against the
case of two goroutines simultaneously calling NewDbusConnManager).
This is a rare call, so taking read-only then read-write mutex does not
make sense.
Fixes: c7f847ed3a
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes isDbusError function, introduced by commit bacfc2c. Due to a
type error it was not working at all.
This also fixes the whole "retry on dbus disconnect" logic.
This also fixes a regression in startUnit (and cgroupManager.Apply()),
which should never return "unit already exists" error but it did.
Fixes: bacfc2c
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These functions are called from multiple places,
and if t.Helper() is not used, the context is not clear.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
libcontainer/cgroups/devices/devices_emulator.go:261:9: `if` block ends with a `return` statement, so drop this `else` and outdent its block (golint)
} else {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Kir Kolyshkin