Repository: opencontainers/runc
Pull request #3611 (2022-09-26 07:34:32)

cgroups: cpuset: fix byte order while parsing cpuset range to bits
Runc parses cpuset range to bits in the case of cgroup v2 + systemd as cgroup driver. The byte order representation differs from systemd expectation, which will set different cpuset range in systemd transient unit if the length of parsed byte array exceeds one.

	# cat config.json
	...
	"resources": {
		...
		"cpu": {
			"cpus": "10-23"
		}
	},
	...
	# runc --systemd-cgroup run test
	# cat /run/systemd/transient/runc-test.scope.d/50-AllowedCPUs.conf
	# This is a drop-in unit file extension, created via "systemctl set-property"
	# or an equivalent operation. Do not edit.
	[Scope]
	AllowedCPUs=0-7 10-15

The cpuset.cpus in cgroup will also be set to wrong value after reloading systemd manager configuration.

	# systemctl daemon-reload
	# cat /sys/fs/cgroup/system.slice/runc-test.scope/cpuset.cpus
	0-7,10-15

This patch simply reverses the byte order at the end of  RangeToBits().
Please help to verify the patch and any comments and suggestions are welcome!

kolyshkin: Thank you for finding the bug and the fix, @yukariatlas!

Three questions:
1. Do you think the fix will work on both little endian and big endian machines?
2. Is there a more standard method to reverse the byte order?
3. Can we have a test case (merely codifying the repro that you posted in description)?
yukariatlas: Thanks for your time, @kolyshkin !

1. Do you think the fix will work on both little endian and big endian machines?
Systemd made transfer of cpuset over dbus endian safe in the following commit:
https://github.com/systemd/systemd/commit/c367f996f5f091a63f812f0140b304c649be77fc

Dbus will handle endian issue in transmission and Systemd has assumed the parsing order of byte array in cpu_set_from_dbus().
If we encode the byte array as the same order in cpu_set_from_dbus(), it will work fine in both little endian and big endian machines.

2. Is there a more standard method to reverse the byte order?
I have checked it before creating a PR but cannot find a built-in function for reversing a slice in the standard library.
sort.Reverse will return a interface to sort in reversed order, which doesn't fit our requirement. (e.g. [5, 2, 6, 3, 1, 4] -> [6 5 4 3 2 1])

3. Can we have a test case (merely codifying the repro that you posted in description)?
Sure, I think that test can avoid encountering the same issue again in the future.
It will be similar to "update cpuset parameters via v2 unified map" test in tests/integration/update.bats.
I consider that we can add a new similar test to emphasize on the cpuset byte order issue.
yukariatlas: Hi, @kolyshkin !

I added a test case for this issue.
Please help to verify the patch and any comments and suggestions are welcome!
jwhonce: Verified on 12 core, Fedora 37 Intel machine on main
kolyshkin: Some unexpected and weird failure in CentOS 9 CI:

```
not ok 14 runc run (cgroup v2 resources.unified override)
# (in test file tests/integration/cgroups.bats, line 251)
#   `[ "$status" -eq 0 ]' failed
# runc spec (status=0):
# 
# runc run -d --console-socket /tmp/bats-run-95811/runc.JNT0V5/tty/sock test_cgroups_unified (status=1):
# time="2022-10-03T03:14:39Z" level=error msg="runc run failed: unable to start container process: container init was OOM-killed (memory limit too low?)"
```

Patch:
diff --git a/libcontainer/cgroups/systemd/cpuset.go b/libcontainer/cgroups/systemd/cpuset.go
index 83d10dd7..dd474cf1 100644
--- a/libcontainer/cgroups/systemd/cpuset.go
+++ b/libcontainer/cgroups/systemd/cpuset.go
@@ -51,5 +51,10 @@ func RangeToBits(str string) ([]byte, error) {
 		// do not allow empty values
 		return nil, errors.New("empty value")
 	}
+
+	// fit cpuset parsing order in systemd
+	for l, r := 0, len(ret)-1; l < r; l, r = l+1, r-1 {
+		ret[l], ret[r] = ret[r], ret[l]
+	}
 	return ret, nil
 }


Test patch:
diff --git a/libcontainer/cgroups/systemd/cpuset_test.go b/libcontainer/cgroups/systemd/cpuset_test.go
index 3030cba9..bda31a5b 100644
--- a/libcontainer/cgroups/systemd/cpuset_test.go
+++ b/libcontainer/cgroups/systemd/cpuset_test.go
@@ -22,13 +22,13 @@ func TestRangeToBits(t *testing.T) {
 		{in: "4-7", out: []byte{0xf0}},
 		{in: "0-7", out: []byte{0xff}},
 		{in: "0-15", out: []byte{0xff, 0xff}},
-		{in: "16", out: []byte{1, 0, 0}},
-		{in: "0-3,32-33", out: []byte{3, 0, 0, 0, 0x0f}},
+		{in: "16", out: []byte{0, 0, 1}},
+		{in: "0-3,32-33", out: []byte{0x0f, 0, 0, 0, 3}},
 		// extra spaces and tabs are ok
 		{in: "1, 2, 1-2", out: []byte{6}},
 		{in: "    , 1   , 3  ,  5-7,	", out: []byte{0xea}},
 		// somewhat large values
-		{in: "128-130,1", out: []byte{7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2}},
+		{in: "128-130,1", out: []byte{2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7}},
 
 		{in: "-", isErr: true},
 		{in: "1-", isErr: true},
diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash
index ee93c42b..a9b70ffd 100644
--- a/tests/integration/helpers.bash
+++ b/tests/integration/helpers.bash
@@ -457,6 +457,13 @@ function requires() {
 				skip_me=1
 			fi
 			;;
+		more_than_8_core)
+			local cpus
+			cpus=$(grep -c '^processor' /proc/cpuinfo)
+			if [ "$cpus" -le 8 ]; then
+				skip_me=1
+			fi
+			;;
 		*)
 			fail "BUG: Invalid requires $var."
 			;;
diff --git a/tests/integration/update.bats b/tests/integration/update.bats
index 1937006f..d2489009 100644
--- a/tests/integration/update.bats
+++ b/tests/integration/update.bats
@@ -557,6 +557,33 @@ EOF
 	check_systemd_value "AllowedMemoryNodes" 1
 }
 
+@test "update cpuset cpus range via v2 unified map" {
+	# This test assumes systemd >= v244
+	[ $EUID -ne 0 ] && requires rootless_cgroup
+	requires systemd cgroups_v2 more_than_8_core cgroups_cpuset
+
+	update_config ' .linux.resources.unified |= {
+				"cpuset.cpus": "0-5",
+			}'
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_update
+	[ "$status" -eq 0 ]
+
+	# check that the initial value was properly set
+	check_systemd_value "AllowedCPUs" "0-5"
+
+	runc update -r - test_update <<EOF
+{
+  "unified": {
+    "cpuset.cpus": "5-8"
+  }
+}
+EOF
+	[ "$status" -eq 0 ]
+
+	# check the updated systemd unit property, the value should not be affected by byte order
+	check_systemd_value "AllowedCPUs" "5-8"
+}
+
 @test "update rt period and runtime" {
 	[ $EUID -ne 0 ] && requires rootless_cgroup
 	requires cgroups_v1 cgroups_rt no_systemd