lifubang (2):
libct: don't send config to nsexec when joining an existing timens
test: exec into a container with private time ns
LGTMs: cyphar lifubang
The previous logic caused runc to hang if CloseExecFrom returned an
error, as the defer waiting on logsDone never finished as the parent
process was never started (and it controls the closing of logsDone via
it's logsPipe).
This moves the defer to after we have started the parent, with means all
the logic related to managing the logsPipe should also be running.
Signed-off-by: Evan Phoenix <evan@phx.io>
(cherry picked from commit 7b26da9ee3)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Prior to kernel Linux 5.5, F_SEAL_FUTURE_WRITE has a bug which maps
memory as shared between processes even if it is set as private. See
kernel commit 05d351102dbe ("mm, memfd: fix COW issue on MAP_PRIVATE and
F_SEAL_FUTURE_WRITE mappings") for more details.
According to the fcntl(2) man pages, F_SEAL_WRITE is enough:
> Furthermore, trying to create new shared, writable memory-mappings via
> mmap(2) will also fail with EPERM.
>
> Using the F_ADD_SEALS operation to set the F_SEAL_WRITE seal fails
> with EBUSY if any writable, shared mapping exists. Such mappings must
> be unmapped before you can add this seal.
F_SEAL_FUTURE_WRITE only makes sense if a read-write shared mapping in
one process should be read-only in another process. This is not case for
runc, especially not for the /proc/self/exe we are protecting.
Signed-off-by: Tomasz Duda <tomaszduda23@gmail.com>
(cyphar: improve the comment regarding F_SEAL_FUTURE_WRITE)
(cyphar: improve commit message)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit c43ea7d629)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Retry Recvfrom, Sendmsg, Readmsg, and Read as they can return EINTR.
Signed-off-by: Evan Phoenix <evan@phx.io>
(cherry picked from commit 28475f12e3)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We should configure the process's timens offset only when we need to
create new time namespace, we shouldn't do it if we are joining an
existing time namespace. (#4635)
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit ad09197e41)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This helper was added for runc-dmz in commit dac417174, but runc-dmz was
later removed in commit 871057d, which forgot to remove the helper.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 83350c24a9)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes k3s cross-compilation on Windows, broken by commit
1912d5988b ("*: actually support joining a userns with a new
container").
[@kolyshkin: commit message]
Fixes: 1912d5988b
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit ccb589bd7d)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This release includes a minor breaking API change that requires us to
rework the types of our wrappers, but there is no practical behaviour
change.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 70e500e7d1)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since v1.2.0 was released, a number of users complained that the removal
of tun/tap device access from the default device ruleset is causing a
regression in their workloads.
Additionally, it seems that some upper-level orchestration tools
(Docker Swarm, Kubernetes) makes it either impossible or cumbersome
to supply additional device rules.
While it's probably not quite right to have /dev/net/tun in a default
device list, it was there from the very beginning, and users rely on it.
Let's keep it there for the sake of backward compatibility.
This reverts commit 2ce40b6ad7.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(Cherry-pick of commit 394f4c3b7012674ebe0232c560713e57cbd653e6.)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(This is a cherry-pick of c0044c7aa403ecf2d9172bd9386d05433b011076.)
If we get an unexpected error here, it is probably because of a library
or kernel change that could cause our detection logic to be invalid. As
a result, these warnings should be louder so users have a chance to tell
us about them sooner (or so we might notice them before doing a release,
as happened with the 1.2.0 regression).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(This is a cherry-pick of dea0e04dd93d3922083e68667d20aac532d31129.)
It is possible for LinkAttachProgram to return ErrNotSupported if
program attachment is not supported at all (which doesn't matter in this
case), but it seems possible that upstream will start returning
ErrNotSupported for BPF_F_REPLACE at some point so it's best to make
sure we don't cause additional regressions here.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(This is a cherry-pick of dea0e04dd93d3922083e68667d20aac532d31129.)
In v0.13.0, cilium/ebpf stopped supporting setting BPF_F_REPLACE as an
explicit flag and instead requires us to use link.Anchor to specify
where the program should be attached.
Commit 216175a9ca ("Upgrade Cilium's eBPF library version to 0.16")
did update this correctly for the actual attaching logic, but when
checking for kernel support we still passed BPF_F_REPLACE. This would
result in a generic error being returned, which our feature-support
checking logic would treat as being an error the indicates that
BPF_F_REPLACE *is* supported, resulting in a regression on pre-5.6
kernels.
It turns out that our debug logging saying that this unexpected error
was happening was being output as a result of this change, but nobody
noticed...
Fixes: 216175a9ca ("Upgrade Cilium's eBPF library version to 0.16")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
If /run/runc and /usr/bin are on different filesystems, overlayfs may
enable the xino feature which results in the following log message:
kernel: overlayfs: "xino" feature enabled using 3 upper inode bits.
Each time we have to protect /proc/self/exe. So disable xino to remove
the log message (we don't care about the inode numbers of the files
anyway).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 9bc42d61bb)
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
If the sub-cgroup RemovePath has failed for any reason, return the
error right away. This way, we don't have to check for err != nil
before retrying rmdir.
This is a cosmetic change and should not change any functionality.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 12e06a7c4f)
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
An issue with runc 1.2.0 was reported to buildkit, in which
runc delete returns with an error, with the log saying:
> unable to destroy container: unable to remove container's cgroup: open /sys/fs/cgroup/snschvixiy3s74w74fjantrdg: no such file or directory
Apparently, what happens is runc is running with no cgroup access
(because /sys/fs/cgroup is mounted read-only). In this case error to
create a cgroup path (in runc create/run) is ignored, but cgroup removal
(in runc delete) is not.
This is caused by commit d3d7f7d, which changes the cgroup removal
logic in RemovePath. In the current code, if the initial rmdir has
failed (in this case with EROFS), but the subsequent os.ReadDir returns
ENOENT, it is returned (instead of being ignored -- as the path does not
exist and so there is nothing to remove).
Here is the minimal fix for the issue.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit db59489b68)
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
There is a typo in the comment (ClonedBinary should be CloneBinary), and
the code has changed a bit since then, and it makes more sense to refer
to CloneSelfExe now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 8cc7375447)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This allows to do
runc update $ID --memory=-1 --memory-swap=$VAL
for cgroup v2, i.e. set memory to unlimited and swap to a specific
value.
This was not possible because ConvertMemorySwapToCgroupV2Value rejected
memory=-1 ("unlimited"). In a hindsight, it was a mistake, because if
memory limit is unlimited, we should treat memory+swap limit as just swap
limit.
Revise the unit test; add description to each case.
Fixes: c86be8a2 ("cgroupv2: fix setting MemorySwap")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 732806e24c)
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Improve readability of ConvertMemorySwapToCgroupV2Value by switching
from a bunch of if statements to a switch, and adding a comment
describing each case.
No functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit cb9f3d6d14)
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Because we have the overlay solution, we can drop runc-dmz binary
solution since it has too many limitations.
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit 871057d863)
Signed-off-by: lifubang <lifubang@acmcoder.com>
Since Go 1.19, the same functionality is there in os/exec package.
As we require go 1.22 now, there's no need to have this.
This basically reverts commit 9258eac0 ("libct/start: use execabs for
newuidmap lookup").
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit eb2ff52ace)
Signed-off-by: Austin Vazquez <macedonv@amazon.com>
Containerd pre-creates userns and netns before calling runc, which
results in the current code not working when SELinux is enabled,
resulting in the following error:
> runc create failed: unable to start container process: error during
container init: error mounting "mqueue" to rootfs at "/dev/mqueue":
setxattr /path/to/rootfs/dev/mqueue: operation not permitted
The solution is to become root in the user namespace right after
we join it.
Fixes#4466.
Co-authored-by: Wei Fu <fuweid89@gmail.com>
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-authored-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit c78f3f2ea0)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is a race situation when we are opening a file, if there is a
small fd was closed at that time, maybe it will be reused by safeExe.
Because of Go stdlib fds shuffling bug, if the fd of safeExe is too
small, go stdlib will dup3 it to another fd, or dup3 a other fd to this
fd, then it will cause the fd type cmd.Path refers to a random path,
and it can lead to an error "permission denied" when starting the process.
Please see #4294 and <https://github.com/golang/go/issues/61751>.
So we should not use the original fd of safeExe, but use the fd after
shuffled by Go stdlib. Because Go stdlib will guarantee this fd refers to
the correct file.
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
Commit b999376fb2 ("nsenter: cloned_binary: remove bindfd logic
entirely") removed the read-only bind-mount logic from our cloned binary
code because it wasn't really safe because a container with
CAP_SYS_ADMIN could remove the MS_RDONLY bit and get write access to
/proc/self/exe (even with user namespaces this could've been an issue
because it's not clear if the flags are locked).
However, copying a binary does seem to have a minor performance impact.
The only way to have no performance impact would be for the kernel to
block these write attempts, but barring that we could try to reduce the
overhead by coming up with a mount that cannot have it's read-only bits
cleared.
The "simplest" solution is to create a temporary overlayfs using
fsopen(2) which uses the directory where runc exists as a lowerdir,
ensuring that the container cannot access the underlying file -- and we
don't have to do any copies.
While fsopen(2) is not free because mount namespace cloning is usually
expensive (and so it seems like the difference would be marginal), some
basic performance testing seems to indicate there is a ~60% improvement
doing it this way and that it has effectively no overhead even when
compared to just using /proc/self/exe directly:
% hyperfine --warmup 50 \
> "./runc-noclone run -b bundle ctr" \
> "./runc-overlayfs run -b bundle ctr" \
> "./runc-memfd run -b bundle ctr"
Benchmark 1: ./runc-noclone run -b bundle ctr
Time (mean ± σ): 13.7 ms ± 0.9 ms [User: 6.0 ms, System: 10.9 ms]
Range (min … max): 11.3 ms … 16.1 ms 184 runs
Benchmark 2: ./runc-overlayfs run -b bundle ctr
Time (mean ± σ): 13.9 ms ± 0.9 ms [User: 6.2 ms, System: 10.8 ms]
Range (min … max): 11.8 ms … 16.0 ms 180 runs
Benchmark 3: ./runc-memfd run -b bundle ctr
Time (mean ± σ): 22.6 ms ± 1.3 ms [User: 5.7 ms, System: 20.7 ms]
Range (min … max): 19.9 ms … 26.5 ms 114 runs
Summary
./runc-noclone run -b bundle ctr ran
1.01 ± 0.09 times faster than ./runc-overlayfs run -b bundle ctr
1.65 ± 0.15 times faster than ./runc-memfd run -b bundle ctr
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This initWaiter logic was introduced by commit 4ecff8d9, but since the logic of
/proc/self/exe was moved out of runc init in commit 0e9a335, this
seems unnecessary to have initWaiter.
Remove it.
This essentially reverts commit 4ecff8d9.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
While we did set +x when "sealing" regular temporary files, the "is
executable" checks were done before then and would thus fail, causing
the fallback to not work properly.
So just set +x after we create the file. We already have a O_RDWR handle
open when we do the chmod so we won't get permission issues when writing
to the file.
Fixes: e089db3b4a ("dmz: add fallbacks to handle noexec for O_TMPFILE and mktemp()")
Signed-off-by: lifubang <lifubang@acmcoder.com>
We are not really interested in the capabilities of the current process,
so there is no need to load those.
This results in some performance improvement since now the capability
package don't have to parse /proc/self/status.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The userns package was moved to the moby/sys/userns module
at commit 3778ae603c.
This patch deprecates the old location, and adds it as an alias
for the moby/sys/userns package.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
When preparing to mount container root, we need to make its parent mount
private (i.e. disable propagation), otherwise the new in-container
mounts are leaked to the host.
To find a parent mount, we use to read mountinfo and find the longest
entry which can be a parent of the container root directory.
Unfortunately, due to kernel bug in all Linux kernels older than v5.8
(see [1], [2]), sometimes mountinfo can't be read in its entirety. In
this case, getParentMount may occasionally return a wrong parent mount.
As a result, we do not change the mount propagation to private, and
container mounts are leaked.
Alas, we can not fix the kernel, and reading mountinfo a few times to
ensure its consistency (like it's done in, say, Kubernetes) does not
look like a good solution for performance reasons.
Fortunately, we don't need mountinfo. Let's just traverse the directory
tree, trying to remount it private until we find a mount point (any
error other than EINVAL means we just found it).
Fixes issue 2404.
[1]: https://github.com/kolyshkin/procfs-test
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f6c61f96f2d97cbb5f
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Initially, this was a commit to switch from strings.Fields to
strings.SplitN in getCpuUsageBreakdown, since strings.Fields
was probably slower than strings.SplitN in some old Go versions.
Afterwards, strings.Cut was also considered for potential
speed improvements.
After writing a benchmark test, we learned that:
- strings.Fields performance is now adequate;
- strings.SplitN is slower than strings.Fields;
- strings.Cut had <5% performance gain from strings.Fields;
So, remove the TODO and keep the benchmark test.
Signed-off-by: Stavros Panakakis <stavrospanakakis@gmail.com>
The example is too long since it lists too many capabilities.
Simplify it, leaving only two capabilities.
Also, remove ambient capabilities from the set. Inheritable capabilities
were removed earlier by commit 98fe566c, but ambient capabilities can't
be raised without inheritable ones.
Fixes: 98fe566c
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 98fe566c removed inheritable capabilities from the example spec
(used by runc spec) and from the libcontainer/integration test config,
but neglected to also remove ambient capabilities.
An ambient capability could only be set if the same inheritable
capability is set, so as a result of the above change ambient
capabilities were not set (but due to a bug in gocapability package,
those errors are never reported).
Once we start using a library with the fix [1], that bug will become
apparent (both bats-based and libct/int tests will fail).
[1]: https://github.com/kolyshkin/capability/pull/3
Fixes: 98fe566c ("runc: do not set inheritable capabilities")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For some rootless container, runc has no access to cgroup,
But the container is still running. So we should return the
`ErrNotRunning` and `ErrCgroupNotExist` error seperatlly.
Signed-off-by: lifubang <lifubang@acmcoder.com>
Shared pid namespace means `runc kill` (or `runc delete -f`) have to
kill all container processes, not just init. To do so, it needs a cgroup
to read the PIDs from.
If there is no cgroup, processes will be leaked, and so such
configuration is bad and should not be allowed. To keep backward
compatibility, though, let's merely warn about this for now.
Alas, the only way to know if cgroup access is available is by returning
an error from Manager.Apply. Amend fs cgroup managers to do so (systemd
doesn't need it, since v1 can't work with rootless, and cgroup v2 does
not have a special rootless case).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In these cases, this is exactly what we want to find out.
Slightly improves performance and readability.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It turns out that the suid and sgid mode bits are silently ignored by
Linux (though the sticky bit is honoured), and some users are requesting
mode bits that are ignored. While returning an error (as securejoin
does) makes some sense, this is a regression.
Ref: https://github.com/cyphar/filepath-securejoin/issues/23
Fixes: dd827f7b71 ("utils: switch to securejoin.MkdirAllHandle")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Aleksa Sarai