Re: [RFC PATCH 7/7] selftests: cgroup: check that high_work reclaim is charged to the memcg

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: integer overflow in CPU time calculations on 32-bit systems
--

commit 07220f623c10651bfa63c3489314840c8641decc
Author: Shakeel Butt <shakeel.butt@linux.dev>

selftests: cgroup: check that high_work reclaim is charged to the memcg

Fills a memcg's TCP receive buffers from softirq while its only task
sleeps, so the memcg goes over memory.high and high_work reclaims.
Checks that the memcg's cpu.stat and memory.pressure grow, which
only happens if that reclaim is charged to the memcg.

> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 0ed82347044ed..00802804c0d98 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
[ ... ]
> +static long now_usec(void)
> +{
> +	struct timespec ts;
> +
> +	clock_gettime(CLOCK_MONOTONIC, &ts);
> +	return ts.tv_sec * 1000000L + ts.tv_nsec / 1000;

[Severity: Medium]
Does this calculation overflow a 32-bit signed long? On 32-bit architectures,
multiplying tv_sec by 1000000L truncates to a 32-bit signed long, which
overflows after approximately 35 minutes of system uptime. Could this corrupt
time differences due to signed overflow?

> +}
[ ... ]
> +static int test_memcg_high_work_cpu_max(const char *root)
> +{
> +	int ret = KSFT_FAIL, pid = -1, hog = -1, i, sk[HIGH_WORK_NCONN];
> +	long current, h0, h1, h2, t0, t1, t2, u1, u2, reclaim, lost;
[ ... ]
> +	/* The receiver sleeps, so the rest of the memcg's time is high_work. */
> +	reclaim = (u2 - u1) - (h2 - h1);
> +	lost = (h1 - h0) * (t2 - t1) / (t1 - t0) - (h2 - h1);

[Severity: Medium]
Can this multiplication overflow a 32-bit long? The time differences
(t2 - t1) and (h1 - h0) appear to be microsecond deltas of several seconds.
On 32-bit systems, their product can unconditionally overflow the 32-bit
signed long maximum, resulting in a bogus negative value that might cause
spurious test failures.

> +	if (reclaim < 30000) {
> +		ksft_print_msg("high_work used only %ld us, too little to test\n",
> +			       reclaim);
> +		ret = KSFT_SKIP;
> +		goto cleanup;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924184714.912181-1-shakeel.butt@linux.dev?part=7