Merge pull request #3223 from kolyshkin/refuse-bad-cgroup

run create/run/exec: refuse bad cgroup

Author: AkihiroSuda

contrib/completions/bash/runc

@@ -173,6 +173,7 @@ _runc_exec() {
 	   --apparmor
 	   --cap, -c
 	   --preserve-fds
+	   --ignore-paused
 	"
 
 	local all_options="$options_with_args $boolean_options"

exec.go

@@ -91,6 +91,10 @@ following will output a list of processes running in the container:
 			Name:  "cgroup",
 			Usage: "run the process in an (existing) sub-cgroup(s). Format is [<controller>:]<cgroup>.",
 		},
+		cli.BoolFlag{
+			Name:  "ignore-paused",
+			Usage: "allow exec in a paused container",
+		},
 	},
 	Action: func(context *cli.Context) error {
 		if err := checkArgs(context, 1, minArgs); err != nil {
@@ -145,7 +149,10 @@ func execProcess(context *cli.Context) (int, error) {
 		return -1, err
 	}
 	if status == libcontainer.Stopped {
-		return -1, errors.New("cannot exec a container that has stopped")
+		return -1, errors.New("cannot exec in a stopped container")
+	}
+	if status == libcontainer.Paused && !context.Bool("ignore-paused") {
+		return -1, errors.New("cannot exec in a paused container (use --ignore-paused to override)")
 	}
 	path := context.String("process")
 	if path == "" && len(context.Args()) == 1 {

libcontainer/factory_linux.go

@@ -160,14 +160,45 @@ 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 {
-		return nil, err
+
+	// Check that cgroup does not exist or empty (no processes).
+	// Note for cgroup v1 this check is not thorough, as there are multiple
+	// separate hierarchies, while both Exists() and GetAllPids() only use
+	// one for "devices" controller (assuming others are the same, which is
+	// probably true in almost all scenarios). Checking all the hierarchies
+	// would be too expensive.
+	if cm.Exists() {
+		pids, err := cm.GetAllPids()
+		// Reading PIDs can race with cgroups removal, so ignore ENOENT and ENODEV.
+		if err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, unix.ENODEV) {
+			return nil, fmt.Errorf("unable to get cgroup PIDs: %w", err)
+		}
+		if len(pids) != 0 {
+			// TODO: return an error.
+			logrus.Warnf("container's cgroup is not empty: %d process(es) found", len(pids))
+			logrus.Warn("DEPRECATED: running container in a non-empty cgroup won't be supported in runc 1.2; https://github.com/opencontainers/runc/issues/3132")
+		}
 	}
-	cm, err := manager.New(config.Cgroups)
+
+	// Check that cgroup is not frozen. Do not use Exists() here
+	// since in cgroup v1 it only checks "devices" controller.
+	st, err := cm.GetFreezerState()
 	if err != nil {
+		return nil, fmt.Errorf("unable to get cgroup freezer state: %w", err)
+	}
+	if st == configs.Frozen {
+		return nil, errors.New("container's cgroup unexpectedly frozen")
+	}
+
+	if err := os.MkdirAll(containerRoot, 0o711); err != nil {
+		return nil, err
+	}
+	if err := os.Chown(containerRoot, unix.Geteuid(), unix.Getegid()); err != nil {
 		return nil, err
 	}
 	c := &linuxContainer{

man/runc-exec.8.md

@@ -59,6 +59,11 @@ multiple times.
 : Pass _N_ additional file descriptors to the container (**stdio** +
 **$LISTEN_FDS** + _N_ in total). Default is **0**.
 
+**--ignore-paused**
+: Allow exec in a paused container. By default, if a container is paused,
+**runc exec** errors out; this option can be used to override it.
+A paused container needs to be resumed for the exec to complete.
+
 **--cgroup** _path_ | _controller_[,_controller_...]:_path_
 : Execute a process in a sub-cgroup. If the specified cgroup does not exist, an
 error is returned. Default is empty path, which means to use container's top

tests/integration/cgroups.bats

@@ -303,3 +303,109 @@ function setup() {
 	# check that the cgroups v2 path is the same for both processes
 	[[ "$run_cgroup" == "$exec_cgroup" ]]
 }
+
+@test "runc exec should refuse a paused container" {
+	if [[ "$ROOTLESS" -ne 0 ]]; then
+		requires rootless_cgroup
+	fi
+	requires cgroups_freezer
+
+	set_cgroups_path
+
+	runc run -d --console-socket "$CONSOLE_SOCKET" ct1
+	[ "$status" -eq 0 ]
+	runc pause ct1
+	[ "$status" -eq 0 ]
+
+	# Exec should not timeout or succeed.
+	runc exec ct1 echo ok
+	[ "$status" -eq 255 ]
+	[[ "$output" == *"cannot exec in a paused container"* ]]
+}
+
+@test "runc exec --ignore-paused" {
+	if [[ "$ROOTLESS" -ne 0 ]]; then
+		requires rootless_cgroup
+	fi
+	requires cgroups_freezer
+
+	set_cgroups_path
+
+	runc run -d --console-socket "$CONSOLE_SOCKET" ct1
+	[ "$status" -eq 0 ]
+	runc pause ct1
+	[ "$status" -eq 0 ]
+
+	# Resume the container a bit later.
+	(
+		sleep 2
+		runc resume ct1
+	) &
+
+	# Exec should not timeout or succeed.
+	runc exec --ignore-paused ct1 echo ok
+	[ "$status" -eq 0 ]
+	[ "$output" = "ok" ]
+}
+
+@test "runc run/create should warn about a non-empty 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" -eq 0 ]
+	[[ "$output" == *"container's cgroup is not empty"* ]]
+
+	# Same but using runc create.
+	runc create --console-socket "$CONSOLE_SOCKET" ct3
+	[ "$status" -eq 0 ]
+	[[ "$output" == *"container's cgroup is not empty"* ]]
+}
+
+@test "runc run/create should refuse pre-existing frozen cgroup" {
+	requires cgroups_freezer
+	if [[ "$ROOTLESS" -ne 0 ]]; then
+		requires rootless_cgroup
+	fi
+
+	set_cgroups_path
+
+	case $CGROUP_UNIFIED in
+	no)
+		FREEZER_DIR="${CGROUP_FREEZER_BASE_PATH}/${REL_CGROUPS_PATH}"
+		FREEZER="${FREEZER_DIR}/freezer.state"
+		STATE="FROZEN"
+		;;
+	yes)
+		FREEZER_DIR="${CGROUP_PATH}"
+		FREEZER="${FREEZER_DIR}/cgroup.freeze"
+		STATE="1"
+		;;
+	esac
+
+	# Create and freeze the cgroup.
+	mkdir -p "$FREEZER_DIR"
+	echo "$STATE" >"$FREEZER"
+
+	# Start a container.
+	runc run -d --console-socket "$CONSOLE_SOCKET" ct1
+	[ "$status" -eq 1 ]
+	# A warning should be printed.
+	[[ "$output" == *"container's cgroup unexpectedly frozen"* ]]
+
+	# Same check for runc create.
+	runc create --console-socket "$CONSOLE_SOCKET" ct2
+	[ "$status" -eq 1 ]
+	# A warning should be printed.
+	[[ "$output" == *"container's cgroup unexpectedly frozen"* ]]
+
+	# Cleanup.
+	rmdir "$FREEZER_DIR"
+}