On Tue, 12 Aug 2025 at 00:51, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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 ?
Fixed.
>
> > + 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.
It is probably because the cgroup fd is still open, I will check and fix.
>
> > +
> > + if (!ASSERT_OK(opts.retval, "test run retval"))
> > + _exit(1);
>
> Nit: why not 'exit'? '_exit' does not flush file descriptors.
Fixed.
>
> > + 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?
Yeah, fixed.
>
> > +cleanup:
> > + cgrp_kfunc_success__destroy(skel);
> > +}
> > +
> > void test_cgrp_kfunc(void)
> > {
> > int i, err;
>
> [...]
