Merge pull request #12952 from chrishenzie/mount-option-removal

Preserve cgroup mount options for privileged containers

Vagrantfile

@@ -278,6 +278,7 @@ EOF
         'GOTESTSUM_JSONFILE': ENV['GOTESTSUM_JSONFILE'],
         'GITHUB_WORKSPACE': '',
         'CGROUP_DRIVER': ENV['CGROUP_DRIVER'],
+        'RUNC_FLAVOR': ENV['RUNC_FLAVOR'] || "runc",
     }
     sh.inline = <<~SHELL
         #!/usr/bin/env bash
@@ -306,6 +307,7 @@ EOF
         'GOTEST': ENV['GOTEST'] || "go test",
         'REPORT_DIR': ENV['REPORT_DIR'],
         'CGROUP_DRIVER': ENV['CGROUP_DRIVER'],
+        'RUNC_FLAVOR': ENV['RUNC_FLAVOR'] || "runc",
     }
     sh.inline = <<~SHELL
         #!/usr/bin/env bash

integration/container_cgroup_mount_options_linux_test.go

@@ -0,0 +1,79 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package integration
+
+import (
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/containerd/cgroups/v3"
+	"github.com/containerd/containerd/v2/core/mount"
+	"github.com/containerd/containerd/v2/integration/images"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPrivilegedContainerCgroupMountOptions(t *testing.T) {
+	if f := os.Getenv("RUNC_FLAVOR"); f == "crun" {
+		t.Skip("Skipping until crun supports cgroup v2 mount options (https://github.com/containers/crun/pull/2040)")
+	}
+	if cgroups.Mode() != cgroups.Unified {
+		t.Skip("Requires cgroup v2")
+	}
+
+	hostMountBefore, err := mount.Lookup("/sys/fs/cgroup")
+	require.NoError(t, err)
+
+	if !strings.Contains(hostMountBefore.VFSOptions, "nsdelegate") && !strings.Contains(hostMountBefore.VFSOptions, "memory_recursiveprot") {
+		t.Skip("requires host cgroup mount to have nsdelegate or memory_recursiveprot")
+	}
+
+	testImage := images.Get(images.BusyBox)
+	EnsureImageExists(t, testImage)
+
+	t.Log("Create a sandbox with privileged=true")
+	sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", "privileged-cgroup-mount-test", WithPodSecurityContext(true))
+
+	t.Log("Create a container with privileged=true")
+	cnConfig := ContainerConfig("container", testImage, WithCommand("sh", "-c", "sleep 1d"), WithSecurityContext(true))
+	cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		if err := runtimeService.RemoveContainer(cn); err != nil {
+			t.Logf("failed to remove container %s: %v", cn, err)
+		}
+	})
+
+	t.Log("Start the container")
+	require.NoError(t, runtimeService.StartContainer(cn))
+	t.Cleanup(func() {
+		if err := runtimeService.StopContainer(cn, 10); err != nil {
+			t.Logf("failed to stop container %s: %v", cn, err)
+		}
+	})
+
+	hostMountAfter, err := mount.Lookup("/sys/fs/cgroup")
+	require.NoError(t, err)
+
+	if strings.Contains(hostMountBefore.VFSOptions, "nsdelegate") {
+		assert.Contains(t, hostMountAfter.VFSOptions, "nsdelegate", "nsdelegate should be preserved on the host cgroup mount")
+	}
+	if strings.Contains(hostMountBefore.VFSOptions, "memory_recursiveprot") {
+		assert.Contains(t, hostMountAfter.VFSOptions, "memory_recursiveprot", "memory_recursiveprot should be preserved on the host cgroup mount")
+	}
+}

internal/cri/opts/spec_linux_opts.go

