Re: [RFC PATCH bpf-next v7 08/11] selftests/bpf: Add tests for memcg_bpf_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The inline assembly in `read_file` fails to actually trigger memory access and page faults.
- [Medium] Error paths in `real_test_memcg_ops` leave child processes orphaned, preventing cgroup cleanup.
- [Medium] The commit message references a `get_high_delay_ms` hook, but the implemented hook is `memcg_charged`.
- [Low] Missing file descriptor validation after manual map lookup using `bpf_map__fd()`.
- [Low] TOCTOU race condition from closing temporary file descriptors generated by `mkstemp()`.
--

commit 239c9254e915ce7f13738ad4bc04054bec8bf86e
Author: Hui Zhu <zhuhui@kylinos.cn>

selftests/bpf: Add tests for memcg_bpf_ops

Add a comprehensive selftest suite for the memcg_bpf_ops functionality. These
tests validate that BPF programs can correctly influence memory cgroup
throttling behavior by implementing the new hooks.

>     The test suite is added in `prog_tests/memcg_ops.c` and covers
>     several key scenarios:
>     
>     1. `test_memcg_ops_over_high`:
>        Verifies that a BPF program can trigger throttling on a low-priority
>        cgroup by returning a delay from the `get_high_delay_ms` hook when a
>        high-priority cgroup is under pressure.
>     
>     2. `test_memcg_ops_below_low_over_high`:
>        Tests the combination of the `below_low` and `get_high_delay_ms`
>        hooks, ensuring they work together as expected.
>     
>     3. `test_memcg_ops_below_min_over_high`:
>        Validates the interaction between the `below_min` and
>        `get_high_delay_ms` hooks.

[Severity: Medium]
The commit message mentions the get_high_delay_ms hook multiple times, but the
code implements memcg_charged_impl which attaches to struct_ops/memcg_charged.
Could the commit message be updated to reflect the actual hook name used?

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_ops.c b/tools/testing/selftests/bpf/prog_tests/memcg_ops.c
> new file mode 100644
> index 0000000000000..19fd4fde22660
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_ops.c
[ ... ]
> +int read_file(const char *filename, int iterations)
> +{
[ ... ]
> +	for (int iter = 0; iter < iterations; iter++) {
> +		for (i = 0; i < FILE_SIZE; i += page_size) {
> +			/* access a byte to trigger page fault */
> +			p = &map[i];
> +			__asm__ __volatile__("" : : "r"(p) : "memory");
> +		}
> +
> +		if (env.verbosity >= VERBOSE_NORMAL)
> +			printf("%s %d %d done\n", __func__, getpid(), iter);
> +	}

[Severity: High]
Does this inline assembly actually trigger a memory access?

The "r"(p) constraint just loads the address into a general-purpose register,
but doesn't dereference it. The memory clobber also does not force a read of
the pointer's target memory. 

Should this be dereferenced, such as "r"(*p) or explicitly via 
(void)*(volatile char *)p, to ensure a page fault is triggered?

[ ... ]
> +static void real_test_memcg_ops(int read_times)
> +{
> +	int ret;
> +	char data_file1[] = "/tmp/test_data_1_XXXXXX";
> +	char data_file2[] = "/tmp/test_data_2_XXXXXX";
> +	char time_file1[] = "/tmp/test_time_1_XXXXXX";
> +	char time_file2[] = "/tmp/test_time_2_XXXXXX";
> +	pid_t pid1, pid2;
> +	double time1, time2;
> +	int status;
> +
> +	ret = mkstemp(data_file1);
> +	if (!ASSERT_GE(ret, 0, "mkstemp"))
> +		return;
> +	close(ret);

[Severity: Low]
By closing the file descriptor returned by mkstemp() immediately and then
having the child process reopen it later with fopen(), does this introduce a
time of check to time of use race condition?

Would it be safer to pass the file descriptor to the child process or use a
different mechanism for temporary file handling?

[ ... ]
> +	ret = waitpid(pid1, &status, 0);
> +	if (!ASSERT_GT(ret, 0, "child1 waitpid"))
> +		goto cleanup;
> +	if (!ASSERT_TRUE(WIFEXITED(status), "child1 exited normally"))
> +		goto cleanup;
> +	if (!ASSERT_EQ(WEXITSTATUS(status), 0, "child1 exit status"))
> +		goto cleanup;
> +
> +	ret = waitpid(pid2, &status, 0);

[Severity: Medium]
If the waitpid for pid1 fails, or if pid1 exits abnormally or returns a
non-zero status, the code jumps to cleanup without waiting for or terminating
pid2.

Will this leave pid2 running in the background and attached to the
high-priority cgroup, preventing cgroup cleanup and affecting subsequent
tests?

[ ... ]
> +void test_memcg_ops_over_high(void)
> +{
[ ... ]
> +	map = bpf_object__find_map_by_name(skel->obj, ".bss");
> +	if (!ASSERT_OK_PTR(map, "bpf_object__find_map_by_name .bss"))
> +		goto out;
> +
> +	map_fd = bpf_map__fd(map);
> +	bss_data = calloc(1, bpf_map__value_size(map));

[Severity: Low]
Should map_fd be validated before being used?

The BPF subsystem selftest guidelines recommend checking file descriptors
returned after manual lookups, for example with ASSERT_GE(map_fd, 0, ...).

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