Re: [PATCH bpf-next v11 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] bpf: TOCTOU race condition in temporary file handling
--

commit 06564da4fc281d602c319554603ab58e4dd5a9b8
Author: Hui Zhu <zhuhui@kylinos.cn>

selftests/bpf: Add memcg async reclaim test

This commit adds a test for BPF-driven async proactive reclaim where a BPF 
program watches refault stats of a memory-pressured cgroup. It also adds a 
second test to verify that a dying target cgroup is properly skipped during 
reclaim.

> 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..7c7d299b3b28a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
[ ... ]
> +static int write_file(const char *filename)
> +{
> +	int ret = -1;
> +	size_t written = 0;
> +	char *buffer;
> +	FILE *fp;
> +
> +	fp = fopen(filename, "wb");

[Severity: Medium]
Could opening the file by name with "wb" follow a symlink? If an attacker
replaces the file in the window after mkstemp() closes its file descriptor,
and this test runs with root privileges in an accessible directory, it looks
like it could lead to an arbitrary file overwrite.

> +	if (!fp)
> +		goto out;
> +
> +	buffer = malloc(BUFFER_SIZE);
[ ... ]
> +static int
> +run_high_low_workload(double *high_elapsed, double *low_elapsed, int read_times)
> +{
> +	char high_data_file[PATH_MAX];
> +	char low_data_file[PATH_MAX];
> +	char high_time_file[PATH_MAX];
> +	char low_time_file[PATH_MAX];
> +	const char *dir = workload_files_dir();
> +	pid_t high_pid = -1, low_pid = -1;
> +	pid_t wait_ret;
> +	int fd, status;
> +	int ret = -1;
> +
> +	snprintf(high_data_file, sizeof(high_data_file),
> +		 "%s/memcg_async_high_data_XXXXXX", dir);
> +	snprintf(low_data_file, sizeof(low_data_file),
> +		 "%s/memcg_async_low_data_XXXXXX", dir);
> +	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]
Does immediately closing the file descriptor from mkstemp() leave the filename
vulnerable to a symlink replacement attack? It seems the file is created
securely, but the binding is lost.

> +
> +	fd = mkstemp(low_data_file);
> +	if (!ASSERT_GE(fd, 0, "mkstemp"))
> +		goto cleanup;
> +	close(fd);
[ ... ]
> +void test_memcg_async_reclaim_dying(void)
> +{
> +	u64 trigger_cgroup_id, target_cgroup_id;
> +	u64 calls_before, bytes_before;
> +	char data_file[PATH_MAX] = "";
> +	struct reclaim_events_seen seen = {};
> +	struct memcg_async_reclaim *skel = NULL;
> +	struct ring_buffer *rb = NULL;
> +	pid_t reader_pid = -1;
> +	int err, fd, i;
> +
> +	err = setup_dying_cgroups(&trigger_cgroup_id, &target_cgroup_id);
> +	if (!ASSERT_OK(err, "setup_dying_cgroups"))
> +		return;
> +
> +	err = setup_bpf(trigger_cgroup_id, target_cgroup_id, &skel);
> +	if (!ASSERT_OK(err, "setup_bpf"))
> +		goto out;
> +
> +	rb = ring_buffer__new(bpf_map__fd(skel->maps.reclaim_events),
> +			      on_reclaim_event, &seen, NULL);
> +	if (!ASSERT_OK_PTR(rb, "ring_buffer__new"))
> +		goto out;
> +
> +	snprintf(data_file, sizeof(data_file),
> +		 "%s/memcg_async_dying_XXXXXX", workload_files_dir());
> +	fd = mkstemp(data_file);
> +	if (!ASSERT_GE(fd, 0, "mkstemp"))
> +		goto out;
> +	close(fd);

[Severity: Medium]
Like the instance above, does closing this file descriptor create a TOCTOU
race window before the keepalive reader opens and writes to it?

> +
> +	reader_pid = spawn_keepalive_reader(data_file);
> +	if (!ASSERT_GT(reader_pid, 0, "fork keepalive reader"))
> +		goto out;

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