[harche]

parse_cpu_max() read some cpu.max values differently from the kernel. With the systemd driver, this could give the scope and the container different CPU limits, or make the container fail to start with an sd-bus error. Details are in #2405.

This makes the parser follow the kernel's rules, and stay stricter where that makes sense:

• Tabs and other whitespace may surround and separate the quota and the period, and empty lines may come before and after the value. A newline between the quota and the period is still rejected, because write_unified_resources() writes each line separately, so the kernel would see "50000\n100000" as two writes.
• Both values must be numbers without a sign (or max for the quota), and only whitespace may follow them.
• The period must be in [1000, 1000000] and the quota in [1000, 2^44−1], as the kernel requires.
• A quota longer than 20 characters is rejected, since the kernel cuts it at 20 and uses the rest as the period.
• On crun update, when cpu.max has no period, the current period is read from the container's cpu.max, like the kernel does, instead of using the default to compute CPUQuotaPerSecUSec.

Well-formed values are read as before, including what the kernel writes in cpu.max (max 100000\n), which complete_cpu_max() reads back.

Testing

• Unit tests in tests/tests_libcrun_cgroup.c: test_parse_cpu_max for valid values (tabs, leading and trailing newlines, range limits), test_parse_cpu_max_invalid for the malformed and out-of-range ones.
• Every test value was also written to a real cpu.max (Fedora 44, kernel 6.19): whatever parse_cpu_max() accepts, the kernel accepts with the same values, and every out-of-range value is rejected by both.
• With the systemd driver, unified["cpu.max"] set to "50000\n100000" or "max\n100000" is rejected with invalid value for cpu.max (with the cgroupfs driver, the container ends up with quota 100000). "\nmax 100000" and "50000 100000\n\n" give the scope and the container the same limits.
• Update with the systemd driver: created a container with cpu.period 200000, then ran crun update with unified["cpu.max"] = "50000". Before, the container got 50000 200000 but systemd recorded CPUQuotaPerSecUSec=500ms. Now systemd records 250ms with a 200ms period, which matches.
• tests/test_update.py passes with both CGROUP_MANAGER=cgroupfs and systemd. tests/test_cgroup_resources.py passes except cgroup-resources-hugetlb-limits, which fails the same way on main (no hugetlb controller in the VM).
• make clang-format is clean.

[harche]

@kolyshkin, this fixes parsing in parse_cpu_max(), which was added in #2296. Could you take a look when you have a moment?

[kolyshkin]

So I think we should use the same validation rules as the kernel (i.e. writing to cpu.max).

The kernel reads sscanf(buf, "%20s %llu"):

• The first word is cut at 20 characters and becomes the quota token.
• The token must start with a digit; anything after the digits is ignored.
  A token that is not a number must be exactly max.
• The period is optional. If it is missing or invalid, the cgroup's
  current period is kept.
• Trailing garbage is ignored.
• Numbers wrap around on overflow without an error.

After parsing, the values must satisfy:

• 1000 ≤ quota ≤ 2^44−1, or max;
• 1000 ≤ period ≤ 1000000.

1. The kernel accepts but parse_cpu_max() rejects

These work with the cgroupfs driver and fail with the systemd driver.

cpu.max	Kernel result	Why crun rejects it
"\tmax", "\nmax 100000"	max / current period, max / 100000	only ' ' is skipped before max (a number may start with any whitespace, because strtoll() skips it)
"50000\n100000", "max\n100000"	50000 / 100000, max / 100000	a newline is not accepted as the separator
"50000 100000 1", "50000 100000x", "50000 100000\n1"	50000 / 100000	trailing garbage
"50000 -1", "50000 +100000", "50000 abc"	50000 / current period (the invalid period is ignored)	invalid period
"50000+200000", "50000-1", "50000x 100000"	50000 / current period; 50000 / 100000 for the last one	garbage attached to the quota
"18446744073709552616" (2^64 + 1000)	quota 1000 (wraps around)	ERANGE

2. parse_cpu_max() accepts but the kernel rejects

Both drivers fail. With systemd, the scope has already been set up when the
write to the container's cpu.max returns EINVAL, so the error shows up
later and is less clear.

cpu.max	crun result	Why the kernel rejects it
"+50000 100000"	50000 / 100000	%llu does not accept a + sign, and "+50000" is not "max"
"50000 0", "max 0"	period 0. With systemd this silently means "no period", and the default is used	period < 1000
"50000 999", "50000 1000001", "50000 18446744073709551615"	period passed to systemd as is (UINT64_MAX is USEC_INFINITY)	period out of [1000, 1000000]
"999 100000", "17592186044416 100000"	quota passed to systemd as is	quota out of [1000, 2^44−1]

3. Both accept, but with different values

cpu.max	Kernel	crun
"000000000000000500001000" (a 24-character word)	quota token "00000000000000050000" is cut at 20 characters, and the remaining "1000" becomes the period: 50000 / 1000	500001000 / default period
"50000" (no period)	keeps the cgroup's current period	linux.resources.cpu.period if set, otherwise the systemd default (100000)

The last row only matters if the current period differs from what crun
sends to systemd, so in practice it probably does not.

