Re: [PATCH 2/2] selftests/bpf: add tests to verify the enforcement of CONFIG_CGROUP_LSM_NUM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Resource leak (Memory & File Descriptors) in test loop.
- [Medium] Missing verification of the actual rejection error code.
- [Medium] Test flakiness due to concurrent execution consuming global limits.
--

commit 643ce21be8f485f762f000339f8443301983ef08
Author: Paul Houssel <paulhoussel2@gmail.com>

selftests/bpf: add tests to verify the enforcement of CONFIG_CGROUP_LSM_NUM
    
Add a selftest that verifies the kernel correctly enforces
CONFIG_CGROUP_LSM_NUM as the maximum number of concurrently attachable
per-cgroup LSM hook slots.

> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_lsm_num.c b/tools/testing/selftests/bpf/prog_tests/cgroup_lsm_num.c
> new file mode 100644
> index 0000000000000..1c5825c6c3d07
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_lsm_num.c
[ ... ]
> +void test_cgroup_lsm_num(void)

Should this test be forced to run sequentially by naming it
serial_test_cgroup_lsm_num()?

Since the kernel limits the maximum number of distinct, concurrently
attachable per-cgroup LSM hooks globally based on CONFIG_CGROUP_LSM_NUM,
if this test runs in parallel with other tests that attach to lsm_cgroup
hooks, could they exhaust the global limit and cause this test to fail
unpredictably?

> +{
> +	struct cgroup_lsm_num *skel = NULL;
> +	struct bpf_program *prog;
> +	int cgroup_fd = -1;
> +	int attached = 0;
> +	int failed = 0;
[ ... ]
> +	bpf_object__for_each_program(prog, skel->obj) {
> +		struct bpf_link *link;
> +
> +		link = bpf_program__attach_cgroup(prog, cgroup_fd);
> +		if (!link) {
> +			if (errno == EOPNOTSUPP) {
> +				test__skip();
> +				goto out;
> +			}
> +			failed++;

When we expect attachment to fail because the slot limit is exhausted,
shouldn't we also check that the error code is exactly what we expect
(such as -E2BIG)?

If the attachment fails for an unrelated reason, the failure counter still
increments, which could obscure the true root cause.

> +		} else {
> +			attached++;
> +		}

Does this code leak the link memory and its file descriptor?

Since the attachment is done manually via bpf_program__attach_cgroup()
instead of using the skeleton's auto-attach method,
cgroup_lsm_num__destroy(skel) will not automatically clean up these links.

Should we store the returned link pointer and explicitly call
bpf_link__destroy() on it to prevent exhausting the file descriptor limit
during a test run?

> +	}
> +
> +	// CONFIG_CGROUP_LSM_NUM set to 10
> +	// -> 10 programs shall be attached
> +	ASSERT_EQ(attached, 10, "at least one attached");
> +	// -> 2 programs shall be rejected
> +	ASSERT_EQ(failed, 2, "limit was enforced");
> +
> +out:
> +	close(cgroup_fd);
> +	cgroup_lsm_num__destroy(skel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260506065048.592474-1-paulhoussel2@gmail.com?part=2