Merge pull request #4398 from kolyshkin/no-shared-pidns

runc create/run: warn on rootless + shared pidns + no cgroup

Author: kolyshkin

libcontainer/cgroups/cgroups.go

@@ -11,6 +11,11 @@ var (
 	// is not configured to set device rules.
 	ErrDevicesUnsupported = errors.New("cgroup manager is not configured to set device rules")
 
+	// ErrRootless is returned by [Manager.Apply] when there is an error
+	// creating cgroup directory, and cgroup.Rootless is set. In general,
+	// this error is to be ignored.
+	ErrRootless = errors.New("cgroup manager can not access cgroup (rootless container)")
+
 	// DevicesSetV1 and DevicesSetV2 are functions to set devices for
 	// cgroup v1 and v2, respectively. Unless
 	// [github.com/opencontainers/runc/libcontainer/cgroups/devices]

libcontainer/cgroups/fs/fs.go

@@ -105,7 +105,7 @@ func isIgnorableError(rootless bool, err error) bool {
 	return false
 }
 
-func (m *Manager) Apply(pid int) (err error) {
+func (m *Manager) Apply(pid int) (retErr error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -129,14 +129,15 @@ func (m *Manager) Apply(pid int) (err error) {
 			// later by Set, which fails with a friendly error (see
 			// if path == "" in Set).
 			if isIgnorableError(c.Rootless, err) && c.Path == "" {
+				retErr = cgroups.ErrRootless
 				delete(m.paths, name)
 				continue
 			}
 			return err
 		}
 
 	}
-	return nil
+	return retErr
 }
 
 func (m *Manager) Destroy() error {

libcontainer/cgroups/fs2/fs2.go

@@ -71,7 +71,7 @@ func (m *Manager) Apply(pid int) error {
 		if m.config.Rootless {
 			if m.config.Path == "" {
 				if blNeed, nErr := needAnyControllers(m.config.Resources); nErr == nil && !blNeed {
-					return nil
+					return cgroups.ErrRootless
 				}
 				return fmt.Errorf("rootless needs no limits + no cgrouppath when no permission is granted for cgroups: %w", err)
 			}

libcontainer/configs/validate/rootless.go

@@ -34,7 +34,7 @@ func rootlessEUIDMappings(config *configs.Config) error {
 		return errors.New("rootless container requires user namespaces")
 	}
 	// We only require mappings if we are not joining another userns.
-	if path := config.Namespaces.PathOf(configs.NEWUSER); path == "" {
+	if config.Namespaces.IsPrivate(configs.NEWUSER) {
 		if len(config.UIDMappings) == 0 {
 			return errors.New("rootless containers requires at least one UID mapping")
 		}

libcontainer/process_linux.go

@@ -580,7 +580,18 @@ func (p *initProcess) start() (retErr error) {
 	// cgroup. We don't need to worry about not doing this and not being root
 	// because we'd be using the rootless cgroup manager in that case.
 	if err := p.manager.Apply(p.pid()); err != nil {
-		return fmt.Errorf("unable to apply cgroup configuration: %w", err)
+		if errors.Is(err, cgroups.ErrRootless) {
+			// ErrRootless is to be ignored except when
+			// the container doesn't have private pidns.
+			if !p.config.Config.Namespaces.IsPrivate(configs.NEWPID) {
+				// TODO: make this an error in runc 1.3.
+				logrus.Warn("Creating a rootless container with no cgroup and no private pid namespace. " +
+					"Such configuration is strongly discouraged (as it is impossible to properly kill all container's processes) " +
+					"and will result in an error in a future runc version.")
+			}
+		} else {
+			return fmt.Errorf("unable to apply cgroup configuration: %w", err)
+		}
 	}
 	if p.intelRdtManager != nil {
 		if err := p.intelRdtManager.Apply(p.pid()); err != nil {

libcontainer/specconv/spec_linux.go

@@ -415,7 +415,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 			}
 			config.Namespaces.Add(t, ns.Path)
 		}
-		if config.Namespaces.Contains(configs.NEWNET) && config.Namespaces.PathOf(configs.NEWNET) == "" {
+		if config.Namespaces.IsPrivate(configs.NEWNET) {
 			config.Networks = []*configs.Network{
 				{
 					Type: "loopback",
@@ -481,7 +481,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 	// Only set it if the container will have its own cgroup
 	// namespace and the cgroupfs will be mounted read/write.
 	//
-	hasCgroupNS := config.Namespaces.Contains(configs.NEWCGROUP) && config.Namespaces.PathOf(configs.NEWCGROUP) == ""
+	hasCgroupNS := config.Namespaces.IsPrivate(configs.NEWCGROUP)
 	hasRwCgroupfs := false
 	if hasCgroupNS {
 		for _, m := range config.Mounts {

tests/integration/create.bats

@@ -81,3 +81,31 @@ function teardown() {
 
 	testcontainer test_busybox running
 }
+
+# https://github.com/opencontainers/runc/issues/4394#issuecomment-2334926257
+@test "runc create [shared pidns + rootless]" {
+	# Remove pidns so it's shared with the host.
+	update_config '	  .linux.namespaces -= [{"type": "pid"}]'
+	if [ $EUID -ne 0 ]; then
+		if rootless_cgroup; then
+			# Rootless containers have empty cgroup path by default.
+			set_cgroups_path
+		fi
+		# Can't mount real /proc when rootless + no pidns,
+		# so change it to a bind-mounted one from the host.
+		update_config '	  .mounts |= map((select(.type == "proc")
+                                | .type = "none"
+                                | .source = "/proc"
+                                | .options = ["rbind", "nosuid", "nodev", "noexec"]
+                          ) // .)'
+	fi
+
+	exp="Such configuration is strongly discouraged"
+	runc create --console-socket "$CONSOLE_SOCKET" test
+	[ "$status" -eq 0 ]
+	if [ $EUID -ne 0 ] && ! rootless_cgroup; then
+		[[ "$output" = *"$exp"* ]]
+	else
+		[[ "$output" != *"$exp"* ]]
+	fi
+}

tests/integration/helpers.bash

@@ -727,6 +727,8 @@ function teardown_bundle() {
 	[ ! -v ROOT ] && return 0 # nothing to teardown
 
 	cd "$INTEGRATION_ROOT" || return
+	echo "--- teardown ---" >&2
+
 	teardown_recvtty
 	local ct
 	for ct in $(__runc list -q); do