@@ -23,6 +23,7 @@ import (
 	"maps"
 	"os"
 	"path/filepath"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -71,11 +72,35 @@ func withMounts(osi osinterface.OS, config *runtime.ContainerConfig, extra []*ru
 		if cgroupWritable {
 			mode = "rw"
 		}
+
+		cgroupOptions := []string{"nosuid", "noexec", "nodev", "relatime", mode}
+
+		hasCgroupNS := false
+		if s.Linux != nil {
+			hasCgroupNS = slices.ContainsFunc(s.Linux.Namespaces, func(ns runtimespec.LinuxNamespace) bool {
+				return ns.Type == runtimespec.CgroupNamespace
+			})
+		}
+
+		// If a container shares the host's cgroup namespace, mounting cgroup2
+		// inside the container applies the new mount options to the single shared
+		// cgroup2 VFS superblock. Therefore, explicitly copy these options from
+		// the host's /sys/fs/cgroup to avoid being stripped.
+		if !hasCgroupNS {
+			if mountInfo, err := osi.LookupMount("/sys/fs/cgroup"); err == nil {
+				for opt := range strings.SplitSeq(mountInfo.VFSOptions, ",") {
+					if opt == "nsdelegate" || opt == "memory_recursiveprot" {
+						cgroupOptions = append(cgroupOptions, opt)
+					}
+				}
+			}
+		}
+
 		s.Mounts = append(s.Mounts, runtimespec.Mount{
 			Source:      "cgroup",
 			Destination: "/sys/fs/cgroup",
 			Type:        "cgroup",
-			Options:     []string{"nosuid", "noexec", "nodev", "relatime", mode},
+			Options:     cgroupOptions,
 		})
 
 		// Copy all mounts from default mounts, except for

internal/cri/opts/spec_linux_test.go

@@ -17,10 +17,15 @@
 package opts
 
 import (
+	"context"
 	"testing"
 
+	"github.com/containerd/containerd/v2/core/mount"
+	ostesting "github.com/containerd/containerd/v2/pkg/os/testing"
+	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
 )
 
 func TestMergeGids(t *testing.T) {
@@ -45,3 +50,73 @@ func TestRestrictOOMScoreAdj(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, got, current+1)
 }
+
+func TestWithMountsCgroupNamespaceOptions(t *testing.T) {
+	tests := []struct {
+		name          string
+		hasCgroupNS   bool
+		hostMountOpts string
+		expectedOpts  []string
+	}{
+		{
+			name:          "has cgroupns, should use default options",
+			hasCgroupNS:   true,
+			hostMountOpts: "rw,nosuid,nodev,noexec,relatime,nsdelegate,memory_recursiveprot",
+			expectedOpts:  []string{"nosuid", "noexec", "nodev", "relatime", "ro"},
+		},
+		{
+			name:          "no cgroupns, with host options present",
+			hasCgroupNS:   false,
+			hostMountOpts: "rw,nosuid,nodev,noexec,relatime,nsdelegate,memory_recursiveprot",
+			expectedOpts:  []string{"nosuid", "noexec", "nodev", "relatime", "ro", "nsdelegate", "memory_recursiveprot"},
+		},
+		{
+			name:          "no cgroupns, with host missing nsdelegate",
+			hasCgroupNS:   false,
+			hostMountOpts: "rw,nosuid,nodev,noexec,relatime,memory_recursiveprot",
+			expectedOpts:  []string{"nosuid", "noexec", "nodev", "relatime", "ro", "memory_recursiveprot"},
+		},
+		{
+			name:          "no cgroupns, with host missing all extra options",
+			hasCgroupNS:   false,
+			hostMountOpts: "rw,nosuid,nodev,noexec,relatime",
+			expectedOpts:  []string{"nosuid", "noexec", "nodev", "relatime", "ro"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fakeOS := ostesting.NewFakeOS()
+			fakeOS.LookupMountFn = func(path string) (mount.Info, error) {
+				if path == "/sys/fs/cgroup" {
+					return mount.Info{VFSOptions: tt.hostMountOpts}, nil
+				}
+				return mount.Info{}, nil
+			}
+
+			config := &runtime.ContainerConfig{
+				Linux: &runtime.LinuxContainerConfig{},
+			}
+
+			spec := &runtimespec.Spec{}
+			if tt.hasCgroupNS {
+				spec.Linux = &runtimespec.Linux{Namespaces: []runtimespec.LinuxNamespace{{Type: runtimespec.CgroupNamespace}}}
+			}
+
+			opt := withMounts(fakeOS, config, nil, "", nil, false)
+			err := opt(context.Background(), nil, nil, spec)
+			require.NoError(t, err)
+
+			var cgroupMount *runtimespec.Mount
+			for _, m := range spec.Mounts {
+				if m.Destination == "/sys/fs/cgroup" {
+					cgroupMount = &m
+					break
+				}
+			}
+
+			require.NotNil(t, cgroupMount)
+			assert.ElementsMatch(t, tt.expectedOpts, cgroupMount.Options)
+		})
+	}
+}

internal/cri/server/container_create.go

@@ -792,6 +792,14 @@ func (c *criService) buildLinuxSpec(
 		}
 	}()
 
+	// cgroupns is used for hiding /sys/fs/cgroup from containers.
+	// For compatibility, cgroupns is not used when running in cgroup v1 mode or in privileged.
+	// https://github.com/containers/libpod/issues/4363
+	// https://github.com/kubernetes/enhancements/blob/0e409b47497e398b369c281074485c8de129694f/keps/sig-node/20191118-cgroups-v2.md#cgroup-namespace
+	if isUnifiedCgroupsMode() && !securityContext.GetPrivileged() {
+		specOpts = append(specOpts, oci.WithLinuxNamespace(runtimespec.LinuxNamespace{Type: runtimespec.CgroupNamespace}))
+	}
+
 	var ociSpecOpts oci.SpecOpts
 	if ociRuntime.CgroupWritable {
 		ociSpecOpts = customopts.WithMountsCgroupWritable(c.os, config, extraMounts, mountLabel, runtimeHandler)
@@ -930,14 +938,6 @@ func (c *criService) buildLinuxSpec(
 		annotations.DefaultCRIAnnotations(sandboxID, containerName, imageName, sandboxConfig, false)...,
 	)
 
-	// cgroupns is used for hiding /sys/fs/cgroup from containers.
-	// For compatibility, cgroupns is not used when running in cgroup v1 mode or in privileged.
-	// https://github.com/containers/libpod/issues/4363
-	// https://github.com/kubernetes/enhancements/blob/0e409b47497e398b369c281074485c8de129694f/keps/sig-node/20191118-cgroups-v2.md#cgroup-namespace
-	if isUnifiedCgroupsMode() && !securityContext.GetPrivileged() {
-		specOpts = append(specOpts, oci.WithLinuxNamespace(runtimespec.LinuxNamespace{Type: runtimespec.CgroupNamespace}))
-	}
-
 	return specOpts, nil
 }
 

internal/cri/server/container_create_linux_test.go

@@ -487,6 +487,52 @@ func TestPrivilegedBindMount(t *testing.T) {
 	}
 }
 
