While testing this with old kernel versions and kernels that don't
support idmap mounts for some of the filesystems used by a container, I
realized we can throw a more clear errors.
Let's make it clear which syscall we are using, when it is not supported
and when if the fs doesn't support idmap mounts, which path it is.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Since systemd v240 (commit 8e8b5d2e6d91180a), one can use
/dev/{char,block}-MAJOR syntax to specify that all MAJOR:*
devices are allowed.
Use it, if available, since it's more straightforward, plus
we can skip somewhat expensive parsing of /proc/devices.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently bind mounts of filesystems with nodev, nosuid, noexec,
noatime, relatime, strictatime, nodiratime options set fail in rootless
mode if the same options are not set for the bind mount.
For ro filesystems this was resolved by #2570 by remounting again
with ro set.
Follow the same approach for nodev, nosuid, noexec, noatime, relatime,
strictatime, nodiratime but allow to revert back to the old behaviour
via the new `--no-mount-fallback` command line option.
Add a testcase to verify that bind mounts of filesystems with nodev,
nosuid, noexec, noatime options set work in rootless mode.
Add a testcase that mounts a nodev, nosuid, noexec, noatime filesystem
with a ro flag.
Add two further testcases that ensure that the above testcases would
fail if the `--no-mount-fallback` command line option is set.
* contrib/completions/bash/runc:
Add `--no-mount-fallback` command line option for bash completion.
* create.go:
Add `--no-mount-fallback` command line option.
* restore.go:
Add `--no-mount-fallback` command line option.
* run.go:
Add `--no-mount-fallback` command line option.
* libcontainer/configs/config.go:
Add `NoMountFallback` field to the `Config` struct to store
the command line option value.
* libcontainer/specconv/spec_linux.go:
Add `NoMountFallback` field to the `CreateOpts` struct to store
the command line option value and store it in the libcontainer
config.
* utils_linux.go:
Store the command line option value in the `CreateOpts` struct.
* libcontainer/rootfs_linux.go:
In case that `--no-mount-fallback` is not set try to remount the
bind filesystem again with the options nodev, nosuid, noexec,
noatime, relatime, strictatime or nodiratime if they are set on
the source filesystem.
* tests/integration/mounts_sshfs.bats:
Add testcases and rework sshfs setup to allow specifying
different mount options depending on the test case.
Signed-off-by: Ruediger Pluem <ruediger.pluem@vodafone.com>
We cannot have both srcFD and idMapFD set at the same time.
So, we can simplify this struct to only have one field which is used a srcFD
most of the time and as idMapFD when we do an id map mount.
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Previously to this commit, we used a string for srcFD as /proc/self/fd/NN.
This commit modified to this behavior, so srcFD is only an *int and the full path
is constructed in mountViaFDs() if srcFD is different than nil.
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
This commit adds support for idmap mounts as specified in the runtime-spec.
We open the idmap source paths and call mount_setattr() in runc PARENT,
as we need privileges in the init userns for that, and then sends the
fds to the child process. For this fd passing we use the same mechanism
used in other parts of thecode, the _LIBCONTAINER_ env vars.
The mount is finished (unix.MoveMount) from go code, inside the userns,
so we reuse all the prepareBindMount() security checks and the remount
logic for some flags too.
This commit only supports idmap mounts when userns are used AND the mappings
are the same specified for the userns mapping. This limitation is to
simplify the initial implementation, as all our users so far only need
this, and we can avoid sending over netlink the mappings, creating a
userns with this custom mapping, etc. Future PRs will remove this
limitation.
Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We will need to pass more slices of fds to these functions in future
patches. Let's add a struct that just contains them all, instead of
adding lot of parameters to these functions.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We will add code that uses this function in future patches. So let's
just split it to a new function now and reuse it later.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Let's move the code to send mount sources to a generic function. Future
patches will use it for idmap sources too.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We add idmap.h with the needed includes and defines in case the system
headers don't have the definition for the idmap syscalls we need.
Future patches will use these helpers.
Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We just added this helper for other parts of the code, let's switch this
function to use the helper too.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
This was a warning already and it was requested to make this an error
while we will add validation of idmap mounts:
https://github.com/opencontainers/runc/pull/3717#discussion_r1154705318
I've also tested a k8s cluster and the config.json generated by
containerd didn't use any relative paths. I tested one pod, so it was
definitely not an extensive test.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Processes can watch /proc/self/mounts or /mountinfo, and the kernel
will notify them whenever the namespace's mount table is modified. The
notified process still needs to read and parse the mountinfo to
determine what changed once notified. Many such processes, including
udisksd and SystemD < v248, make no attempt to rate-limit their
mountinfo notifications. This tends to not be a problem on many systems,
where mount tables are small and mounting and unmounting is uncommon.
Every runC exec which successfully uses the try_bindfd container-escape
mitigation performs two mount()s and one umount() in the host's mount
namespace, causing any mount-watching processes to wake up and parse the
mountinfo file three times in a row. Consequently, using 'exec' health
checks on containers has a larger-than-expected impact on system load
when such mount-watching daemons are running. Furthermore, the size of
the mount table in the host's mount namespace tends to be proportional
to the number of OCI containers as a unique mount is required for the
rootfs of each container. Therefore, on systems with mount-watching
processes, the system load increases *quadratically* with the number of
running containers which use health checks!
Prevent runC from incidentally modifying the host's mount namespace for
container-escape mitigations by setting up the mitigation in a temporary
mount namespace.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Modify receive_fd() and send_fd() so they can be more readily reused in
cloned_binary.c. Change receive_fd() to have a single responsibility:
receiving and returning a single file descriptor over a UNIX domain
socket. Make send_fd() useable in precarious execution contexts such as
a clone(CLONE_VFORK|CLONE_VM) "thread" where allocating heap memory or
calling exit() would be dangerous.
Signed-off-by: Cory Snider <csnider@mirantis.com>
runc delete is supposed to remove all the container's artefacts.
In case systemd cgroup driver is used, and the systemd unit has failed
(e.g. oom-killed), systemd won't remove the unit (that is, unless the
"CollectMode: inactive-or-failed" property is set).
Call reset-failed from manager.Destroy so the failed unit will be
removed during "runc delete".
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes we call resetFailedUnit as a cleanup measure, and we don't
care if it fails or not. So, move error reporting to its callers, and
ignore error in cases we don't really expect it to succeed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Replace a panic with a warning, unless it's ENOENT and we're running in
a user namespace. In the latter case, do the same as before, i.e. report
the error but using a Debug logging level.
This prevents software that uses libcontainer from panicking in
some exotic setups.
This will also print a warning on some very old systems which does not
use /sys/fs/cgroup for cgroup mount point. My bet is such systems no
longer exist.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When a directory already exists (or after a container is restarted) the
perms of the directory being mounted to were being used even when a
different permission is set on the tmpfs mount options.
This prepends the original directory perms to the mount options.
If the perms were already set in the mount opts then those perms will
win.
This eliminates the need to perform a chmod after mount entirely.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
We read output from the following files if they exists:
- cpu.pressure
- memory.pressure
- io.pressure
Each are in format:
```
some avg10=0.00 avg60=0.00 avg300=0.00 total=0
full avg10=0.00 avg60=0.00 avg300=0.00 total=0
```
Signed-off-by: Daniel Dao <dqminh89@gmail.com>
Co-authored-by: Sandor Szücs <sandor.szuecs@zalando.de>
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In code we have frozen the cgroup to avoid the processes get
an occasional "permission denied" error, while the systemd's application of device
rules is done disruptively. When the processes in the container can not
be frozen over 2 seconds (which defined in fs/freezer.go),
we still update the cgroup which resulting the container get an occasional
"permission denied" error in some cases.
Return error directly without updating cgroup, when freeze fails.
Fixes: #3803
Signed-off-by: Zoe <hi@zoe.im>
As of previous commit, this is implied in a particular scenario. In
fact, this is the one and only scenario that justifies the use of -a.
Drop the option from the documentation. For backward compatibility, do
recognize it, and retain the feature of ignoring the "container is
stopped" error when set.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
By default, the container has its own PID namespace, and killing (with
SIGKILL) its init process from the parent PID namespace also kills all
the other processes.
Obviously, it does not work that way when the container is sharing its
PID namespace with the host or another container, since init is no
longer special (it's not PID 1). In this case, killing container's init
will result in a bunch of other processes left running (and thus the
inability to remove the cgroup).
The solution to the above problem is killing all the container
processes, not just init.
The problem with the current implementation is, the killing logic is
implemented in libcontainer's initProcess.wait, and thus only available
to libcontainer users, but not the runc kill command (which uses
nonChildProcess.kill and does not use wait at all). So, some workarounds
exist:
- func destroy(c *Container) calls signalAllProcesses;
- runc kill implements -a flag.
This code became very tangled over time. Let's simplify things by moving
the killing all processes from initProcess.wait to container.Signal,
and documents the new behavior.
In essence, this also makes `runc kill` to automatically kill all container
processes when the container does not have its own PID namespace.
Document that as well.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When someone is using libcontainer to start and kill containers from a
long lived process (i.e. the same process creates and removes the
container), initProcess.wait method is used, which has a kludge to work
around killing containers that do not have their own PID namespace.
The code that checks for own PID namespace is not entirely correct.
To be exact, it does not set sharePidns flag when the host/caller PID
namespace is implicitly used. As a result, the above mentioned kludge
does not work.
Fix the issue, add a test case (which fails without the fix).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There are two very distinct usage scenarios for signalAllProcesses:
* when used from the runc binary ("runc kill" command), the processes
that it kills are not the children of "runc kill", and so calling
wait(2) on each process is totally useless, as it will return ECHLD;
* when used from a program that have created the container (such as
libcontainer/integration test suite), that program can and should call
wait(2), not the signalling code.
So, the child reaping code is totally useless in the first case, and
should be implemented by the program using libcontainer in the second
case. I was not able to track down how this code was added, my best
guess is it happened when this code was part of dockerd, which did not
have a proper child reaper implemented at that time.
Remove it, and add a proper documentation piece.
Change the integration test accordingly.
PS the first attempt to disable the child reaping code in
signalAllProcesses was made in commit bb912eb00c, which used a
questionable heuristic to figure out whether wait(2) should be called.
This heuristic worked for a particular use case, but is not correct in
general.
While at it:
- simplify signalAllProcesses to use unix.Kill;
- document (container).Signal.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, TestInit sets up logrus, and init uses it to log an error
from StartInitialization(). This is solely used by TestExecInError
to check that error returned from StartInitialization is the one it
expects.
Note that the very same error is communicated to the runc init parent
and is ultimately returned by container.Run(), so checking what
StartInitialization returned is redundant.
Remove logrus setup and use from TestMain/init.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a cosmetic change to improve code readability, making it easier
to distinguish between a local error and the error being returned.
While at it, rename e to err (it was originally called e to not clash
with returned error named err) and ee to err2.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of having newContainerInit return an interface, and let its
caller call Init(), it is easier to call Init directly.
Do that, and rename newContainerInit to containerInit.
I think it makes the code more readable and straightforward.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Adding fd field to mountConfig was not a good thing since mountConfig
contains data that is not specific to a particular mount, while fd is
a mount entry attribute.
Introduce mountEntry structure, which embeds configs.Mount and adds
srcFd to replace the removed mountConfig.fd.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
lifubang