[release-1.27] server: allow Bidirectional mounts that contain storage root

server/container_create_linux.go

@@ -324,7 +324,7 @@
 	cgroup2RW := node.CgroupIsV2() && sb.Annotations()[crioann.Cgroup2RWAnnotation] == "true"
 
 	s.resourceStore.SetStageForResource(ctx, ctr.Name(), "container volume configuration")
-	containerVolumes, ociMounts, err := addOCIBindMounts(ctx, ctr, mountLabel, s.config.RuntimeConfig.BindMountPrefix, s.config.AbsentMountSourcesToReject, maybeRelabel, skipRelabel, cgroup2RW)
+	containerVolumes, ociMounts, err := addOCIBindMounts(ctx, ctr, mountLabel, s.config.RuntimeConfig.BindMountPrefix, s.config.AbsentMountSourcesToReject, maybeRelabel, skipRelabel, cgroup2RW, s.Config().Root)
 	if err != nil {
 		return nil, err
 	}
@@ -923,7 +923,7 @@
 	m.Options = append(m.Options, "rw")
 }
 
-func addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, mountLabel, bindMountPrefix string, absentMountSourcesToReject []string, maybeRelabel, skipRelabel, cgroup2RW bool) ([]oci.ContainerVolume, []rspec.Mount, error) {
+func addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, mountLabel, bindMountPrefix string, absentMountSourcesToReject []string, maybeRelabel, skipRelabel, cgroup2RW bool, storageRoot string) ([]oci.ContainerVolume, []rspec.Mount, error) {
 	ctx, span := log.StartSpan(ctx)
 	defer span.End()
 
@@ -978,6 +978,12 @@
 		if m.HostPath == "/" && dest == "/" {
 			log.Warnf(ctx, "Configuration specifies mounting host root to the container root.  This is dangerous (especially with privileged containers) and should be avoided.")
 		}
+
+		if isSubDirectoryOf(storageRoot, m.HostPath) && m.Propagation == types.MountPropagation_PROPAGATION_PRIVATE {
+			log.Infof(ctx, "Mount propogration for the host path %s will be set to HostToContainer as it includes the container storage root", m.HostPath)
+			m.Propagation = types.MountPropagation_PROPAGATION_HOST_TO_CONTAINER
+		}
+
 		src := filepath.Join(bindMountPrefix, m.HostPath)
 
 		resolvedSrc, err := resolveSymbolicLink(bindMountPrefix, src)
@@ -1201,3 +1207,28 @@
 		Apparmor:         &types.SecurityProfile{},
 	}
 }
+
+// isSubDirectoryOf checks if the base path contains the target path.
+// It assumes that paths are Unix-style with forward slashes ("/").
+// It ensures that both paths end with a "/" before comparing, so that "/var/lib" will not incorrectly match "/var/libs".
+
+// The function returns true if the base path starts with the target path, providing a way to check if one directory is a subdirectory of another.
+
+// Examples:
+
+// isSubDirectoryOf("/var/lib/containers/storage", "/") returns true
+// isSubDirectoryOf("/var/lib/containers/storage", "/var/lib") returns true
+// isSubDirectoryOf("/var/lib/containers/storage", "/var/lib/containers") returns true
+// isSubDirectoryOf("/var/lib/containers/storage", "/var/lib/containers/storage") returns true
+// isSubDirectoryOf("/var/lib/containers/storage", "/var/lib/containers/storage/extra") returns false
+// isSubDirectoryOf("/var/lib/containers/storage", "/va") returns false
+// isSubDirectoryOf("/var/lib/containers/storage", "/var/tmp/containers") returns false
+func isSubDirectoryOf(base, target string) bool {
+	if !strings.HasSuffix(target, "/") {
+		target += "/"
+	}
+	if !strings.HasSuffix(base, "/") {
+		base += "/"
+	}
+	return strings.HasPrefix(base, target)
+}

server/container_create_linux_test.go

@@ -31,7 +31,7 @@ func TestAddOCIBindsForDev(t *testing.T) {
 		t.Error(err)
 	}
 