Notes

• Group 1 is the only one where the systemd driver fails and the cgroupfs
  driver works. Most of it is rejected on purpose by this PR, and runc
  rejects the same values. Only "\tmax" looks like an accidental
  inconsistency.
• Group 2 is easiest to close with range checks in parse_cpu_max()
  itself.

[kolyshkin]

To clarify: I think our rules must be at least as strict as the kernel (say the kernel does not accept + and we currently do) and might be stricter (say we can error out on garbage at the end even if the kernel does not), but I would not error out on say \n (unless there's a good reason to do that, which I don't see now).

[harche]

Thanks for going through this so carefully. The tables made it easy to see what was off.

I agree with the rule: be at least as strict as the kernel, stricter is fine, but don't reject things like \n without a good reason. Here's what I changed:

Group 1. Tabs and other whitespace now work around and between the two values, and so do empty lines before or after the value, so "\tmax" and "\nmax 100000" are accepted. Trailing garbage, bad periods and garbage stuck to the quota are still rejected, as you suggested.

I kept rejecting a newline between the two values ("50000\n100000", "max\n100000"), though. The kernel would read them as quota and period if it got the whole string in one write, but crun doesn't write a unified value in one go: write_unified_resources() splits it on newlines and writes each line separately. So for "50000\n100000" the kernel first gets "50000" and then "100000". I tried it with the cgroupfs driver, starting from period 200000, and the container ended up with 100000 200000. Accepting it in parse_cpu_max() would have told systemd 50000/100000, which is exactly the kind of mismatch this PR is trying to fix.

Group 2. I added the same checks the kernel does: no sign on the quota, a period between 1000 and 1000000, and a quota between 1000 and 2^44−1. These values now fail early with invalid value for cpu.max, before anything is sent to systemd.

Group 3. The 24-character case is now rejected: a quota longer than 20 characters is an error.

I took a closer look at "50000" with no period. On create it's fine. crun first writes the period from the spec, or the new cgroup starts with 100000, which is what crun tells systemd anyway. On crun update it can go wrong, though. I tried it: I created a container with period 200000, then updated it with cpu.max set to "50000". The kernel kept period 200000, so the container got 25% of a CPU. But crun used 100000 to work out the value for systemd, so systemd recorded 50% (CPUQuotaPerSecUSec=500ms). Now, on update, when cpu.max has no period, crun reads the current period from the container's cpu.max, like the kernel does. After the change systemd records 250ms, which matches.

I also wrote each test value to a real cpu.max on kernel 6.19, to check that whatever crun accepts, the kernel accepts with the same numbers.

[harche]

@kolyshkin while working on this, I looked for the same kind of problem in other parts of crun and found a few. I'm sharing them here so you know about them. I'll send fixes as separate PRs once this one is merged.

1. Other unified values with the systemd driver

get_value_from_unified_map() turns memory.max, memory.high, memory.low, memory.min, memory.swap.max, memory.zswap.max, pids.max, cpu.weight and cpu.idle into systemd properties, with the same kind of mismatch as cpu.max. I tested these on kernel 6.19 with systemd 259:

• A leading zero means octal to the kernel but decimal to crun. With pids.max = "0100", systemd gets TasksMax=100 but the container gets 64. The same happens with cpu.weight and the memory files. The container starts fine, with different limits in the scope and the container.
• An empty value becomes 0, while the cgroupfs driver skips it. With memory.swap.max = "", systemd gets MemorySwapMax=0, so the scope can't swap, but the container's cgroup still shows max.
• Values the kernel rejects are still sent to systemd: "-1" (which becomes infinity), "+N", pids.max above 4194304, cpu.weight = "0" (which systemd reads as "make it idle"), and cpu.idle = "2". The error only comes later, at the raw write.

2. The systemd driver rejects values that cgroupfs accepts

This is allowed under the "stricter is fine" rule, but the two drivers behave differently:

• memory.max = "512M" works with cgroupfs and fails with systemd.
• cpuset_string_to_bitmask() rejects "0-3:1/2", "1-N" and "0, 2" for cpuset.cpus, which the kernel accepts.

I'm not sure these are worth supporting. What do you think?

3. The "memory limit could be too low" hint

is_memory_limit_too_low() reads unified memory.max with strtol and doesn't check where the number ends. So "max" reads as 0, and a typed limit: -1 also counts as too low. Any start failure then gets the "the memory limit could be too low" prefix, even when memory is unlimited.

[kolyshkin]

@harche this looks much better now, thanks! Can you split the reading of the missing period during update into a separate commit?

Also, read_cpu_max_period and complete_cpu_max look similar -- maybe split the common part into e.g. read_cpu_max (path, &quota, &period, &has_period, err)?

Other than that, LGTM

[kolyshkin]

@harche can you use your real email in signed-off-by please?

[harche]

@kolyshkin that's the address I use to sign off everywhere, including my Kubernetes commits. It's my GitHub noreply address, and I use it to keep my email away from spammers, since commit history is public and gets scraped. It's tied to my GitHub account, the name is my real name, and the DCO check on this PR passes.