Re: [RFC PATCH bpf-next v3 09/12] selftests/bpf: Add tests for memcg_bpf_ops

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

> 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 000000000000..9a8d16296f2d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_ops.c

[ ... ]

> +static void real_test_memcg_ops(int read_times)
> +{
> +	int ret;
> +	char data_file1[] = "/tmp/test_data_XXXXXX";
> +	char data_file2[] = "/tmp/test_data_XXXXXX";
> +	char time_file1[] = "/tmp/test_time_XXXXXX";
> +	char time_file2[] = "/tmp/test_time_XXXXXX";
> +	pid_t pid1, pid2;
> +	double time1, time2;

[ ... ]

> +	pid1 = fork();
> +	if (!ASSERT_GE(pid1, 0, "fork"))
> +		goto cleanup;
> +	if (pid1 == 0)
> +		real_test_memcg_ops_child_work(CG_LOW_DIR,
> +					       data_file1,
> +					       time_file1,
> +					       read_times);
> +
> +	pid2 = fork();
> +	if (!ASSERT_GE(pid1, 0, "fork"))
                       ^^^^

Should this check pid2 instead of pid1?  After assigning pid2 = fork(),
checking pid1 means a failed second fork() would go undetected if the
first fork() succeeded.

> +		goto cleanup;
> +	if (pid2 == 0)
> +		real_test_memcg_ops_child_work(CG_HIGH_DIR,
> +					       data_file2,
> +					       time_file2,
> +					       read_times);

[ ... ]

> +void test_memcg_ops_over_high(void)
> +{
> +	int err, map_fd;
> +	struct memcg_ops *skel;
              ^^^^^^^^^^^^^^^

The skel variable is not initialized to NULL here.  If setup_cgroup()
fails early and jumps to the out label, memcg_ops__detach(skel) would
be called with an uninitialized pointer.

Also, the cleanup path calls memcg_ops__detach() but never calls
memcg_ops__destroy().  Would this leak the skeleton object and its
associated BPF resources?

> +	struct bpf_map *map;
> +	size_t bss_sz;
> +	struct memcg_ops__bss *bss_data;
> +	__u32 key = 0;
> +	struct bpf_program *prog = NULL;
> +	struct bpf_link *link = NULL, *link2 = NULL;
> +	DECLARE_LIBBPF_OPTS(bpf_struct_ops_opts, opts);
> +	int high_cgroup_id, low_cgroup_fd = -1;
> +
> +	err = setup_cgroup(&high_cgroup_id, &low_cgroup_fd, NULL);
> +	if (!ASSERT_OK(err, "setup_cgroup"))
> +		goto out;
> +
> +	skel = memcg_ops__open_and_load();

[ ... ]

> +out:
> +	bpf_link__destroy(link);
> +	bpf_link__destroy(link2);
> +	memcg_ops__detach(skel);
> +	close(low_cgroup_fd);
> +	cleanup_cgroup_environment();
> +}

[ ... ]

> +void test_memcg_ops_below_low_over_high(void)
> +{
> +	int err, map_fd;
> +	struct memcg_ops *skel;

Same issue here - skel is not initialized to NULL.

[ ... ]

> +out:
> +	bpf_link__destroy(link);
> +	bpf_link__destroy(link_high);
> +	bpf_link__destroy(link_low);
> +	memcg_ops__detach(skel);

[ ... ]

> +void test_memcg_ops_below_min_over_high(void)
> +{
> +	int err, map_fd;
> +	struct memcg_ops *skel;

Same issue here as well - skel is not initialized to NULL.

[ ... ]

> +out:
> +	bpf_link__destroy(link);
> +	bpf_link__destroy(link_high);
> +	bpf_link__destroy(link_low);
> +	memcg_ops__detach(skel);
> +	close(high_cgroup_fd);
> +	close(low_cgroup_fd);
> +	cleanup_cgroup_environment();
> +}


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/21280790825