Re: [PATCH bpf-next v2 18/21] selftests/bpf: Add tracing_multi bpf prog rollback test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: selftest tracing_multi rollback logic relies on unreliable FD ordering
--

commit 00cadf9aa091042698069799ffcf7faa2dd5e6a5
Author: Leon Hwang <leon.hwang@linux.dev>

selftests/bpf: Add tracing_multi bpf prog rollback test

This commit adds a rollback test for multi-attach fentry BPF programs. It
sets up two targets, pre-attaches to one, and then verifies that a subsequent
multi-attach to both correctly handles the busy state and rolls back.

> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> --- a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> @@ -1308,6 +1308,67 @@ static void test_attach_api_bpf_prog_fails(void)

[ ... ]

> +	first = target_fds[0] < target_fds[1] ? 0 : 1;
> +	second = 1 - first;
> +	fds[0] = target_fds[first];
> +	fds[1] = target_fds[second];
> +	ids[0] = target_ids[first];
> +	ids[1] = target_ids[second];

[Severity: Medium]
Does this code incorrectly rely on file descriptor order to predict the
kernel's internal sort order?

The kernel sorts the targets based on a computed key derived from the
program ID and BTF ID: ((u64)prog->aux->id << 32) | btf_id. While program
IDs monotonically increase, FD allocation uses the lowest available FD.

If a lower FD becomes available (for example, closed by another thread or
process) between loading the first and second targets, the second target
receives a smaller FD but a larger ID. This reversed order means the test
will incorrectly predict how the kernel sorts the targets.

> +
> +	prog_fd = bpf_program__fd(skel->progs.test_fentry);
> +	lopts.tracing_multi.ids = &ids[1];
> +	lopts.tracing_multi.fds = &fds[1];
> +	lopts.tracing_multi.cnt = 1;
> +	pre_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_FENTRY_MULTI, &lopts);
> +	if (!ASSERT_GE(pre_fd, 0, "pre_attach"))
> +		goto cleanup;
> +
> +	lopts.tracing_multi.ids = ids;
> +	lopts.tracing_multi.fds = fds;
> +	lopts.tracing_multi.cnt = ARRAY_SIZE(ids);
> +
> +	fd = bpf_link_create(prog_fd, 0, BPF_TRACE_FENTRY_MULTI, &lopts);
> +	if (!ASSERT_EQ(fd, -EBUSY, "bpf_link_create"))
> +		goto cleanup;
> +
> +	prog = first == 0 ? skel->progs.target_1 : skel->progs.target_2;
> +	if (run_bpf_target(prog, first + 1, "rollback_target"))
> +		goto cleanup;
> +	ASSERT_EQ(skel->bss->test_result_fentry, 0, "test_result_fentry");

[Severity: Medium]
Can this assertion pass without the rollback path ever being tested?

If the FD order doesn't match the ID order as mentioned above, the test will
pre-attach the element that the kernel sorts first. The kernel will then
fail immediately on the first element of the multi-attach request. Because
it failed on the first element, it never proceeds to attach (or need to roll
back) the other elements.

The test_result_fentry variable remains 0 in this case, meaning this
assertion will pass and silently mask that the rollback path wasn't
actually exercised.

> +
> +cleanup:
> +	if (fd >= 0)
> +		close(fd);
> +	if (pre_fd >= 0)
> +		close(pre_fd);
> +	tracing_multi_bpf__destroy(skel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012154024.29423-1-leon.hwang@linux.dev?part=18