Re: [PATCH bpf-next v6 2/2] selftests/bpf: Add memcg async reclaim test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Insecure temporary file creation using mkstemp() followed by close() and reopening by name.
--

commit 6ab0cdc46bdb6e587b82be1eda75b832722b30af
Author: Hui Zhu <zhuhui@kylinos.cn>
selftests/bpf: Add memcg async reclaim test

This commit adds the memcg_async_reclaim selftest. It verifies that
BPF-driven async proactive reclaim mitigates refault-induced slowdown
under memory pressure using the bpf_wq and bpf_proactive_reclaim() APIs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> new file mode 100644
> index 0000000000000..1270d73c9116e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
[ ... ]
> @@ -0,0 +1,480 @@
[ ... ]
> +static int write_file(const char *filename)
> +{
> +	int ret = -1;
> +	size_t written = 0;
> +	char *buffer;
> +	FILE *fp;
> +
> +	fp = fopen(filename, "wb");

[Severity: Medium]
Does this code reopen a securely created temporary file by name? 

The file descriptor from mkstemp() was closed earlier, which can create a
Time-of-Check to Time-of-Use (TOCTOU) race condition. If an attacker replaces
the file with a symlink during this window, it could lead to unintended file
truncation or writes.

[ ... ]
> +static int real_test_child_work(const char *cgroup_path, char *data_filename,
> +				char *time_filename, int read_times)
> +{
[ ... ]
> +	printf("%.6f\n", elapsed);
> +
> +	fp = fopen(time_filename, "w");

[Severity: Medium]
Similarly, could opening this time file by name present a symlink truncation
risk?

[ ... ]
> +static int
> +run_high_low_workload(double *high_elapsed, double *low_elapsed, int read_times)
> +{
[ ... ]
> +	snprintf(high_time_file, sizeof(high_time_file),
> +		 "%s/memcg_async_high_time_XXXXXX", dir);
> +	snprintf(low_time_file, sizeof(low_time_file),
> +		 "%s/memcg_async_low_time_XXXXXX", dir);
> +
> +	fd = mkstemp(high_data_file);
> +	if (!ASSERT_GE(fd, 0, "mkstemp"))
> +		goto cleanup;
> +	close(fd);

[Severity: Medium]
Closing the file descriptor returned by mkstemp() here discards the security
guarantees it provides, opening the race window.

Could the test be refactored to pass the open file descriptor instead of
relying on the filename string? This same pattern appears to be repeated for
all four temporary files created in this function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788228773.git.zhuhui@kylinos.cn?part=2