+func TestCgroupNamespace(t *testing.T) {
+	testPid := uint32(1234)
+	c := newTestCRIService()
+	testSandboxID := "sandbox-id"
+	testContainerName := "container-name"
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
+
+	tests := []struct {
+		desc                  string
+		privileged            bool
+		expectCgroupNamespace bool
+	}{
+		{
+			desc:                  "non-privileged container should get cgroup namespace",
+			privileged:            false,
+			expectCgroupNamespace: true,
+		},
+		{
+			desc:                  "privileged container should not get cgroup namespace",
+			privileged:            true,
+			expectCgroupNamespace: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			containerConfig.Linux.SecurityContext.Privileged = tt.privileged
+			sandboxConfig.Linux.SecurityContext.Privileged = tt.privileged
+
+			spec, err := c.buildContainerSpec(currentPlatform, t.Name(), testSandboxID, testPid, "", testContainerName, testImageName, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime, nil)
+			assert.NoError(t, err)
+
+			hasCgroupNS := false
+			for _, ns := range spec.Linux.Namespaces {
+				if ns.Type == runtimespec.CgroupNamespace {
+					hasCgroupNS = true
+					break
+				}
+			}
+
+			assert.Equal(t, tt.expectCgroupNamespace, hasCgroupNS)
+		})
+	}
+}
+
 func TestMountPropagation(t *testing.T) {
 
 	sharedLookupMountFn := func(string) (mount.Info, error) {

script/test/cri-integration.sh

@@ -46,6 +46,10 @@ CMD=""
 if [ -n "${sudo}" ]; then
   CMD+="${sudo} "
 fi
+CMD+="env "
+if [ -n "${RUNC_FLAVOR:-}" ]; then
+  CMD+="RUNC_FLAVOR=${RUNC_FLAVOR} "
+fi
 CMD+="${PWD}/bin/cri-integration.test"
 
 ${CMD} --test.run="${FOCUS}" --test.v \