From 2a71e3d286ed77e6034d614723d88a9b3bed4b71 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Tue, 17 Aug 2021 16:56:12 -0700
Subject: [PATCH] runc run: refuse existing cgroup

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.

All this may lead to various hard-to-debug situations in production
(runc init stuck, cgroup removal error, wrong resource limits etc).

One obvious solution is to require a non-existent cgroup for the
new container, or fail if a cgroup already exists.

This is exactly what this commit implements.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/error.go          |  1 +
 libcontainer/factory_linux.go  | 13 +++++++++----
 tests/integration/cgroups.bats | 21 +++++++++++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/libcontainer/error.go b/libcontainer/error.go
index bd5ad1f8bf7..cb1532b5e4b 100644
--- a/libcontainer/error.go
+++ b/libcontainer/error.go
@@ -10,6 +10,7 @@ var (
 	ErrRunning    = errors.New("container still running")
 	ErrNotRunning = errors.New("container not running")
 	ErrNotPaused  = errors.New("container not paused")
+	ErrCgExist    = errors.New("container's cgroup already exists")
 )
 
 type ConfigError struct {
diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go
index 522bf1de667..77d926f5dfc 100644
--- a/libcontainer/factory_linux.go
+++ b/libcontainer/factory_linux.go
@@ -160,14 +160,19 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err
 	} else if !os.IsNotExist(err) {
 		return nil, err
 	}
-	if err := os.MkdirAll(containerRoot, 0o711); err != nil {
+
+	cm, err := manager.New(config.Cgroups)
+	if err != nil {
 		return nil, err
 	}
-	if err := os.Chown(containerRoot, unix.Geteuid(), unix.Getegid()); err != nil {
+	if cm.Exists() {
+		return nil, ErrCgExist
+	}
+
+	if err := os.MkdirAll(containerRoot, 0o711); err != nil {
 		return nil, err
 	}
-	cm, err := manager.New(config.Cgroups)
-	if err != nil {
+	if err := os.Chown(containerRoot, unix.Geteuid(), unix.Getegid()); err != nil {
 		return nil, err
 	}
 	c := &linuxContainer{
diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats
index 7b8603db867..ecd2c04961b 100644
--- a/tests/integration/cgroups.bats
+++ b/tests/integration/cgroups.bats
@@ -301,3 +301,24 @@ function setup() {
 	[ "$status" -eq 255 ]
 	[[ "$output" == *"container cgroup is frozen"* ]]
 }
+
+@test "runc run/create should refuse an existing cgroup" {
+	if [[ "$ROOTLESS" -ne 0 ]]; then
+		requires rootless_cgroup
+	fi
+
+	set_cgroups_path
+
+	runc run -d --console-socket "$CONSOLE_SOCKET" ct1
+	[ "$status" -eq 0 ]
+
+	# Run a second container sharing the cgroup with the first one.
+	runc --debug run -d --console-socket "$CONSOLE_SOCKET" ct2
+	[ "$status" -ne 0 ]
+	[[ "$output" == *"container's cgroup already exists"* ]]
+
+	# Same but using runc create.
+	runc create --console-socket "$CONSOLE_SOCKET" ct3
+	[ "$status" -ne 0 ]
+	[[ "$output" == *"container's cgroup already exists"* ]]
+}