This will make releases much simpler. I've back-filled the changelog
with everything since runc 1.0.0 (there's not much point going further
back than that).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This check was always broken, and it slipped through the cracks because
we never run it without additional architectures now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It was released about a month ago. I don't see anything major
in the changelog but it makes sense to keep tracking upstream deps.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using "$@" instead of $1 in update_config() allows us to use it from
hooks.bats, where jq is used with more options than usual.
We need to disable SC2016 as otherwise shellcheck sees $something inside
single quotes and think we are losing the shell expansion (we are not).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This can be used to specify a different runc binary, for example:
sudo -E RUNC=$PWD/runc.mine tests/integration/cwd.bats
A different (but compatible enough) runtime also works:
sudo -E RUNC=/usr/local/bin/crun tests/integration/cwd.bats
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new mount option "rro" makes the mount point recursively read-only,
by calling `mount_setattr(2)` with `MOUNT_ATTR_RDONLY` and `AT_RECURSIVE`.
https://man7.org/linux/man-pages/man2/mount_setattr.2.html
Requires kernel >= 5.12.
The "rro" option string conforms to the proposal in util-linux/util-linux Issue 1501.
Fix issue 2823
Similary, this commit also adds the following mount options:
- rrw
- r[no]{suid,dev,exec,relatime,atime,strictatime,diratime,symfollow}
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
My GPG keys are not available inside the container, so it makes little
sense to try to sign the binaries inside the container's release.sh. The
solution is to split things into separate build and sign stages, with
signing ocurring after the in-Docker build.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
When writing netlink messages, it is possible to have a byte array
larger than UINT16_MAX which would result in the length field
overflowing and allowing user-controlled data to be parsed as control
characters (such as creating custom mount points, changing which set of
namespaces to allow, and so on).
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Since commit 7296dc1712, type intelRdtData is only used by tests,
and since commit 79d292b9f, its only member is config.
Change the test to use config directly, and remove the type.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This adds a new GHA CI job which runs a few extra linters. This is only
done for pull requests, and should only warn about new code.
The justification is simple: we want more linters, but since this is not
a new project, adding a new linter meaning we have to fix all the
existing warnings. In some cases having all the warnings fixed is
difficult and takes time, plus it is usually a low priority task.
Therefore, we are stuck with inability to add new linters because we
can't fix all their warnings. Meanwhile, new pull requests add more
code which is not linted.
This is an attempt to break this vicious cycle. Let's enable godot
and revive for now and see how it is going.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It never returns any error, so let's drop it (in case it needs to be
re-added, it is easy to do so).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those were never used (ctx was added by the initial commit, and
error was added by commit 25fd4a6757).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This function never returned anything other than nil, and its return
value is discarded since it is only called from defer.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 8850636eb3 (February 2015) this function is no longer
used (replaced by (*ConfigValidator).rootfs), so let's remove it,
together with its unit tests (which were added by commit 917c1f6d6 in
April 2016).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Delegating cgroups to the container enables more complex workloads,
including systemd-based workloads. The OCI runtime-spec was
recently updated to explicitly admit such delegation, through
specification of cgroup ownership semantics:
https://github.com/opencontainers/runtime-spec/pull/1123
Pursuant to the updated OCI runtime-spec, change the ownership of
the container's cgroup directory and particular files therein, when
using cgroups v2 and when the cgroupfs is to be mounted read/write.
As a result of this change, systemd workloads can run in isolated
user namespaces on OpenShift when the sandbox's cgroupfs is mounted
read/write.
It might be possible to implement this feature in other cgroup
managers, but that work is deferred.
Signed-off-by: Fraser Tweedale <ftweedal@redhat.com>
This is not used since commit 5e7b48f7c0 (23 Mar 2017).
In case there are external users, they should switch to
opencontainers/selinux.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Move test case comments to doc strings, and use t.Run.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is no need to run hooks when `Config.Hooks` is just an empty map,
(dlv) p p.config.Config.Hooks
github.com/opencontainers/runc/libcontainer/configs.Hooks []
Signed-off-by: Dave Chen <dave.chen@arm.com>
1. Instead of distinguishing between errors and warnings, let's treat all
errors as warnings, thus simplifying the code. This changes the
function behaviour for input like hugepages-BadNumberKb --
previously, the error from Atoi("BadNumber") was considered fatal,
now it's just another warnings.
2. Move the warning logging to HugePageSizes, thus simplifying the test
case, which no longer needs to read what logrus writes. Note that we
do not want to log all the warnings (as chances are very low we'll
get any, and if we do this means the code need to be updated), only
the first one.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I have noticed that libct/cg/fs allocates 8K during init on every runc
execution:
> init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.028 ms clock, 8512 bytes, 13 allocs
Apparently this is caused by global HugePageSizes variable init, which
is only used from GetStats (i.e. it is never used by runc itself).
Remove it, and use HugePageSizes() directly instead. Make it init-once,
so that GetStats (which, I guess, is periodically called by kubernetes)
does not re-read huge page sizes over and over.
This also removes 12 allocs and 8K from libct/cg/fs init section:
> $ time GODEBUG=inittrace=1 ./runc --help 2>&1 | grep cgroups/fs
> init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.003 ms clock, 16 bytes, 1 allocs
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Since GetHugePageSize do not have any external users (checked by
sourcegraph), and no internal user ever uses its second return value
(the error), let's drop it.
2. Rename GetHugePageSize -> HugePageSizes (drop the Get prefix as per
Go guidelines, add suffix since we return many sizes).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using null bytes as control characters for sending strings via netlink
opens us up to a user explicitly putting a null byte in a mount string
(which JSON will happily let you do) and then causing us to open a mount
path different to the one expected.
In practice this is more of an issue in an environment such as
Kubernetes where you may have path-based access control policies (which
are more susceptible to these kinds of flaws).
Found by Google Project Zero.
Fixes: 9c444070ec ("Open bind mount sources from the host userns")
Reported-by: Felix Wilhelm <fwilhelm@google.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
As reported in [1], in a case where read-only fuse (sshfs) mount
is used as a volume without specifying ro flag, the kernel fails
to remount it (when adding various flags such as nosuid and nodev),
returning EPERM.
Here's the relevant strace line:
> [pid 333966] mount("/tmp/bats-run-PRVfWc/runc.RbNv8g/bundle/mnt", "/proc/self/fd/7", 0xc0001e9164, MS_NOSUID|MS_NODEV|MS_REMOUNT|MS_BIND|MS_REC, NULL) = -1 EPERM (Operation not permitted)
I was not able to reproduce it with other read-only mounts as the source
(tried tmpfs, read-only bind mount, and an ext2 mount), so somehow this
might be specific to fuse.
The fix is to check whether the source has RDONLY flag, and retry the
remount with this flag added.
A test case (which was kind of hard to write) is added, and it fails
without the fix. Note that rootless user need to be able to ssh to
rootless@localhost in order to sshfs to work -- amend setup scripts
to make it work, and skip the test if the setup is not working.
[1] https://github.com/containers/podman/issues/12205
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The staticcheck linter points out that the err != nil comparison
after system.Exec is always true:
> libcontainer/standard_init_linux.go#L253
> SA4023: this comparison is always true (staticcheck)
> libcontainer/system/linux.go#L43
> SA4023(related information): github.com/opencontainers/runc/libcontainer/system.Exec never returns a nil interface value (staticcheck)
Indeed, Exec either returns an error or does not return at all.
Remove the (useless) check.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Decapitalize errors.
2. Rename isValidName to checkPropertyName.
3. Make it return a specific error.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit fb4c27c4b7 (went into v1.0.0-rc93) fixed a bug with
read-only tmpfs, but introduced a bug with read-only /dev.
This happens because /dev is a tmpfs mount and is therefore remounted
read-only a bit earlier than before.
To fix,
1. Revert the part of the above commit which remounts all tmpfs mounts
as read-only in mountToRootfs.
2. Reuse finalizeRootfs (which is already used to remount /dev
read-only) to also remount all ro tmpfs mounts that were previously
mounted rw in mountPropagate.
3. Remove the break in finalizeRootfs, as now we have more than one
mount to care about.
4. Reorder the if statements in finalizeRootfs to perform the fast check
(for ro flag) first, and compare the strings second. Since /dev is
most probably also a tmpfs mount, do the m.Device check first.
Add a test case to validate the fix and prevent future regressions;
make sure it fails before the fix:
✗ runc run [ro /dev mount]
(in test file tests/integration/mounts.bats, line 45)
`[ "$status" -eq 0 ]' failed
runc spec (status=0):
runc run test_busybox (status=1):
time="2021-11-12T12:19:48-08:00" level=error msg="runc run failed: unable to start container process: error during container init: error mounting \"devpts\" to rootfs at \"/dev/pts\": mkdir /tmp/bats-run-VJXQk7/runc.0Fj70w/bundle/rootfs/dev/pts: read-only file system"
Fixes: fb4c27c4b7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also, add a simple test and a benchmark (just out of sheer curiosity).
Benchmark results:
name old time/op new time/op delta
IsValidName-4 540ns ± 3% 45ns ± 1% -91.76% (p=0.008 n=5+5)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 1cd71dfd7 added isSecSuffix, but the same thing can be done
easily without a regex. This is faster and saves some init time and
memory.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
parseMountOption already returns way too many values, making the code
kind of hard to read.
Since all of the return values are used as is to populate the fields of
configs.Mount, let's change it to return (semi-)populated *configs.Mount
instead.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes the repeated calls to parseMountOptions faster,
and decreases the amount of garbage to collect.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These two maps are the same, except that mountPropagationMapping
has an extra element with key of "" and value of 0. Since the
code already checks for f != 0, this extra element is not a problem.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Eliminate some of these allocations when starting runc:
> init github.com/opencontainers/runc/libcontainer/specconv @10 ms, 0.11 ms clock, 5408 bytes, 70 allocs
Most of this (4K) is the two regexes, which are left intact for now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When I tried to start a rootless container under a different/wrong user,
I got:
$ ../runc/runc --systemd-cgroup --root /tmp/runc.$$ run 445
ERRO[0000] runc run failed: operation not permitted
This is obviously not good enough. With this commit, the error is:
ERRO[0000] runc run failed: fchown fd 9: operation not permitted
Alas, there are still some code that returns unwrapped errnos from
various unix calls.
This is a followup to commit d8ba4128b2 which wrapped many, but not
all, bare unix errors. Do wrap some more, using either os.PathError or
os.SyscallError.
While at it,
- use os.SyscallError instead of os.NewSyscallError;
- use errors.Is(err, os.ErrXxx) instead of os.IsXxx(err).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A bug in systemd-249.6-2.fc35.x86_64 prevents rootless containers from
start when systemd manager is used.
Apparently, "config exclude" is not working in F35 dnf shell either, so
use a workaround of specifying --exclude from the command line.
This should fix runc CI for the time being.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes a container cgroup already exists but is frozen.
When this happens, runc init hangs, and it's not clear what is going on.
Refuse to run in a frozen cgroup; add a test case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently runc allows multiple containers to share the same cgroup (for
example, by having the same cgroupPath in config.json). While such
shared configuration might be OK, there are some issues:
- When each container has its own resource limits, the order of
containers start determines whose limits will be effectively applied.
- When one of containers is paused, all others are paused, too.
- When a container is paused, any attempt to do runc create/run/exec
end up with runc init stuck inside a frozen cgroup.
- When a systemd cgroup manager is used, this becomes even worse -- such
as, stop (or even failed start) of any container results in
"stopTransientUnit" command being sent to systemd, and so (depending on
unit properties) other containers can receive SIGTERM, be killed after a
timeout etc.
Any of the above may lead to various hard-to-debug situations in production
(runc init stuck, cgroup removal error, wrong resource limits, init not
reaping zombies etc.).
One obvious solution is to refuse a non-empty cgroup when starting a new
container. This would be a breaking change though, so let's make it in
steps, with the first step is issue a warning and a deprecated notice
about a non-empty cgroup.
Later (in runc 1.2) we will replace this warning with an error.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, if a container is paused (i.e. its cgroup is frozen), runc exec
just hangs, and it is not obvious why.
Refuse to exec in a paused container. Add a test case.
In case runc exec in a paused container is a legit use case,
add --ignore-paused option to override the check. Document it,
add a test case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also rename `Vagrantfile.fedora%d` to `Vagrantfile.fedora` so that
we do not need to reset the commit log on upgrading the Fedora release.
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Some test directories are created using mktemp -d, and so they have
permissions set to 0700 and are thus inaccessible to a user inside
userns. This was workarounded for $ROOT in userns.bats before.
Now, when we have updated Cirrus CI config to use Fedora 35 (rather than
34), userns tests fail:
> runc run failed: unable to start container process: error during
> container init: error preparing rootfs: mount
> /tmp/bats-run-4pCERd/runc.f66gCC/bundle/rootfs:/tmp/bats-run-4pCERd/runc.f66gCC/bundle/rootfs,
> flags: 0x5000: permission denied
Fedora 34 image used kernel v5.11, while Fedora 35 has v5.15.
Apparently, the newer kernel also checks that the parent directories
are accessible by the user before doing mount.
Move the old workaround from userns.bats to helpers.bats, drop the r bit
(not needed), and add $BATS_RUN_TMPDIR (also created by mktemp -d) to
fix userns.bats test failures under Fedora 35.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The misc cgroup controller, introduced in Linux 5.13, is still unknown
to systemd, and thus it cannot delegate it. Add an appropriate fixup
to the test case, similar to an earlier commit 601cf5825f.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Runtime spec says:
> sysctl (object, OPTIONAL) allows kernel parameters to be modified at
> runtime for the container. For more information, see the sysctl(8)
> man page.
and sysctl(8) says:
> variable
> The name of a key to read from. An example is
> kernel.ostype. The '/' separator is also accepted in place of a '.'.
Apparently, runc config validator do not support sysctls with / as a
separator. Fortunately this is a one-line fix.
Add some more test data where / is used as a separator.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a unit test to check that bind mounts that have a part of its
path non accessible by others still work when using user namespaces.
To do this, we also modify newRoot() to return rootfs directories that
can be traverse by others, so the rootfs created works for all test
(either running in a userns or not).
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
Go 1.15 is not supported since Go 1.17 release (16 Aug 2021), and some
packages that we use already require Go 1.16+ (notably,
github.com/cilium/ebpf v0.7.0).
Let's require Go 1.16+.
Remove Go version requirement from README when describing dependencies,
since it is no longer needed:
$ GO=go1.15.15 make vendor
go1.15.15 mod tidy
go mod tidy: go.mod file indicates go 1.16, but maximum supported version is 1.15
make: *** [Makefile:141: vendor] Error 1
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Jobs verify/compile-buildtags and verify/deps relied on whatever Go
version is available from the Ubuntu-20.04 image, which seems to be
1.15.x).
Job test/cross-i386 was installing whatever Go version is considered to
be the default one by actions/setup-go@v2, which seems to be go 1.15.15
at the moment.
Fix all three jobs to install Go 1.x (which should translate to latest
stable Go version, i.e. 1.17.2 as of now).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The previous commit fixed an issue opening bind mount sources. This
commit just adds integration tests to make sure we don't regress on this
in the future.
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
The source of the bind mount might not be accessible in a different user
namespace because a component of the source path might not be traversed
under the users and groups mapped inside the user namespace. This caused
errors such as the following:
# time="2020-06-22T13:48:26Z" level=error msg="container_linux.go:367:
starting container process caused: process_linux.go:459:
container init caused: rootfs_linux.go:58:
mounting \"/tmp/busyboxtest/source-inaccessible/dir\"
to rootfs at \"/tmp/inaccessible\" caused:
stat /tmp/busyboxtest/source-inaccessible/dir: permission denied"
To solve this problem, this patch performs the following:
1. in nsexec.c, it opens the source path in the host userns (so we have
the right permissions to open it) but in the container mntns (so the
kernel cross mntns mount check let us mount it later:
https://github.com/torvalds/linux/blob/v5.8/fs/namespace.c#L2312).
2. in nsexec.c, it passes the file descriptors of the source to the
child process with SCM_RIGHTS.
3. In runc-init in Golang, it finishes the mounts while inside the
userns even without access to the some components of the source
paths.
Passing the fds with SCM_RIGHTS is necessary because once the child
process is in the container mntns, it is already in the container userns
so it cannot temporarily join the host mntns.
This patch uses the existing mechanism with _LIBCONTAINER_* environment
variables to pass the file descriptors from runc to runc init.
This patch uses the existing mechanism with the Netlink-style bootstrap
to pass information about the list of source mounts to nsexec.c.
Rootless containers don't use this bind mount sources fdpassing
mechanism because we can't setns() to the target mntns in a rootless
container (we don't have the privileges when we are in the host userns).
This patch takes care of using O_CLOEXEC on mount fds, and close them
early.
Fixes: #2484.
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
Export getIntelRdtRoot function as Root.
This is needed by google/cadvisor, which is (ab)using GetIntelRdtPath,
removed by commit 7296dc1712.
While at it, do some minimal refactoring to always use Root()
internally, not relying on variable value. Other than that it's just
some renaming.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If the container binary to be run is removed in between runc create
and runc start, the latter spits the following error:
> can't exec user process: no such file or directory
This is a bit confusing since we don't see what file is missing.
Wrap the unix.Exec error into os.PathError, like in many other cases,
to provide some context. Remove the error wrapping from
(*linuxStandardInit).Init as it is now redundant.
With this patch, the error is now:
> exec /bin/false: no such file or directory
Reported-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case hugetlb is not supported, GetStats() should not error out,
and yet it does.
Assume that if GetHugePageSize return an error, hugetlb is
not supported (this is what cgroup v1 manager do).
Fixes: 89a87adb
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Users would like to have the possibility to skip checks for their
tests the same way they are skipped within the tests in runc.
Not exposing this variable makes it very hard to test components
that use this library. To avoid copying-and-pasting the code
into outside projects this variable sould be exposed to the users.
Signed-off-by: Itamar Holder <iholder@redhat.com>
Commit f30244ee1b broke the scenario of using Dockefile for
anything but making a release. This happened because it installed
native libseccomp build to a temporary directory, and so linking against
libseccomp required setting a few environment variables.
Let's fix this, and simplify libseccomp installation. Instead of using
temporary directories, let's install native libseccomp to a specified
directory, all the cross-builds to its subdirectories, and set
PKG_CONFIG_PATH and LD_LIBRARY_PATH in Dockerfile so that the built
library will found by pkg-config and the dynamic linker (without setting
LD_LIBRARY_PATH, ld picks up distro-provided libseccomp.so).
While at it, fix some bugs introduced by the abovementioned commit.
This fixes building runc in make targets like shell, dbuild,
integration, unittest -- i.e. those that depend on runcimage.
Fixes: f30244ee1b
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, we can create subcgroup in a rootless container with systemd cgroupv2 on centos8.
But after the container exited, the container cgroup and its subcgroup will not be removed.
Fix this by removing all directories recursively.
Fixes: https://github.com/opencontainers/runc/issues/3225
Signed-off-by: Kang Chen <kongchen28@gmail.com>
Commit 7a0302f0d7 already removed "runc init" from runc help output,
as this is an internal option not supposed to be used by the end user.
Let's remove runc init completion, too.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In some setups, multiple cgroups are used inside a container,
and sometime there is a need to execute a process in a particular
sub-cgroup (in case of cgroup v1, for a particular controller).
This is what this commit implements.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Check that runc run and runc exec put the process on the same cgroups v2
when using hybrid mode.
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently the parent process of the container is moved to the right
cgroup v2 tree when systemd is using a hybrid model (last line with 0::):
$ runc --systemd-cgroup run myid
/ # cat /proc/self/cgroup
12:cpuset:/system.slice/runc-myid.scope
11:blkio:/system.slice/runc-myid.scope
10:devices:/system.slice/runc-myid.scope
9:hugetlb:/system.slice/runc-myid.scope
8:memory:/system.slice/runc-myid.scope
7:rdma:/
6:perf_event:/system.slice/runc-myid.scope
5:net_cls,net_prio:/system.slice/runc-myid.scope
4:freezer:/system.slice/runc-myid.scope
3:pids:/system.slice/runc-myid.scope
2:cpu,cpuacct:/system.slice/runc-myid.scope
1:name=systemd:/system.slice/runc-myid.scope
0::/system.slice/runc-myid.scope
However, if a second process is executed in the same container, it is
not moved to the right cgroup v2 tree:
$ runc exec myid /bin/sh -c 'cat /proc/self/cgroup'
12:cpuset:/system.slice/runc-myid.scope
11:blkio:/system.slice/runc-myid.scope
10:devices:/system.slice/runc-myid.scope
9:hugetlb:/system.slice/runc-myid.scope
8:memory:/system.slice/runc-myid.scope
7:rdma:/
6:perf_event:/system.slice/runc-myid.scope
5:net_cls,net_prio:/system.slice/runc-myid.scope
4:freezer:/system.slice/runc-myid.scope
3:pids:/system.slice/runc-myid.scope
2:cpu,cpuacct:/system.slice/runc-myid.scope
1:name=systemd:/system.slice/runc-myid.scope
0::/user.slice/user-1000.slice/session-8.scope
This commit makes that processes executed with exec are placed into the
right cgroup v2 tree. The implementation checks if systemd is using a
hybrid mode (by checking if cgroups v2 is mounted in
/sys/fs/cgroup/unified), if yes, the path of the cgroup v2 slice for
this container is saved into the cgroup path list.
The fs group driver has a similar issue, in this case none of the runc
run or runc exec commands put the process in the right cgroups v2. This
commit also fixes that.
Having the processes of the container in its own cgroup v2 is useful
for any BPF programs that rely on bpf_get_current_cgroup_id(), like
https://github.com/kinvolk/inspektor-gadget/ for instance.
[@kolyshkin: rebased]
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The function used here, cgroups.EnterPid, silently skips non-existing
paths, and it does not look like a good idea to do so for an existing
container with already configured cgroups.
Switch to cgroups.WriteCgroupProc which does not do that, so in case
a cgroup does not exist, we'll get an error.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
No need to add a file name to the error messages, as errors from
OpenFile and (*os.File).Write both contain the file name already.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin (5):
ci/gha: test criu-dev with latest go
ci/gha: remove debug info
CI/GHA: switch to OBS criu repo
Dockerfile: fix apt-key warning
Dockerfile: use Debian_11 repo for criu
LGTMs: mrunalp cyphar
As commits 120f74060 and a58718013 were added independently,
criu-dev go version was left at 1.16.x. Fix this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes
> Warning: apt-key is deprecated. Manage keyring files in trusted.gpg.d instead (see apt-key(8)).
Apparently, "the internets" disagree with the above, instead suggesting
using /usr/share/keyrings and a signed-by= declaration in sources.list.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The Debian_11 was not available in this repo at the time when commit 24d318b8b
was made, so we had to use Debian_10 URL for Debian 11 (apparently without any
consequences).
Now Debian_11 is available, so let's switch to it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is assumed that m.config is not nil, so these checks are redundant
(in case it is nil, NewManager panics and this code is unreachable).
Note that cgroups/manager.New checks that config is not nil.
Remove them.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Cgroup controllers should never panic, and yet sometimes they do.
Add a unit test to check that controllers never panic when called with
nil arguments and/or resources, and fix a few found cases.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
cgName and cgParent are only used when cgPath is empty, so move
their cleaning to the body of the appropriate "if" statement.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This way we
- won't re-initialize the paths if they were provided;
- will always have paths ready for every method.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is already documented but I guess more explanations (in particular,
why the path is being removed from paths) won't hurt.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Separate path initialization logic from Apply to initPaths,
and call initPaths from NewManager, so:
- we can error out early (in NewManager rather than Apply);
- always have m.paths available (e.g. in Destroy or Exists).
- do not unnecessarily call subsysPath from Apply in case
the paths were already provided.
2. Add a check for non-nil cgroups.Resources to NewManager,
since initPaths, as well as some controller's Apply methods,
need it.
3. Move cgroups.Resources.Unified check from Apply to NewManager,
so we can error out early (same check exists in Set).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Make Rootless and Systemd flags part of config.Cgroups.
2. Make all cgroup managers (not just fs2) return error (so it can do
more initialization -- added by the following commits).
3. Replace complicated cgroup manager instantiation in factory_linux
by a single (and simple) libcontainer/cgroups/manager.New() function.
4. getUnifiedPath is simplified to check that only a single path is
supplied (rather than checking that other paths, if supplied,
are the same).
[v2: can't -> cannot]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This implements cross-build for "make release", moving the build into a
container. This way we can support arm, arm64, ppc, and whatnot.
* script/seccomp.sh: separate out of script/release.sh, amend to support
cross-compile and save needed environment variables to a file.
* Dockerfile: add installing libseccomp from source, as this is needed
for release builds.
* script/release.sh: amend to support more architectures in addition to
the native build. Additional arches can be added by specifying
"-a <arch>" argument (can be specified multiple times), or
"make RELEASE_ARGS="-a arm64" release" if called via make.
All supported architectures can be enabled via "make releaseall".
* Makefile: move "release" target to "localrelease", add "release" and
"releaseall" targets to build via the Dockerfile. This is done because
most distros (including Fedora and openSUSE) lack cross-glibc, which is
needed to cross-compile libseccomp.
* Makefile: remove 'cross' and 'localcross' targets, as this is now done
by the release script.
* .github/workflows/validate.yum: amend the release CI job to cross-build
for supported architectures, remove cross job.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is no need to have a static version of recvtty and/or sd-helper
binary.
This speeds up script/release.sh a bit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. The seccompagent target it built in the same way as others in contrib,
so there is no need to have a separate rule.
2. Mark seccompagent as phony, because it is (it rarely happens, but I
actually just had an issue because this was absent).
3. Add seccompagent binary to clean target.
Fixes: e21a9ee81
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Wire through CRIU's support to change the mount context on restore.
This is especially useful if restoring a container in a different pod.
Single container restore uses the same SELinux process label and
same mount context as during checkpointing. If a container is being
restored into an existing pod the process label and the mount context
needs to be changed to the context of the pod.
Changing process label on restore is already supported by runc. This
patch adds the possibility to change the mount context.
Signed-off-by: Adrian Reber <areber@redhat.com>
runc delete -f is not working for a paused container, since in cgroup v1
SIGKILL does nothing if a process is frozen (unlike cgroup v2, in which
you can kill a frozen process with a fatal signal).
Theoretically, we only need this for v1, but doing it for v2 as well is
OK.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Dismantle and remove struct cgroupData. It contained three unrelated
entities (cgroup paths, pid, and resources), and made the code
harder to read. Most importantly, though, it is not needed.
Now, subsystems' Apply methods take path, resources, and pid.
To a reviewer -- the core of the changes is in fs.go and paths.go,
the rest of it is adapting to the new signatures and related test
changes.
2. Dismantle and remove struct cgroupTestUtil. This is a followup
to the previous item -- since cgroupData is gone, there is nothing
to hold in cgroupTestUtil. The change itself is very small (see
util_test.go), but this patch is big because of it -- mostly
because we had to replace helper.cgroup.Resources with
&config.Resources{}.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case c.Path is set, c.Name and c.Parent are not used, and so
calls to utils.CleanPath are entirely unnecessary. Move them to
inside of the "if" statement body.
Get rid of the intermediate cgPath variable, it is not needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Now fs.go is not very readable as its public API functions are
intermixed with internal stuff about getting cgroup paths.
Move that out to paths.go, without changing any code.
Same for the tests -- move paths-related tests to paths_test.go.
This commit is separate to make the review easier.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As ExpandSlice("system.slice") returns "/system.slice", there is no need
to call it for such paths (and the slash will be added by path.Join
anyway).
The same optimization was already done for v2 as part of commit
bf15cc99b1.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We were checking if a unit is a slice two times. Consolidate those
checks, and improve comments while we're at it.
The code is the same in v1 and v2 but it's too complicated to factor it
out, thus we just do the same changes in v1.go and v2.go.
No functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As the error may contain anything, it may not be clear to a user that
the whole (create or run) operation failed. Amend the errors.
Also, change the code flow in create to match that of run, so we don't
have to add the fake "return nil" at the end.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
All three callers* of startContainer call revisePidFile and createSpec
before calling it, so it makes sense to move those calls to inside of
the startContainer, and drop the spec argument.
* -- in fact restore does not call revisePidFile, but it should.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Error messages should not usually contain newlines.
Testing shows that the error runc delete prints is the same before and
after this commit:
[kir@kir-rhat runc-tst]$ sudo ../runc/runc delete xx3
ERRO[0000] cannot delete container xx3 that is not stopped: running
[kir@kir-rhat runc-tst]$
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit adds the config.json as generated by the script. Note that
the diff is minimal if you see this commit with "git show -w". The
differences are mostly whitespaces and some ordering.
We add a simple test that runs this and expects sucess.
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Currently, if the log level is not set to e.g. "debug", runc init sends
some debug logs to the parent, which parses and discards it.
It is better to not send those in the first place.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The code already parses an environment variable into an integer twice,
and we're about to add a third one.
Factor it out to getenv_int().
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes it possible to use bail() even if logging is not set up
(yet), so we don't have to think whether it's OK to use it or not.
In addition, this might help some unit tests that do not set log
forwarding.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of passing _LIBCONTAINER_LOGLEVEL as a string
(like "debug" or "info"), use a numeric value.
Also, simplify the init log level passing code -- since we actually use
the same level as the runc binary, just get it from logrus.
This is a preparation for the next commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Two new seccomp actions have been added to the libseccomp-golang
dependency, which can be now supported by runc, too.
ActKillThread kills the thread that violated the rule. It is the same as
ActKill. All other threads from the same thread group will continue to
execute.
ActKillProcess kills the process that violated the rule. All threads in
the thread group are also terminated. This action is only usable when
libseccomp API level 3 or higher is supported.
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
These are just boilerplate and are only really useful for the two
actions which require us to set a default errno/aux value (ActErrno and
ActTrace).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Kir Kolyshkin (6):
runc --debug: shorter caller info
libct/logs: do not show caller in nsexec logs
libct/logs: parse log level implicitly
libct/logs: test: make more robust
libct/logs: remove ConfigureLogging
init.go, main.go: don't use logs.ConfigureLogging
LGTMs: thaJeztah cyphar
Kir Kolyshkin (3):
libct/system: add I and P process states
libct/system.Stat: fix/improve/speedup
libct/system/proc_test: fix, improve, add benchmark
LGTMs: thaJeztah cyphar
Add functional test to check seccomp notify end-to-end. This test uses the
sample seccomp agent from the contrib/cmd folder.
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
Implement sample seccomp agent. It's also used in integration tests in
the following commit.
Instructions how to use it in contrib/cmd/seccompagent/README.md
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
Extend the SetupSeccomp tests by adding the following cases:
- Test nil config
- Test empty config
- Test bad action and architecture
- Test all possible actions
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
This commit implements support for the SCMP_ACT_NOTIFY action. It
requires libseccomp-2.5.0 to work but runc still works with older
libseccomp if the seccomp policy does not use the SCMP_ACT_NOTIFY
action.
A new synchronization step between runc[INIT] and runc run is introduced
to pass the seccomp fd. runc run fetches the seccomp fd with pidfd_get
from the runc[INIT] process and sends it to the seccomp agent using
SCM_RIGHTS.
As suggested by @kolyshkin, we also make writeSync() a wrapper of
writeSyncWithFd() and wrap the error there. To avoid pointless errors,
we made some existing code paths just return the error instead of
re-wrapping it. If we don't do it, error will look like:
writing syncT <act>: writing syncT: <err>
By adjusting the code path, now they just look like this
writing syncT <act>: <err>
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Rodrigo Campos <rodrigo@kinvolk.io>
SendFds is a helper function for sending a set of file descriptors and a message
over a unix domain socket.
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
if function returns error before WriteJSON defer, error will not be
printed out, so move this defer as early as possible and use logrus to
print out error if returns before it.
Signed-off-by: xiadanni <xiadanni1@huawei.com>
openSUSE comes with site-config package, which makes configure select
${prefix}/lib64 as libdir on x86_64, unless explicitly specified.
Since release.sh relies on a particular libdir path (for pkgconfig), it
breaks things:
> + make -C /home/kir/git/runc PKG_CONFIG_PATH=/tmp/tmp.QgIJ1sR5c9/lib/pkgconfig COMMIT_NO= EXTRA_FLAGS=-a 'EXTRA_LDFLAGS=-w -s -buildid=' static
> make[1]: Entering directory '/home/kir/git/runc'
> CGO_ENABLED=1 go build -trimpath -a -tags "seccomp netgo osusergo" -ldflags "-extldflags -static -X main.gitCommit=v1.0.0-204-g963e0146 -X main.version=1.0.0+dev -w -s -buildid=" -o runc .
> Package libseccomp was not found in the pkg-config search path.
> Perhaps you should add the directory containing `libseccomp.pc'
To fix, we have to explicitly specify libdir.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 9f3d7534ea enabled logrus to show information about log
caller, if --debug is set.
The problem is, the file name and in many cases the function name have a
long prefix of github.com/opencontainers/runc (this is with -trimpath,
and without it it's worse).
Add a function to trim the prefix.
Note all this happens only when --debug is given.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 9f3d7534ea enabled logrus to show information about log
caller, if --debug is set. It is helpful in many scenarios, but does
not work very well when we are debugging runc init, for example:
# runc --debug run -d xx4557
DEBU[0000]libcontainer/logs/logs.go:45 github.com/opencontainers/runc/libcontainer/logs.processEntry() nsexec[279687]: logging set up
DEBU[0000]libcontainer/logs/logs.go:45 github.com/opencontainers/runc/libcontainer/logs.processEntry() nsexec[279687]: logging set up
DEBU[0000]libcontainer/logs/logs.go:45 github.com/opencontainers/runc/libcontainer/logs.processEntry() nsexec[279687]: => nsexec container setup
DEBU[0000]libcontainer/logs/logs.go:45 github.com/opencontainers/runc/libcontainer/logs.processEntry() nsexec[279687]: update /proc/self/oom_score_adj to '30'
As we're merely forwarding the logs here, printing out filename:line and
function is useless and clutters the logs a log.
To fix, create and use a copy of the standard logger with caller info
turned off.
With this in place, nsexec logs are sane again:
# runc --debug --log-format=text run -d xe34
DEBU[0000] nsexec[293595]: logging set up
DEBU[0000] nsexec[293595]: logging set up
DEBU[0000] nsexec[293595]: => nsexec container setup
DEBU[0000] nsexec[293595]: update /proc/self/oom_score_adj to '30'
This patch also changes Logf to Log in processEntry, as this is what it
should be.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There's no need to call logrus.ParseLevel as logrus.Level already
implements encoding.TextUnmarshaler interface.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When playing with the log forwarder, I broke it, but all the units tests
were still passing. This happened because test cases were merely looking
for a word (like "kitten") in the log output, which also happened to be
there in case of an error (as a part of an error message produced by log
forwarder).
Make the test a bit more robust by
- looking for a complete log message, not just part of it;
- also checking that log file does NOT contain errors.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
- add missing colons before error message;
- unify error messages after cmd.Start and cmd.Wait, so that they show
context and the error itself.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of reading a single message, do read all the logs from the init,
and use DisallowUnknownFields for stricter checking.
While at it, use reapChildren to reap zombies (and add an extra check).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The test was not working since at least commit 64bb59f592
renamed pid to stage2_pid (or maybe even earlier), so the pid
was never received (i.e. pid.Pid was 0).
The problem was not caught because os.FindProcess never return an error
on Unix.
Factor out and fix pid decode function:
- use DisallowUnknownInput to get error if JSON will be changed;
- check pids to make sure they are valid
- and use unix.Wait4 to reap zombies.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Make sure we close all file descriptors at the end of the test.
2. Make sure we close child fds after the start.
3. Use newPipe for logs as well, for simplicity and uniformity.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Treat warning as errors only in the CI. We can enforce it in the source
code (like setting CFLAGS in libcontainer/nsenter/nsenter.go), but that
can force other downstream to patch the code if thei C compiler produces
warnings. For that reason, we do it only on the CI.
Todays CGO warnings are quite hidden in the CI (only shown for the
compilation step, that is collapsed) and CI is green anyways. With this
patch, CI fails if a warning is introduced.
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Go 1.17 introduce this new (and better) way to specify build tags.
For more info, see https://golang.org/design/draft-gobuild.
As a way to seamlessly switch from old to new build tags, gofmt (and
gopls) from go 1.17 adds the new tags along with the old ones.
Later, when go < 1.17 is no longer supported, the old build tags
can be removed.
Now, as I started to use latest gopls (v0.7.1), it adds these tags
while I edit. Rather than to randomly add new build tags, I guess
it is better to do it once for all files.
Mind that previous commits removed some tags that were useless,
so this one only touches packages that can at least be built
on non-linux.
Brought to you by
go1.17 fmt ./...
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These are not needed as these packages (libcontainer/cgroups,
libcontainer/cgroups/fs, and libcontainer/cgroups/systemd) can
not be built under non-linux anyway (for various reasons).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Only some libcontainer packages can be built on non-linux platforms
(not that it make sense, but at least go build succeeds). Let's call
these "good" packages.
For all other packages (i.e. ones that fail to build with GOOS other
than linux), it does not make sense to have linux build tag (as they
are broken already, and thus are not and can not be used on anything
other than Linux).
Remove linux build tag for all non-"good" packages.
This was mostly done by the following script, with just a few manual
fixes on top.
function list_good_pkgs() {
for pkg in $(find . -type d -print); do
GOOS=freebsd go build $pkg 2>/dev/null \
&& GOOS=solaris go build $pkg 2>/dev/null \
&& echo $pkg
done | sed -e 's|^./||' | tr '\n' '|' | sed -e 's/|$//'
}
function remove_tag() {
sed -i -e '\|^// +build linux$|d' $1
go fmt $1
}
SKIP="^("$(list_good_pkgs)")"
for f in $(git ls-files . | grep .go$); do
if echo $f | grep -qE "$SKIP"; then
echo skip $f
continue
fi
echo proc $f
remove_tag $f
done
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was added by commit 5aa82c950 back in the day when we thought
runc is going to be cross-platform. It's very clear now it's Linux-only
package.
While at it, further clarify it in README that we're Linux only.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For files that end with _linux.go or _linux_test.go, there is no need to
specify linux build tag, as it is assumed from the file name.
In addition, rename libcontainer/notify_linux_v2.go -> libcontainer/notify_v2_linux.go
for the file name to make sense.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test case for an issue fixed by the previous commit.
Unfortunately, this is somewhat complicated as there's no easy way to
create a transient unit, so a binary, sd-helper, had to be added. On top
of that, an ability to create a parent/pod cgroup is added to
helpers.bash, which might be useful for future integration tests.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As reported in issue 3084, sometimes setting CPU quota period fails
when a new period is lower and a parent cgroup has CPU quota limit set.
This happens as in cgroup v1 the quota and the period can not be set
together (this is fixed in v2), and since the period is being set first,
new_limit = old_quota/new_period may be higher than the parent cgroup
limit.
The fix is to retry setting the period after the quota, to cover all
possible scenarios.
Add a test case to cover a regression caused by an earlier version of
this patch (ignoring a failure of setting invalid period when quota is
not set).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This reverts commit 814f3ae1d9. This
changed the on-disk state which breaks runc when it has to operate on
containers started with an older runc version. Working around this is
far more complicated than just reverting it.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
ConfigError was added by commit e918d02139, while removing runc own
error system, to preserve a way for a libcontainer user to distinguish
between a configuration error and something else.
The way ConfigError is implemented requires a different type of check
(compared to all other errors defined by error.go). An attempt was made
to rectify this, but the resulting code became even more complicated.
As no one is using this functionality (of differentiating a "bad config"
type of error from other errors), let's just drop the ConfigError type.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
All the errors returned from Validate should tell about a configuration
error. Some were lacking a context, so add it.
While at it, fix abusing fmt.Errorf and logrus.Warnf where the argument
do not contain %-style formatting.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Looking into data generated by setting
GODEBUG="inittrace=1"
I have noticed this line:
init github.com/opencontainers/runc/libcontainer/cgroups/devices @1.2 ms, 0.020 ms clock, 10512 bytes, 133 allocs
This is the leader for both bytes and allocs among the packages from
this repo, and all of it is caused by a single regex:
> var devicesListRegexp = regexp.MustCompile(`^([abc])\s+(\d+|\*):(\d+|\*)\s+([rwm]+)$`)
It seems that the same parsing can be done without relying on
a regular expression, no decrease in readability, and 2x faster
(according to the benchmark added), and also makes runc start
slightly faster and leaner.
Before:
BenchmarkParseLine-4 176240 6768 ns/op 6576 B/op 64 allocs/op
After:
BenchmarkParseLine-4 322441 3535 ns/op 5520 B/op 53 allocs/op
[v2: single split with SplitFunc; fix a typo in error message]
[v3: rebase after 3159 merge; re-ran benchmarks (results are similar)]
Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... and add the file to be checked by shellcheck.
The warnings fixed are:
In tests/integration/helpers.bash line 10:
INTEGRATION_ROOT=$(dirname "$(readlink -f "$BASH_SOURCE")")
^----------^ SC2128: Expanding an array without an index only gives the first element.
In tests/integration/helpers.bash line 22:
TESTDATA="${INTEGRATION_ROOT}/testdata"
^------^ SC2034: TESTDATA appears unused. Verify use (or export if used externally).
In tests/integration/helpers.bash line 42:
echo "runc $@ (status=$status):" >&2
^-- SC2145: Argument mixes string and array. Use * or separate argument.
^-----^ SC2154: status is referenced but not assigned.
In tests/integration/helpers.bash line 43:
echo "$output" >&2
^-----^ SC2154: output is referenced but not assigned.
In tests/integration/helpers.bash line 77:
| .linux.gidMappings += [{"hostID": '"$(($ROOTLESS_GIDMAP_START + 10))"', "containerID": 1, "size": 20}]
^--------------------^ SC2004: $/${} is unnecessary on arithmetic variables.
In tests/integration/helpers.bash line 78:
| .linux.gidMappings += [{"hostID": '"$(($ROOTLESS_GIDMAP_START + 100))"', "containerID": 1000, "size": '"$(($ROOTLESS_GIDMAP_LENGTH - 1000))"'}]'
^--------------------^ SC2004: $/${} is unnecessary on arithmetic variables.
^---------------------^ SC2004: $/${} is unnecessary on arithmetic variables.
In tests/integration/helpers.bash line 125:
base_path=$(gawk '$(NF-2) == "cgroup" && $NF ~ /\<'${g}'\>/ { print $5; exit }' /proc/self/mountinfo)
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
base_path=$(gawk '$(NF-2) == "cgroup" && $NF ~ /\<'"${g}"'\>/ { print $5; exit }' /proc/self/mountinfo)
In tests/integration/helpers.bash line 127:
eval CGROUP_${g^^}_BASE_PATH="${base_path}"
^----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
eval CGROUP_"${g^^}"_BASE_PATH="${base_path}"
In tests/integration/helpers.bash line 229:
if [ "x$CGROUP_UNIFIED" = "xyes" ]; then
^----------------^ SC2268: Avoid x-prefix in comparisons as it no longer serves a purpose.
Did you mean:
if [ "$CGROUP_UNIFIED" = "yes" ]; then
In tests/integration/helpers.bash line 234:
eval cgroup=\$${var}${REL_CGROUPS_PATH}
^----^ SC2086: Double quote to prevent globbing and word splitting.
^-----------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
eval cgroup=\$"${var}""${REL_CGROUPS_PATH}"
In tests/integration/helpers.bash line 236:
cat $cgroup/$source
^-----^ SC2086: Double quote to prevent globbing and word splitting.
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
cat "$cgroup"/"$source"
In tests/integration/helpers.bash line 242:
current="$(get_cgroup_value $1)"
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
current="$(get_cgroup_value "$1")"
In tests/integration/helpers.bash line 245:
echo "current" $current "!?" "$expected"
^------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
echo "current" "$current" "!?" "$expected"
In tests/integration/helpers.bash line 257:
[ $(id -u) != "0" ] && user="--user"
^------^ SC2046: Quote this to prevent word splitting.
In tests/integration/helpers.bash line 259:
current=$(systemctl show $user --property $source $SD_UNIT_NAME | awk -F= '{print $2}')
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
current=$(systemctl show $user --property "$source" $SD_UNIT_NAME | awk -F= '{print $2}')
In tests/integration/helpers.bash line 261:
[ "$current" = "$expected" ] || [ -n "$expected2" -a "$current" = "$expected2" ]
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tests/integration/helpers.bash line 309:
check_cgroup_value "cpu.weight" $weight
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
check_cgroup_value "cpu.weight" "$weight"
In tests/integration/helpers.bash line 310:
check_systemd_value "CPUWeight" $weight
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
check_systemd_value "CPUWeight" "$weight"
In tests/integration/helpers.bash line 383:
if [ $CGROUP_UNIFIED = "no" -a ! -e "${CGROUP_MEMORY_BASE_PATH}/memory.memsw.limit_in_bytes" ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tests/integration/helpers.bash line 412:
local cpu_count=$(grep -c '^processor' /proc/cpuinfo)
^-------^ SC2155: Declare and assign separately to avoid masking return values.
In tests/integration/helpers.bash line 450:
sleep $delay
^----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
sleep "$delay"
In tests/integration/helpers.bash line 453:
echo "Command \"$@\" failed $attempts times. Output: $output"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tests/integration/helpers.bash line 471:
runc state $1
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
runc state "$1"
In tests/integration/helpers.bash line 472:
if [ $2 == "checkpointed" ]; then
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if [ "$2" == "checkpointed" ]; then
In tests/integration/helpers.bash line 484:
mkdir $dir
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
mkdir "$dir"
In tests/integration/helpers.bash line 497:
kill -9 $(cat "$dir/pid")
^---------------^ SC2046: Quote this to prevent word splitting.
In tests/integration/helpers.bash line 508:
export ROOT=$(mktemp -d "$BATS_RUN_TMPDIR/runc.XXXXXX")
^--^ SC2155: Declare and assign separately to avoid masking return values.
In tests/integration/helpers.bash line 512:
cd "$ROOT/bundle"
^---------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
Did you mean:
cd "$ROOT/bundle" || exit
In tests/integration/helpers.bash line 535:
cd "$INTEGRATION_ROOT"
^--------------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
Did you mean:
cd "$INTEGRATION_ROOT" || exit
For more information:
https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
https://www.shellcheck.net/wiki/SC2034 -- TESTDATA appears unused. Verify u...
https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is not used since PR 2757, as all tests are run with cd to bundle
directory.
runc_spec argument count checking is removed since otherwise shellcheck
complains:
> SC2120: runc_spec references arguments, but none are ever passed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Previous commits removed all its users -- the only one left is package's
own unit tests.
Modify those unit tests to configure logrus directly, and remove
ConfigureLogging for good. The world is better without it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This function is somewhat strange and I always wanted to remove it,
as it tries to satisfy both init.go and main.go, which have somewhat
different needs.
It is more straightforward and readable to configure logrus directly.
While at it, simplify errors on panic (errors from logrus.ParseLevel
and strconv.Atoi already contain value which they fail to parse, and
panic already contains enough context to figure out what's wrong).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use the term "clos group" instead of "container_id group" as the group
that a container belongs to is not necessarily tied to its container id.
Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>
Those states are available since Linux 4.14 (kernel commits
8ef9925b02c23e3838d5 and 06eb61844d841d003). Before this
patch, they were shown as unknown.
This is mostly cosmetical.
Note that I is described in /proc/pid/status as just "idle", although
elsewhere it says it's an idle kernel thread. Let's have it as "idle"
for brevity.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Remove PID field as it is useless.
2. Rewrite parseStat() to make it faster and more correct:
- do not use fmt.Scanf as it is very slow;
- avoid splitting data into 20+ fields, of which we only need 2;
- make sure to not panic on short lines and other bad input;
- add some bad input tests (some fail with old code);
- use LastIndexByte instead of LastIndex.
Benchmarks:
before (from the previous commit message):
> BenchmarkParseStat-4 116415 10804 ns/op
> BenchmarkParseRealStat-4 240 4781769 ns/op
after:
> BenchmarkParseStat-4 1164948 1068 ns/op
> BenchmarkParseRealStat-4 331 3458315 ns/op
We are seeing 10x speedup in a synthetic benchmark, and about 1.4x
speedup in a real world benchmark.
While at it, do not ignore any possible errors, and properly wrap those.
[v2: use pkg/errors more, remove t.Logf from test]
[v3: rebased; drop pkg/errors; gofumpt'ed]
[v4: rebased; improved description]
[v5: rebased; mention bad input tests, added second benchmark results]
[v6: remove PID field, do not use strings.Split, further speedup]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Add a test case that tests parentheses in command.
2. Replace individual comparisons with reflect.DeepEqual.
This also fixes wrong %-style types in Fatalf statements.
3. Replace Fatalf with Errorf so we don't bail out on the first
failure, and do not check result on error.
4. Add two benchmarks. On my laptop, they show:
BenchmarkParseStat-4 116415 10804 ns/op
BenchmarkParseRealStat-4 240 4781769 ns/op
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
runc resolves symlink before doing bind mount. So
we should save original path while formatting CriuReq for
dump and restore.
"checkpoint: resolve symlink for external bind mount" is merged as
da22625f6986f0ef196eaa1f8bb6adce098f0fb7(PR 2902) previously. And reverted
in commit 70fdc0573dced3464e9c31d674559f77c1de3973(PR 3043) duo to behavior changes
caused by commit 0ca91f44f1664da834bc61115a849b56d22f595f(Fixes: CVE-2021-30465)
Signed-off-by: Liu Hua <weldonliu@tencent.com>
This is helpful to kubernetes in cases it knows for sure that the freeze
is not required (since it created the systemd unit with no device
restrictions).
As the code is trivial, no tests are required.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test for freezeBeforeSet, checking various scenarios including
those that were failing before the fix in the previous commit.
[v2: add more cases, add a check before creating a unit.]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes the behavior intended to avoid freezing containers/control
groups without it being necessary. This is important for end users of
libcontainer who rely on the behavior of no freeze.
The previous implementation would always get error trying to get
DevicePolicy from the Unit via dbus, since the Unit interface doesn't
contain DevicePolicy.
Signed-off-by: Odin Ugedal <odin@uged.al>
The two exceptions I had to add to codespellrc are:
- CLOS (used by intelrtd);
- creat (syscall name used in tests/integration/testdata/seccomp_*.json).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 2bab4a5 resulted in a warning from gcc:
nsexec.c: In function ‘write_log’:
nsexec.c:171:2: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result]
171 | write(logfd, json, ret);
| ^~~~~~~~~~~~~~~~~~~~~~~
As there's nothing we can or want to do in case write fails,
let's just tell the compiler we're not going to use it.
Fixes: 2bab4a5
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Today we support the seccomp build tag only that is used by default.
However, we are not testing that compiling without any build tag works.
I found the CI didn't catch this when working on #2682, that the CI was
green but compilation without build tags was broken.
We test compilation without build tags only, compilation with the only
build tag supported is done extensively in other actions.
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
The contents of the pointer returned on asprintf() error are undefined
i.e., it can be anything there. We set it to NULL on error so that
free() afterwards won't get a garbage pointer.
This patch applies the above to message and stage as well to be
consistent with what we do for json.
Signed-off-by: Kailun Qin <kailun.qin@intel.com>
According to C standards, `size_t` is always an unsigned integer type.
Thus, checking unsigned expressions to be less than zero is not needed.
Signed-off-by: Kailun Qin <kailun.qin@intel.com>
Possibly there was a specific reason to use a rune for this, but I noticed
that there's various parts in the code that has to convert values from a
string to this type. Using a string as type for this can simplify some of
that code.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
GCP images description at [1] claims that:
- For CentOS 8 and CentOS Stream 8, the PowerTools repository is
enabled.
- For CentOS 7, EPEL is enabled.
Apparently,
- we do not need epel for centos-stream-8;
- powertools is not enabled on centos-stream-8 despite [1].
Anyway, the less yum commands the better, as we have seen those fail
sometimes due to occasional networking problems etc.
[1] https://cloud.google.com/compute/docs/images/os-details#centos
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
opencontainers/runc issue 3026 describes a scenario in which OpenFile
failed to open a legitimate existing cgroupfs file. Added debug
(similar to what this commit does) shown that cgroupFd is no longer
opened to "/sys/fs/cgroup", but to "/" (it's not clear what caused it,
and the source code is not available, but they might be using the same
process on the both sides of the container/chroot/pivot_root/mntns
boundary, or remounting /sys/fs/cgroup).
Consider such use incorrect, but give a helpful hint as two what is
going on by wrapping the error in a more useful message.
NB: this can potentially be fixed by reopening the cgroupFd once we
detected that it's screwed, and retrying openat2. Alas I do not have
a test case for this, so left this as a TODO suggestion.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As reported in issue 3119, there is a race in nsexec logging
that can lead to garbled json received by log forwarder, which
complains about it with a "failed to decode" error.
This happens because dprintf (used since the very beginning of nsexec
logging introduced in commit ba3cabf932) relies on multiple write(2)
calls, and with additional logging added by 64bb59f592 a race is
possible between runc init parent and its children.
The fix is to prepare a string and write it using a single call to
write(2).
[v2: NULLify json on error from asprintf]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin (3):
libct/cg: GetAllPids: optimize for go 1.16+
libct/cg: improve GetAllPids and readProcsFile
libct/cg: move GetAllPids out of utils.go
LGTMs: AkihiroSuda cyphar
Closes#3133
Fix reading cgroup files from the top cgroup directory, i.e.
/sys/fs/cgroup.
The code was working for for any subdirectory of /sys/fs/cgroup, but
for dir="/sys/fs/cgroup" a fallback (open and fstatfs) was used, because
of the way the function worked with the dir argument.
Fix those cases, and add unit tests to make sure they work. While at it,
make the rules for dir and name components more relaxed, and add test
cases for this, too.
While at it, improve OpenFile documentation, and remove a duplicated
doc comment for openFile.
Without these fixes, the unit test fails the following cases:
file_test.go:67: case {path:/sys/fs/cgroup name:cgroup.controllers}: fallback
file_test.go:67: case {path:/sys/fs/cgroup/ name:cgroup.controllers}: openat2 /sys/fs/cgroup//cgroup.controllers: invalid cross-device link
file_test.go:67: case {path:/sys/fs/cgroup/ name:/cgroup.controllers}: openat2 /sys/fs/cgroup///cgroup.controllers: invalid cross-device link
file_test.go:67: case {path:/ name:/sys/fs/cgroup/cgroup.controllers}: fallback
file_test.go:67: case {path:/ name:sys/fs/cgroup/cgroup.controllers}: fallback
file_test.go:67: case {path:/sys/fs/cgroup/cgroup.controllers name:}: openat2 /sys/fs/cgroup/cgroup.controllers/: not a directory
Here "fallback" means openat2-based implementation fails, and the fallback code
is used (and works).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Run unit tests irrespective of the underlying system configuration, i.e.
even if RDT has not been enabled or is not supported. The tests do not
depend on real kernel interfaces.
Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>
Check that the ClosID directory pre-exists if no L3 or MB schema has
been specified. Conform with the following line from runtime-spec
(config-linux):
If closID is set, and neither of l3CacheSchema and memBwSchema are
set, runtime MUST check if corresponding pre-configured directory
closID is present in mounted resctrl. If such pre-configured directory
closID exists, runtime MUST assign container to this closID and
generate an error if directory does not exist.
Add a TODO note for verifying existing schemata against L3/MB
parameters.
Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>
Handle ClosID parameter of IntelRdt. Makes it possible to use
pre-configured classes/ClosIDs and avoid running out of available IDs
which easily happens with per-container classes.
Remove validator checks for empty L3CacheSchema and MemBwSchema fields
in order to be able to leave them empty, and only specify ClosID for
a pre-configured class.
Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>
This was initially added by commits 41d9d26513 and 4a8f0b4db4,
apparently to implement docker run --cgroup container:ID, which was
never merged. Therefore, this code is not and was never used.
It needs to be removed mainly because having it makes it much harder to
understand how cgroup manager works (because with this in place we have
not one or two but three sets of cgroup paths to think about).
Note if the paths are known and there is a need to add a PID to existing
cgroup, cgroup manager is not needed at all -- something like
cgroups.WriteCgroupProc or cgroups.EnterPid is sufficient (and the
latter is what runc exec uses in (*setnsProcess).start).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
filepath.WalkDir function, introduced in Go 1.16, doesn't do stat(2)
on every entry, and is therefore somewhat faster (see below).
Since we have to support Go 1.15, keep the old version for backward
compatibility.
Add a quick benchmark, which shows approximately 3x improvement:
$ go1.15.15 test -bench AllPid -run xxx .
BenchmarkGetAllPids-4 48 23528839 ns/op
$ go version
go version go1.16.6 linux/amd64
$ go test -bench AllPid -run xxx .
BenchmarkGetAllPids-4 147 7700170 ns/op
(Unrelated but worth noting -- go 1.17rc2 is pushing it even further)
$ go1.17rc2 test -bench AllPid -run xxx .
BenchmarkGetAllPids-4 164 6820994 ns/op
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since every cgroup directory is guaranteed to have cgroup.procs file,
we don't have to do filename comparison in GetAllPids() and just read
cgroup.procs in every directory.
While at it, switch readProcsFile to use our own OpenFile.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add testing against criu-dev branch instead of a released version
(happens to be criu v3.15 at the moment), to check how it works.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently there's no way to distinguish between the two cases:
- runc exec failed;
- the command executed returned 1.
This was possible before commit 8477638aab, as runc exec exited with
the code of 255 if exec itself has failed. The code of 255 is the same
convention as used by e.g. ssh.
Re-introduce the feature, document it, and add some tests so it won't be
broken again.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
What it takes is add an empty buildid, which, together with previously
added strip invocation, results in reproducible build!
NB: earlier versions of this patch also added the following:
1. non-random libseccomp install $prefix;
2. "objcopy --enable-deterministic-archives $prefix/lib/libseccomp.a"
to strip ar dates and UIDs/GIDs;
3. "-B=0x00" to EXTRA_LDFLAGS to have non-variable NT_GNU_BUILD_ID.
Apparently, all this is not needed with strip.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This patch
* drops the default `-w` flag for `make static`, which helps with
debugging the static runc binary;
* adds `EXTRA_LDFLAGS="-w -s"` to `script/release.sh` to disable DWARF
generation and symbol table for the release runc binary;
* adds strip in `script/release.sh` for a further size-optimized release
runc binary.
Signed-off-by: Kailun Qin <kailun.qin@intel.com>
As pointed out in TODO item added by commit 64bb59f59, it is not
necessary to have a special sync mechanism for cgroupns, as the parent
adds runc init to cgroup way earlier (before sending nl bootstrap data.
This sync was added by commit df3fa115f9, which was also added a
second cgroup manager.Apply() call, later removed in commit
d1ba8e39f8. It seems the original author had the idea to wait for
that second Apply().
Fixes: df3fa115f9
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
runc init is special. For one thing, it needs to do a few things before
main(), so we have func init() that checks if we're init and does that.
What happens next is main() is called, which does some options parsing,
figures out it needs to call initCommand.Action and so it does.
Now, main() is entirely unnecessary -- we can do everything right from
init().
Hopefully the change makes things slightly less complicated.
From a user's perspective, the only change is runc help no longer
lists 'runc init` (which I think it also good).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Replace ioutil.TempDir (mostly) with t.TempDir, which require no
explicit cleanup.
While at it, fix incorrect usage of os.ModePerm in libcontainer/intelrdt
test. This is supposed to be a mask, not mode bits.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Use t.TempDir instead of ioutil.TempDir. This means no need for an
explicit cleanup, which removes some code, including newTestBundle
and newTestRoot.
2. Move newRootfs invocation down to newTemplateConfig, removing a need
for explicit rootfs creation. Also, remove rootfs from tParam as it
is no longer needed (there was a since test case in which two
containers shared the same rootfs, but it does not look like it's
required for the test).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes using runc with podman on my system (Fedora 34).
> $ podman --runtime `pwd`/runc run --rm --memory 4M fedora echo it works
> Error: unable to start container process: error adding seccomp filter rule for syscall bdflush: permission denied: OCI permission denied
The problem is, libseccomp returns EPERM when a redundant rule (i.e. the
rule with the same action as the default one) is added, and podman (on
my machine) sets the following rules in config.json:
<....>
"seccomp": {
"defaultAction": "SCMP_ACT_ERRNO",
"architectures": [
"SCMP_ARCH_X86_64",
"SCMP_ARCH_X86",
"SCMP_ARCH_X32"
],
"syscalls": [
{
"names": [
"bdflush",
"io_pgetevents",
<....>
],
"action": "SCMP_ACT_ERRNO",
"errnoRet": 1
},
<....>
(Note that defaultErrnoRet is not set, but it defaults to 1).
With this commit, it works:
> $ podman --runtime `pwd`/runc run --memory 4M fedora echo it works
> it works
Add an integration test (that fails without the fix).
Similar crun commit:
* https://github.com/containers/crun/commit/08229f3fb904c5ea19a7d9
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Rather than silently ignoring unknown syscalls, print a warning.
While at it, fix imports ordering (stdlib, others, ours).
[v2: demote Warn to Debug]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As Cirrus CI does not provide a real terminal this uses the same
'ssh -tt' workaround as the Vagrant setup. This sets up the
CentOS 7 and 8 to allow SSH as root to localhost so that we can run
all the tests via 'ssh -tt'.
Not going through vagrant reduces CI times for CentOS 7 and 8 from 6
minutes to 4 minutes.
Signed-off-by: Adrian Reber <areber@redhat.com>
Without this, the test case fails with
> Writing 1000000 to /sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/cpu.rt_period_us
> /tmp/bats-run-106836/bats.116418.src: line 548: /sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/cpu.rt_period_us: Permission denied
Since we do not currently have a setup to test this, this went
unnoticed (can be seen in RHEL8 though).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
Since commit f09a3e1b8d, the value passed on to read starts with
a slash, resulting in the first element of the array to be empty.
As a result, the test tries to write to the top-level cgroup, which
fails when rootless:
> # Writing 1000000 to /sys/fs/cgroup/cpu,cpuacct//cpu.rt_period_us
> # /tmp/bats-run-106184/bats.115768.src: line 548: /sys/fs/cgroup/cpu,cpuacct//cpu.rt_period_us: Permission denied
To fix, remove the leading slash.
An alternative fix would be to do "for ((i = 1;" instead of "i = 0", but
that seems less readable.
Fixes: f09a3e1b8d
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
"`runc` X.Y.Z should implement the X.Y version of the specification." is no longer correct.
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
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>
In addition to freezing and thawing a container via Pause/Resume,
there is a way to also do so via Set.
This way was broken though and is being fixed by a few preceding
commits. The test is added to make sure this is fixed and won't regress.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The t.Name() usage in libcontainer/integration prevented subtests
to be used, since in such case it returns a string containing "/",
and thus it can't be used to name a container.
Fix this by replacing slashes with underscores where appropriate.
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>
We need to update the eBPF library so that we can get the raw syscall
errors from bpf(2) syscalls using errors.Is.
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>
Run device update tests on cgroup v2, and add a test verifying that we
don't allow access to devices when we don't intend to.
Signed-off-by: Odin Ugedal <odin@uged.al>
Add CAP_SYSLOG to ensure that /dev/kmsg can be accesses on systems where
the sysctl kernel.dmesg_restrict = 1.
Signed-off-by: Odin Ugedal <odin@uged.al>
The main change is the switch to Go 1.13-style "%w" error wrapping,
dropping one of the github.com/pkg/errors dependencies we have left.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Lines in /etc/group longer than 64 characters breaks the current
implementation of group parser. This is caused by bufio.Scanner
buffer limit.
Fix by re-using the fix for a similar problem in golang os/user,
namely https://go-review.googlesource.com/c/go/+/283601.
Add some tests.
Co-authored-by: Andrey Bokhanko <andreybokhanko@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Same as in other places (other parsers here, as well as golang os/user
parser and glibc parser all tolerate extra space at BOL and EOL).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Don't ignore close(2) return code, rather bail if there is any
unexpected failures. By checking the close return code we make sure we
don't introduce the same bug (closing an already closed fd) I've fixed
in the previous patch.
As a side note, we are not handling in this patch when close(2) returns
EINTR and the go runtime, since go 1.14, sends SIGURG to preempt
goroutines. This should not happen here though, as nsenter is guaranteed
to be executed before the go runtime starts.
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
This was closed in the child[1], before calling clone_parent (so runc
INIT will have this fd closed too), there is no point closing it again.
This was not causing issues because we ignore the return code of
close(2) and no one was opening a new fd between both calls to close.
However, with the new patches that I'm working on (PR #2576), this
problem is no longer inocuos: we do open a new fd in that PR, sometimes
that fd is allocated between the two close(2) calls and, as the lowest
fd is allocated to the new fd, sometimes the second close ends up
incorrectly closing this new fd.
Before it was not a problem in practice, but it was incorrect
nevertheless.
This seems to be long standing bug, present since at least 2018
(a54316bae), when SYNC_GRANDCHILD was introduced.
[1]: https://github.com/opencontainers/runc/blob/5547b5774f71f75a088e7432fa961778750a0fbd/libcontainer/nsenter/nsexec.c#L888
Co-authored-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
When running a script from an azure file share interrupted syscall
occurs quite frequently, to remedy this add retries around execve
syscall, when EINTR is returned.
Signed-off-by: Maksim An <maksiman@microsoft.com>
The test is failing like this:
not ok 70 runc run --no-pivot must not expose bare /proc
# (in test file tests/integration/no_pivot.bats, line 20)
# `[[ "$output" == *"mount: permission denied"* ]]' failed
# runc spec (status=0):
#
# runc run --no-pivot test_no_pivot (status=1):
# unshare: write error: Operation not permitted
Apparently, a recent kernel commit db2e718a47984b9d prevents
root from doing unshare -r unless it has CAP_SETFPCAP.
Add the capability for this specific test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These are not used anywhere outside of the package
(I have also checked the only external user of the package
(github.com/google/cadvisor).
No changes other than changing the case. The following
identifiers are now private:
* IntelRdtTasks
* NewLastCmdError
* NewStats
Brought to you by gorename.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... the stack, so every caller will automatically benefit from it.
The only change that it causes is the user in
libcontainer/process_linux.go will get a better error message.
[v2: typo fix]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For errors that only have a string and an underlying error, using
fmt.Errorf with %w to wrap an error is sufficient.
In this particular case, the code is simplified, and now we have
unwrappable errors as a bonus (same could be achieved by adding
(*LastCmdError).Unwrap() method, but that's adding more code).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Initially, this was copied over from libcontainer/cgroups, where it made
sense as for cgroup v1 we have multiple controllers and mount points.
Here, we only have a single mount, so there's no need for the whole
type.
Replace all that with a simple error (which is currently internal since
the only user is our own test case).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case getIntelRdtData() returns an error, d is set to nil.
In case the error returned is of NotFoundError type (which happens
if resctlr mount is not found in /proc/self/mountinfo), the function
proceeds to call d.join(), resulting in a nil deref and a panic.
In practice, this never happens in runc because of the checks in
intelrdt() function in libcontainer/configs/validate, which raises
an error in case any of the parameters are set in config but
the IntelRTD itself is not available (that includes checking
that the mount point is there).
Nevertheless, the code is wrong, and can result in nil dereference
if some external users uses Apply on a system without resctrl mount.
Fix this by removing the exclusion.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In cases we have something like
if y != "" {
x = y
}
where both x and y are strings, and x was not set before,
it makes no sense to have a condition, as such code is
equivalent to mere
x = y
Simplify such cases by removing "if".
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This removes libcontainer's own error wrapping system, consisting of a
few types and functions, aimed at typization, wrapping and unwrapping
of errors, as well as saving error stack traces.
Since Go 1.13 now provides its own error wrapping mechanism and a few
related functions, it makes sense to switch to it.
While doing that, improve some error messages so that they start
with "error", "unable to", or "can't".
A few things that are worth mentioning:
1. We lose stack traces (which were never shown anyway).
2. Users of libcontainer that relied on particular errors (like
ContainerNotExists) need to switch to using errors.Is with
the new errors defined in error.go.
3. encoding/json is unable to unmarshal the built-in error type,
so we have to introduce initError and wrap the errors into it
(basically passing the error as a string). This is the same
as it was before, just a tad simpler (actually the initError
is a type that got removed in commit afa844311; also suddenly
ierr variable name makes sense now).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
Instead of using errors.Wrap, use fmt.Errorf with %w for error wrapping.
Also, use errors.Is instead of direct error comparison.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use fmt.Errorf with %w instead.
Convert the users to the new wrapping.
This fixes an errorlint warning.
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>
Errors from os.Open, os.Symlink etc do not need to be wrapped, as they
are already wrapped into os.PathError.
Error from unix are bare errnos and need to be wrapped. Same
os.PathError is a good candidate.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Errors returned by unix are bare. In some cases it's impossible to find
out what went wrong because there's is not enough context.
Add a mountError type (mostly copy-pasted from github.com/moby/sys/mount),
and mount/unmount helpers. Use these where appropriate, and convert error
checks to use errors.Is.
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>
Errors from strconv.Atoi are already descriptive enough, and contain the
value being converted, so our error messages do not need to contain it.
While at it, use %w to wrap errors.
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>
An errror from ioutil.WriteFile already contains file name, so there is
no need to duplicate that information.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin (3):
libct/cg/fs/blkio: do not set weight == 0
libct/cg/fs2: set per-device io weight if available
tests/int/cgroups: add test for bfq per-device weight
LGTMs: AkihiroSuda mrunalp cyphar
Closes#3022
Current runc man pages are ugly (no proper man page formatting)
and very short (mostly just a copy-paste from the "runc <command>
--help" output. They are also somewhat obsoleted as not all CLI updates
were propagated to man/*.
This commits makes the first step to solving this.
In short:
- added some more information about some options;
- lots of formatting fixes;
- use references to other man pages and web pages;
- fix SYNOPSYS (formatting, mostly);
- removed the repeated description of <container_id> from every page;
- added SEE ALSO;
- something else I forgot.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Now runc puts dump/restore logs in c.root defaultly, which will be deleted
when container exits. So if checkpinting/restoring failed, we can not get
these logs and analyze why.
This patch lets criu use its default if --work-path is not set:
- Use WorkDirectory found in criu's configfile.
- Use ImageDirectory.
Signed-off-by: Liu Hua <weldonliu@tencent.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>
Commit 52390d6804 made this parameters obsoleted, but they are
still shown in e.g. runc update --help output.
Hide them (and maybe in 5 years we can remove them).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was removed from runc exec by commit f61c6e413f about 5 years ago,
so it's time to remove it entirely.
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>
Sebastiaan van Stijn (2):
libcontainer: relax validation for absolute paths
configs/validator: move cgroup validation to the list of checks
LGTMs: kolyshkin cyphar
Closes#3004
Commits 1f1e91b1a0 and 2192670a24
added validation for mountpoints to be an absolute path, to match the OCI
specs.
Unfortunately, the old behavior (accepting the path to be a relative path)
has been around for a long time, and although "not according to the spec",
various higher level runtimes rely on this behavior.
While higher level runtime have been updated to address this requirement,
there will be a transition period before all runtimes are updated to carry
these fixes.
This patch relaxes the validation, to generate a WARNING instead of failing,
allowing runtimes to update (but allowing them to update runc to the current
version, which includes security fixes).
We can remove this exception in a future patch release.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
Restoring an SELinux enabled container with Podman will result in
a container with the exactly same SELinux process labels as during
checkpointing. CRIU takes care of all the process labels.
Restoring multiple copies of a checkpointed container will result in all
containers having the same SELinux process labels, which might be
undesired.
When looking at Pods all container in a Pod share the process label
of the infrastructure container. To restore a container into and
existing Pod it is necessary to tell CRIU to restore the container
with the infrastructure container process label.
CRIU supports setting different process labels using --lsm-profile for a
long time and this just passes the process label information from runc
to CRIU.
Unfortunately CRIU has a bug as no one was using the --lsm-profile
option so this changes requires the upcoming CRIU version 3.16.
Signed-off-by: Adrian Reber <areber@redhat.com>
... and remove the one from tests/integration.
The idea is similar to the one for the test case being removed -- try
updating device rules many times to make sure we are not leaking eBPF
programs after every update/Set(). This is better though as we can
really change the device rules every time (which "runc update" can't)
and check that the rule is applied.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These were deprecated and moved; the stubs were included in the
last two (rc94, rc95) releases, so external consumers would have
the chance to update their code.
Removing this so that this doesn't get into v1.0.0 GA
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These were deprecated and moved; the stubs were included in the
last two (rc94, rc95) releases, so external consumers would have
the chance to update their code.
Removing this so that this doesn't get into v1.0.0 GA
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
The runc update CLI is not able to modify devices, so let's set SkipDevices
(so that a cgroup controller won't try to update devices cgroup).
This helps use cases when some other device management (NVIDIA GPUs)
applies its configuration on top of what runc does.
Make sure we do not save SkipDevices into state.json.
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>
Explain where the "/dev/tty: no such device or address" error is coming
from, and provide ways to solve the issue.
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>
This prevents having to maintain GoDoc for the stub implementations,
and makes sure that the "stub" implementations have the same signature
as the "non-stub" versions.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
The "runc run [device cgroup allow rm block device]" test calls lsblk
three times to get device name, minor and major number. This creates a
potential problem when the devices are changed between the calls.
Simplify the code by using bash read together with IFS (as there's no
way to have lsblk output MAJOR:MINOR pair without a semicolon).
Note that head -n 1 is not needed as read already reads a single line.
[v2: don't use PATH as CentOS7's lsblk does not support it.]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently not everyone compiles runc via the provided Makefile. For
example, one can just run "go build", in which case Version variable
is left empty, which leads to:
$ ./runc -v
runc version spec: 1.0.2-dev
go: go1.16.3
Surely, the main problem here is runc was built in a wrong way, but the
second problem is such output is very confusing -- it may seem that we
have runc 1.0.2.
To solve, make sure to _always_ add version (even if empty), and set the
default to "unknown".
NOTE this does not change anything in case runc is compiled via the
Makefile.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It seems that golangci-lint didn't warn us about new lint errors that
were added after we enabled it, so just run the full thing and give us
all the errors on every PR run -- as long as we keep master lint-clean
it doesn't matter whether we set this or not.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.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>
This is to ensure that we aren't leaking eBPF programs after "runc
update". Unfortunately we cannot directly test the behaviour of cgroup
program updates in an integration test because "runc update" doesn't
support that behaviour at the moment.
So instead we rely on the fact that each "runc update" implicitly
triggers the devices rules to be updated. Without the previous patches
applied, this new test will fail with errors (on cgroupv2 systems).
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>
When running inside a Docker container, systemd is not available. The
new TestFdLeaksSystemd forgot to include the relevant t.Skip section.
Fixes: a7feb42395 ("libct/int: add TestFdLeaksSystemd")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Use "git describe --dirty --long" instead of "git rev-parse". As a
result, the commit ID will contain the closest tag, the number of commits
since the tag, and the (abbreviated) git commit sha (see example below).
NOTE that this tag is still unique and can be used instead of bare sha
for all git commands.
Example output of "runc -v | grep commit".
Before:
commit: 4d87573871
After:
commit: v1.0.0-rc95-9-g6f55d074
This means that
- the closest tag is v1.0.0-rc95
- there were 9 commits after the tag
- the abbreviated sha is 6f55d074
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.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>
1. These tests can't be run in parallel since they do check
a global variable (mbaScEnabled).
2. findIntelRdtMountpointDir() relies on mbaScEnabled to be initially
set to the default value (false) and this the test fails if run
more than once:
> go test -count 2
> ...
> intelrdt_test.go:243: expected mbaScEnabled=false, got true
> --- FAIL: TestFindIntelRdtMountpointDir/Valid_mountinfo_with_MBA_Software_Controller_disabled (0.00s)
Fixes: 2c70d2384
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
runc resolves symlink before doing bind mount. So
we should save original path while formatting CriuReq for
checkpoint.
Signed-off-by: Liu Hua <weldonliu@tencent.com>
2021-04-25 09:50:00 +08:00
669 changed files with 32370 additions and 34618 deletions
||(echo -e "git status:\n $$(git status -- go.mod go.sum vendor/)\nerror: vendor/, go.mod and/or go.sum not up to date. Run \"make vendor\" to update";exit 1)\
&&echo"all vendor files are up to date."
cross:runcimage
$(CONTAINER_ENGINE) run $(CONTAINER_ENGINE_RUN_FLAGS)\
"set -x; cd /dev/shm; mkdir test-dir; touch test-file; chmod 777 test-file; stat /dev/shm/test-dir-foo && ls -l /dev/shm && echo \"Note the agent added a suffix for the directory name and chmod fails\" "
t.Fatalf("Failed to write %d on attempt %d: %+v",limit,i,err)
}
}
}
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.