The correct way to do that conversion according to
https://pkg.go.dev/syscall#Errno is:
```
err = nil
if errno != 0 {
err = errno
}
```
In this case the error check will always report a false positive in
unix.RawSyscall(unix.SYS_SECCOMP, ...), probably nobody has faced this
problem because the code takes the other path in most of the cases.
Fixes: 7a8d7162f9 ("seccomp: prepend -ENOSYS stub to all filters")
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Before this commit, Set() used GetState() to check the freezer state
and retry the operation if the actual state still differs from requested.
This should help with the situation when a new process (such as one
added by runc exec) is added to the container's cgroup while it's being
freezed by the kernel, but it's not working as it should.
The problem is, GetState() never returns FREEZING state, looping until
the state is either FROZEN or THAWED, so Set() does not have a chance
to repeate the freeze attempt.
As a result, the container might end up stuck in a FREEZING state,
with GetState() never returning (which in turn blocks some other
operations).
One way to fix this would be to have GetState returning FREEZING state
instead of retrying ad infinitum. It would result in changing the public
API, and no callers of GetState expects it to return this.
To fix, let's not use GetState() from Set(). Instead, read the
freezer.state file directly and act accordingly -- return success
on FROZEN, retry on FREEZING, and error out on any other (unexpected)
value.
While at it, further improve the code:
- limit the number of retries;
- if retries are exceeded, thaw and return an error;
- don't retry (or read the state back) on THAW.
I played a lot with various reproducers for this bug, including
- parallel runc execs and runc pause/resumes
- parallel runc execs and runc --systemd-cgroup update
(the latter performs freeze/unfreeze);
- continuously running /bin/printf inside container
in parallel with runc pause/resume;
- running pthread bomb (from criu test suite) in parallel
with runc pause/resume;
and I was not able to make freeze work 100%, meaning sometimes
runc pause fails, or runc --systemd-cgroup update produces a warning.
With that said, it's still a big improvement over the previous
state of affairs where container is stuck in FREEZING state,
and GetState() (and all its users) are also stuck.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This function is called by `InitSeccomp`, but only when compiled
with seccomp (and cgo) enabled, so should not be needed for other
situations.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The merge 6eed6e5795 broke the build because ab27e12ceb ("Implement
GetStat for cpuset cgroup.") dropped the errors import which was used by
c85cd2b325 ("libct/cg/fs/cpuset: don't parse mountinfo") and the CI
wasn't retriggered.
Fix this by just importing "github.com/pkg/errors" again.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Having -EPERM is the default was a fairly significant mistake from a
future-proofing standpoint in that it makes any new syscall return a
non-ignorable error (from glibc's point of view). We need to correct
this now because faccessat2(2) is something glibc critically needs to
have support for, but they're blocked on container runtimes because we
return -EPERM unconditionally (leading to confusion in glibc). This is
also a problem we're probably going to keep running into in the future.
Unfortunately there are several issues which stop us from having a clean
solution to this problem:
1. libseccomp has several limitations which require us to emulate
behaviour we want:
a. We cannot do logic based on syscall number, meaning we cannot
specify a "largest known syscall number";
b. libseccomp doesn't know in which kernel version a syscall was
added, and has no API for "minimum kernel version" so we cannot
simply ask libseccomp to generate sane -ENOSYS rules for us.
c. Additional seccomp rules for the same syscall are not treated as
distinct rules -- if rules overlap, seccomp will merge them. This
means we cannot add per-syscall -EPERM fallbacks;
d. There is no inverse operation for SCMP_CMP_MASKED_EQ;
e. libseccomp does not allow you to specify multiple rules for a
single argument, making it impossible to invert OR rules for
arguments.
2. The runtime-spec does not have any way of specifying:
a. The errno for the default action;
b. The minimum kernel version or "newest syscall at time of profile
creation"; nor
c. Which syscalls were intentionally excluded from the allow list
(weird syscalls that are no longer used were excluded entirely,
but Docker et al expect those syscalls to get EPERM not ENOSYS).
3. Certain syscalls should not return -ENOSYS (especially only for
certain argument combinations) because this could also trigger glibc
confusion. This means we have to return -EPERM for certain syscalls
but not as a global default.
4. There is not an obvious (and reasonable) upper limit to syscall
numbers, so we cannot create a set of rules for each syscall above
the largest syscall number in libseccomp. This means we must handle
inverse rules as described below.
5. Any syscall can be specified multiple times, which can make
generation of hotfix rules much harder.
As a result, we have to work around all of these things by coming up
with a heuristic to stop the bleeding. In the future we could hopefully
improve the situation in the runtime-spec and libseccomp.
The solution applied here is to prepend a "stub" filter which returns
-ENOSYS if the requested syscall has a larger syscall number than any
syscall mentioned in the filter. The reason for this specific rule is
that syscall numbers are (roughly) allocated sequentially and thus newer
syscalls will (usually) have a larger syscall number -- thus causing our
filters to produce -ENOSYS if the filter was written before the syscall
existed.
Sadly this is not a perfect solution because syscalls can be added
out-of-order and the syscall table can contain holes for several
releases. Unfortuntely we do not have a nicer solution at the moment
because there is no library which provides information about which Linux
version a syscall was introduced in. Until that exists, this workaround
will have to be good enough.
The above behaviour only happens if the default action is a blocking
action (in other words it is not SCMP_ACT_LOG or SCMP_ACT_ALLOW). If the
default action is permissive then we don't do any patching.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Kir Kolyshkin (3):
tests/int/mounts.bats: cleanup
tests/int/mount.bats: reformat
runc run: resolve tmpfs mount dest in container scope
LGTMs: @AkihiroSuda @cyphar
Closes#2715
Test that CommandHook actually executes a new process with the given env
variables, parameters and json state.
This commit also solves an issue with the previous approach that was calling
'os.Exit(0)' failing to signal test failures.
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
A recent commit a35cad3b22 added warnings about systemd being too
old. While those warnings are valid, they break some existing tests,
and also don't add much value to a user (IOW no one is going to upgrade
systemd because runc says it's old).
Demote those to warnings.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It was already explained why we ignore the error, so let's ignore this
deliberately.
This fixes
> name.go:22:7: Error return value of `join` is not checked (errcheck)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Rewrite getPageUsageByNUMA
1. Be less strict to unknown contents, i.e. skip it. This makes the
function more future-proof. Before this commit, if a line like
"a=b" is encountered, the function returns an error, which is
propagated all the way up to and returned by (CgroupManager).GetStats.
2. Be more strict to contents it recognizes, i.e. return an error.
In case the first field in the line is recognized (e.g. "total=123",
the rest of the line should be in format "N<id>=<value> ...".
3. Optimize. Before this commit, addNUMAStatsByType was called for every
item in the line, which is excessive and might even be slow in case
there are many NUMA nodes. It is enough to look up the field once.
4. Remove a bunch of global numaNode* and numaStat* constants. Those
were used by only one function, and it does not make sense to have
them defined globally. Some were moved to the function, some were
eliminated entirely.
5. Improve readability and added code comments.
Finally, add some test cases for good and bad contents.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
strings.SplitN not always return N fields if not staify, sometimes
cgroup interface add some custom fields make parse memory.numa_stat
fails, it will case panic
Signed-off-by: acetang <aceapril@126.com>
As buf is instantiated outside the loop, it is appended to,
so if/once an error happens, it contains the output of all previous
iterations. Not a big problem but looks a bit untidy.
Move the declaration to inside the loop.
Fixes: 06a684d6a7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This reverts most of commit 24c05b7, as otherwise it causes
a few regressions (docker cli, TestDockerSwarmSuite/TestServiceLogsTTY).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This:
> === RUN TestGetHugePageSizeImpl
> utils_test.go:504: (input [hugepages-akB], error strconv.Atoi: parsing "a": invalid syntax)
feels like an error but it's not.
Only log errors.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Add a check to unifiedResToSystemdProps that systemd is recent enough
to support AllowedCPUs/AllowedMemoryNodes unit properties, and skip
setting the property if it is not supported.
Note that this is not an error as the setting is still applied to
the underlying cgroupfs -- it's just systemd unit property that is
being skipped.
2. In all the places we skip an unsupported property, warn about it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Support for systemd properties AllowedCPUs and AllowedMemoryNodes
was added by commit 13afa58d0e, but only for unified resources
of systemd v2 driver.
This adds support for Cpu.Cpus and Cpu.Mems resources to
both systemd v1 and v2 cgroup drivers.
An integration test is added to check that the settings work.
[v2: check for systemd version]
[v3: same in the test]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As the caller of this function just logs the error, it does not make
sense to pass it. Instead, log it (once) and return -1.
This is a preparation for the second user.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit adjusts the file mode to use the latest golang style
and also changes the file mode value in accordance with default.
Signed-off-by: Kenta Tada <Kenta.Tada@sony.com>
Simplify the tty code by using 1 goroutine instead of 2.
Improve error reporting by wrapping the errors.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The TestExecInTTY test case is sometimes failing like this:
> execin_test.go:332: unexpected carriage-return in output "PID USER TIME COMMAND\r\n 1 root 0:00 cat\r\n 7 root 0:00 ps\r\n"
or this:
> execin_test.go:332: unexpected carriage-return in output "PID USER TIME COMMAND\r\n 1 root 0:00 cat\n 7 root 0:00 ps\n"
(this is easy to repro with `go test -run TestExecInTTY -count 1000`).
This is caused by a race between
- an Init() (in this case it is is (*linuxSetnsInit.Init(), but
(*linuxStandardInit).Init() is no different in this regard),
which creates a pty pair, sends pty master to runc, and execs
the container process,
and
- a parent runc process, which receives the pty master fd and calls
ClearONLCR() on it.
One way of fixing it would be to add a synchronization mechanism
between these two, so Init() won't exec the process until the parent
sets the flag. This seems excessive, though, as we can just move
the ClearONLCR() call to Init(), putting it right after console.NewPty().
Note that bug only happens in the TestExecInTTY test case, but
from looking at the code it seems like it can happen in runc run
or runc exec, too.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I noticed this was the only place in this function where we didn't
handle errors on freezing/thawing. Logging as a warning, consistent
with the other cases.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 9c1242ecb ("Add white list for bind mount chec", Jan 6 2016)
added a set of entries under /proc which we allow to be mounted to,
for the benefit of lxcfs-like fuse-backed hack to have container's
own version of /proc/meminfo etc.
For some reason, the allow list check is performed at the very
beginning of the function, which is not optimal.
Move the check to the end -- at this point in the code we already
know we're under /proc, so it make sense to consult the allow list.
This makes the code slightly more logical and hopefully slightly faster.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a new variable added by go 1.16 so we'll have to wait
until 1.16 is minimally supported version, thus TODO for now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
While working on a test case for [1], I got the following warning:
> level=warning msg="unable to terminate initProcess" error="exit status 1"
Obviously, the warning is bogus since the initProcess is terminated.
This is happening because terminate() can return errors from either
Kill() or Wait(), and the latter returns an error if the process has
not finished successfully (i.e. exit status is not 0 or it was killed).
Check for a particular error type and filter out those errors.
[1] https://github.com/opencontainers/runc/issues/2683
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case a tmpfs mount path contains absolute symlinks, runc errors out
because those symlinks are resolved in the host (rather than container)
filesystem scope.
The fix is similar to that for bind mounts -- resolve the destination
in container rootfs scope using securejoin, and use the resolved path.
A simple integration test case is added to prevent future regressions.
Fixes https://github.com/opencontainers/runc/issues/2683.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Mauricio Vásquez