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 <[email protected]> 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/[email protected]?part=18
