Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add a test for bpf_cgroup_from_id lookup in non-root cgns

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2025-08-11 at 12:59 -0700, Kumar Kartikeya Dwivedi wrote:

[...]

> +static void test_cgrp_from_id_ns(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct cgrp_kfunc_success *skel;
> +	struct bpf_program *prog;
> +	int fd, pid, pipe_fd[2];
> +
> +	skel = open_load_cgrp_kfunc_skel();
> +	if (!ASSERT_OK_PTR(skel, "open_load_skel"))
> +		return;
> +
> +	if (!ASSERT_OK(skel->bss->err, "pre_mkdir_err"))
> +		goto cleanup;
> +
> +	prog = bpf_object__find_program_by_name(skel->obj, "test_cgrp_from_id_ns");

Nit: skel->test_cgrp_from_id_ns ?

> +	if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
> +		goto cleanup;
> +
> +	if (!ASSERT_OK(pipe(pipe_fd), "pipe"))
> +		goto cleanup;
> +
> +	pid = fork();
> +	if (!ASSERT_GE(pid, 0, "fork result"))
> +		goto pipe_cleanup;
> +
> +	if (pid == 0) {
> +		int ret = 1;
> +
> +		close(pipe_fd[0]);
> +		fd = create_and_get_cgroup("cgrp_from_id_ns");
> +		if (!ASSERT_GE(fd, 0, "cgrp_fd"))
> +			_exit(1);
> +
> +		if (!ASSERT_OK(join_cgroup("cgrp_from_id_ns"), "join cgrp"))
> +			goto fail;
> +
> +		if (!ASSERT_OK(unshare(CLONE_NEWCGROUP), "unshare cgns"))
> +			goto fail;
> +
> +		ret = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);
> +		if (!ASSERT_OK(ret, "test run ret"))
> +			goto fail;
> +
> +		remove_cgroup("cgrp_from_id_ns");

If this test is executed in -vvv mode, the following is printed:

  (cgroup_helpers.c:412: errno: Device or resource busy) rmdiring cgroup cgrp_from_id_ns ...

And cgroup is still in place after exit.  As far as I understand,
child process needs to change cgroup again or remove_cgroup needs to
be called in the parent process.

> +
> +		if (!ASSERT_OK(opts.retval, "test run retval"))
> +			_exit(1);

Nit: why not 'exit'? '_exit' does not flush file descriptors.

> +		ret = 0;
> +		close(fd);
> +		if (!ASSERT_EQ(write(pipe_fd[1], &ret, sizeof(ret)), sizeof(ret), "write pipe"))
> +			_exit(1);
> +
> +		_exit(0);
> +fail:
> +		remove_cgroup("cgrp_from_id_ns");
> +		_exit(1);
> +	} else {
> +		int res;
> +
> +		close(pipe_fd[1]);
> +		if (!ASSERT_EQ(read(pipe_fd[0], &res, sizeof(res)), sizeof(res), "read res"))
> +			goto pipe_cleanup;
> +		if (!ASSERT_OK(res, "result from run"))
> +			goto pipe_cleanup;
> +	}
> +
> +pipe_cleanup:
> +	close(pipe_fd[1]);

Nit: should this be pipe_fd[0]?
     in case of a fork() failure, should this be both?

> +cleanup:
> +	cgrp_kfunc_success__destroy(skel);
> +}
> +
>  void test_cgrp_kfunc(void)
>  {
>  	int i, err;

[...]