In a case where the host /proc mount has already been overmounted, the
MS_MOVE handling would get ENOENT when trying to hide (for instance)
"/proc/bus" because it had already hidden away "/proc". This revealed
two issues in the previous implementation of this hardening feaure:
1. No checks were done to make sure the mount was a "full" mount (it is
a mount of the root of the filesystem), but the kernel doesn't permit
a non-full mount to be converted to a full mount (for reference, see
mnt_already_visible). This just removes extra busy-work during setup.
2. ENOENT was treated as a critical error, even though it actually
indicates the mount doesn't exist and thus isn't a problem. A more
theoretically pure solution would be to store the set of mountpoints
to be hidden and only ignore the error if an ancestor directory of
the current mountpoint was already hidden, but that would just add
complexity with little justification.
In addition, better document the reasoning behind this logic so that
folks aren't confused when looking at it.
Fixes: 28a697cce3 ("rootfs: umount all procfs and sysfs with --no-pivot")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
If the values of CMT and MBM counters are zero, they will be omitted as
empty items when getting Intel RDT stats.
Remove 'omitempty' property from CMT and MBM counters to display zero
values.
Signed-off-by: Xiaochen Shen <xiaochen.shen@intel.com>
If the cgroup's init process doesn't complete successfully, Wait returns a
non-nil error. We should still kill all the process in the cgroup if process
namespace is shared. Otherwise, it may result in process leak.
Fixes#2632
Signed-off-by: Chaitanya Bandi <kbandi@cs.stonybrook.edu>
While at it,
- change some functions to not be methods of CpusetCgroup as
they don't use any members;
- simplify isEmpty.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
fscommon.WriteFile is added specifically to work with cgroup files,
and the error it returns does not need to be wrapped.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This removes package dependency on cgroup, as following commits
make cgroup use fscommon, which would result in dependency cycle.
The code to find out memory cgroup root is not really needed,
as 99% of test envrionments will have it at /sys/fs/cgroup/memory.
If not, that means we're either on cgroupv2 or on some very custom
system, so just skip the test.
The code that checks if we're on cgroupv2 is replaced by the check
of the particular v1 control file.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
GetProcessStartTime was deprecated over three Years ago in
439eaa3584, so we may as well remove
it now.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
errors.Wrap(err, "some error") returns nil if err is nil, so it's
slightly clearer to just return early than to set the error to nil
and call errors.Wrap(). This is also somewhat defensive in case
we decide to replace `errors.Wrap()` for golang's native `%w`
wrapping.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This is a function to convert huge page sizes (obtained by reading
/sys/kernel/mm/hugepages directory entries) to strings user for hugetlb
cgroup controller resource files. Those strings are when used to get the
hugetlb resource statistics.
This function used external library, floating point numbers, and can
(theoretically) produce invalid values, since the kernel only uses KB,
MB, and GB suffixes.
Rewrite it to produce the same strings as used in the kernel (see [1]).
As a result, it's also faster, more future-proof (entries that do not
start with "hugepages-" and/or incorrect suffix are skipped), and does
more input sanity checks. As a side effect, libcontainer no longer
depends on docker/go-units.
While at it, add more test cases.
Before:
BenchmarkGetHugePageSize-8 187452 6265 ns/op
BenchmarkGetHugePageSizeImpl-8 396769 2998 ns/op
After:
BenchmarkGetHugePageSize-8 222898 4554 ns/op
BenchmarkGetHugePageSizeImpl-8 4738924 241 ns/op
NOTE on removing HugePageSizeUnitList -- this was added by commit
6f77e35da and was used by kubernetes code in [2], which was later
superceded by [3], so there are (hopefully) no external users.
If there are any, they should not be doing that.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/hugetlb_cgroup.c?id=eff48ddeab782e35e58ccc8853f7386bbae9dec4#n574
[2] https://github.com/kubernetes/kubernetes/pull/78495
[3] https://github.com/kubernetes/kubernetes/pull/84154
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When we call terminate(), we kill the process, and wait
returns the error indicating the process was killed.
This is exactly what we expect here, so there is no reason
to treat it as an error.
Before this patch, when a container with invalid cgroup parameters is
started:
> WARN[0000] unable to terminate initProcess error="signal: killed"
> ERRO[0000] container_linux.go:366: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write "555": open /sys/fs/cgroup/blkio/user.slice/xx33/blkio.weight: permission denied
After:
> ERRO[0000] container_linux.go:366: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write "555": open /sys/fs/cgroup/blkio/user.slice/xx33/blkio.weight: permission denied
I.e. the useless warning is gone.
NOTE this breaks a couple of integration test cases, since they were
expecting a particular message in the second line, and now due to
"signal: killed" removed it's in the first line. Fix those, too.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case cgroup configuration is invalid (some parameters can't be set
etc.), p.manager.Set fails, the error is returned, and then we try to
remove cgroups (by calling p.manager.Destroy) in a defer.
The problem is, the container init is not yet killed (as it is killed in
the caller, i.e. (*linuxContainer).start), so cgroup removal fails like
this:
> time="2020-09-26T07:46:25Z" level=warning msg="Failed to remove cgroup (will retry)" error="rmdir /sys/fs/cgroup/net_cls,net_prio/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod28ce6e74_694c_4b77_a953_dc01e182ac76.slice/crio-f6984c5eeb6c6b49ff3f036bdcb9ded317b3d0b2469ebbb35705442a2afd98c2.scope: device or resource busy"
> ...
> time="2020-09-26T07:46:27Z" level=error msg="Failed to remove cgroup" error="rmdir /sys/fs/cgroup/net_cls,net_prio/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod28ce6e74_694c_4b77_a953_dc01e182ac76.slice/crio-f6984c5eeb6c6b49ff3f036bdcb9ded317b3d0b2469ebbb35705442a2afd98c2.scope: device or resource busy"
The above is repeated for every controller, and looks quite scary.
To fix, move the init termination to the abovementioned defer.
Do the same for (*setnsProcess).start() for uniformity.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is not a good practice to have the name `err` for the error returned
by a function. Switch to `retErr`.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case it takes more than 1 second for systemd to create a unit,
startUnit() times out with a warning and then runc proceeds
(to create cgroups using fs manager and so on).
Now runc and systemd are racing, and multiple scenarios are possible.
In one such scenario, by the time runc calls systemd manager's Apply()
the unit is not yet created, the dbusConnection.SetUnitProperties()
call fails with "unit xxx.scope not found", and the whole container
start also fails.
To eliminate the race, we need to return an error in case the timeout is
hit.
To reduce the chance to fail, increase the timeout from 1 to 30 seconds,
to not error out too early on a busy/slow system (and times like 3-5
seconds are not unrealistic).
While at it, as the timeout is quite long now, make sure to not leave
a stray timer.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Mrunal Patel