-	_, binds, err := addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, false)
+	_, binds, err := addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, false, "")
 	if err != nil {
 		t.Error(err)
 	}
@@ -75,7 +75,7 @@ func TestAddOCIBindsForSys(t *testing.T) {
 		t.Error(err)
 	}
 
-	_, binds, err := addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, false)
+	_, binds, err := addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, false, "")
 	if err != nil {
 		t.Error(err)
 	}
@@ -107,7 +107,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) {
 	}); err != nil {
 		t.Error(err)
 	}
-	_, _, err = addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, true)
+	_, _, err = addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, true, "")
 	if err != nil {
 		t.Error(err)
 	}
@@ -141,7 +141,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) {
 		t.Error(err)
 	}
 	var hasCgroupRO bool
-	_, _, err = addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, false)
+	_, _, err = addOCIBindMounts(context.Background(), ctr, "", "", nil, false, false, false, "")
 	if err != nil {
 		t.Error(err)
 	}
@@ -158,3 +158,28 @@ func TestAddOCIBindsCGroupRW(t *testing.T) {
 		t.Error("Cgroup mount not added with RO.")
 	}
 }
+
+func TestIsSubDirectoryOf(t *testing.T) {
+	tests := []struct {
+		base, target string
+		want         bool
+	}{
+		{"/var/lib/containers/storage", "/", true},
+		{"/var/lib/containers/storage", "/var/lib", true},
+		{"/var/lib/containers/storage", "/var/lib/containers", true},
+		{"/var/lib/containers/storage", "/var/lib/containers/storage", true},
+		{"/var/lib/containers/storage", "/var/lib/containers/storage/extra", false},
+		{"/var/lib/containers/storage", "/va", false},
+		{"/var/lib/containers/storage", "/var/tmp/containers", false},
+	}
+
+	for _, tt := range tests {
+		testname := tt.base + " " + tt.target
+		t.Run(testname, func(t *testing.T) {
+			ans := isSubDirectoryOf(tt.base, tt.target)
+			if ans != tt.want {
+				t.Errorf("got %v, want %v", ans, tt.want)
+			}
+		})
+	}
+}

test/ctr.bats

@@ -1004,6 +1004,42 @@ function check_oci_annotation() {
 	! crictl create "$pod_id" "$TESTDIR/config" "$TESTDATA"/sandbox_config.json
 }
 
+@test "ctr that mounts container storage as shared should keep shared" {
+	# parent of `--root`, keep in sync with test/helpers.bash
+	PARENT_DIR="$TESTDIR"
+	CTR_DIR="/host"
+	jq --arg path "$PARENT_DIR" --arg ctr_dir "$CTR_DIR" \
+		'  .mounts = [ {
+			host_path: $path,
+			container_path: $ctr_dir,
+			propagation: 2
+		} ]' \
+		"$TESTDATA"/container_redis.json > "$TESTDIR/config"
+
+	start_crio
+
+	ctr_id=$(crictl run "$TESTDIR/config" "$TESTDATA"/sandbox_config.json)
+	crictl exec --sync "$ctr_id" findmnt -no TARGET,PROPAGATION "$CTR_DIR" | grep shared
+}
+
+@test "ctr that mounts container storage as private should not be private" {
+	# parent of `--root`, keep in sync with test/helpers.bash
+	PARENT_DIR="$TESTDIR"
+	CTR_DIR="/host"
+	jq --arg path "$PARENT_DIR" --arg ctr_dir "$CTR_DIR" \
+		'  .mounts = [ {
+			host_path: $path,
+			container_path: $ctr_dir,
+			propagation: 1
+		} ]' \
+		"$TESTDATA"/container_redis.json > "$TESTDIR/config"
+
+	start_crio
+
+	ctr_id=$(crictl run "$TESTDIR/config" "$TESTDATA"/sandbox_config.json)
+	crictl exec --sync "$ctr_id" findmnt -no TARGET,PROPAGATION "$CTR_DIR" | grep -v private
+}
+
 @test "ctr has containerenv" {
 	start_crio
 	pod_id=$(crictl runp "$TESTDATA"/sandbox_config.json)