Re: [PATCH bpf-next 10/13] selftests/bpf: Add tracing_multi bpf prog cookie test

On 10/8/26 00:20, [email protected] wrote:
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
>> index 5cf11707eaf89..070266a864d4f 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
>> @@ -304,9 +304,12 @@ static int run_bpf_target(struct bpf_program *prog, __u32 retval, const char *na
>>  	return 0;
>>  }
>>
>> -static void test_link_api_bpf_prog(void)
>> +static void __test_link_api_bpf_prog(bool test_cookies)
>>  {
>>  	const char *funcs[] = { "target_1", "target_2" };
>> +	__u64 fentry_cookies[] = { 0xfeed01, 0xfeed01 };
>> +	__u64 fexit_cookies[] = { 0xfeed02, 0xfeed02 };
>> +	__u64 fsession_cookies[] = { 0xfeed03, 0xfeed03 };
> 
> A subsystem pattern flags this as potentially concerning: every per-target
> cookie array holds the same value twice. The test on the BPF side compares
> against a single scalar global set from element [0]:
> 
>   skel->bss->fentry_cookie = fentry_cookies[0];
> 
> and in progs/tracing_multi_bpf.c:
> 
>   if (!test_cookies || bpf_get_attach_cookie(ctx) == fentry_cookie)
>           test_result_fentry++;
> 
> Since cookie[0] == cookie[1] for all three arrays, the test cannot
> distinguish a correct cookie-to-target mapping from a permuted one. The
> assertions (test_result_fentry == 2, etc.) succeed as long as some
> correct-looking cookie value is delivered on both invocations, but swapping
> the cookies of target_1 and target_2 in the kernel is invisible.
> 
> That matters because the kernel path this commit exercises is exactly a
> permutation: bpf_tracing_multi_attach() calls check_dup_keys() which calls
> keys_swap_r() in kernel/trace/bpf_trace.c to reorder the cookies, ids, and
> progs arrays to follow the sorted trampoline keys:
> 
>   id_a    = ids + (key_a - keys);
>   if (data->progs) { ... swap(*prog_a, *prog_b); }
>   if (cookies) { cookie_a = cookies + (id_a - ids); ... swap(*cookie_a, *cookie_b); }
> 
> An indexing mistake in that swap could desynchronise cookies from progs/ids,
> yet the test would still pass. Since fds[]/ids[] come from BPF prog FDs, the
> sort order is not the array order the caller supplied.
> 
> The neighbouring test for kernel-function targets in this same file already
> does this correctly: bpf_fentry_test_cookies[] uses ten distinct values
> deliberately out of order (8, 9, 7, 5, 4, 2, 3, 1, 10, 6) and
> progs/tracing_multi_check.c checks the cookie per-IP with something like:
> 
>   err |= test_cookies ? cookie != 8 : 0;
> 
> under `ip == &bpf_fentry_test1`.
> 
> Can the test use distinct cookies and a bpf_get_func_ip()-based per-target
> expectation so it is sensitive to cookie mis-association?
> 

Will update the cookies with different values.

>>  	LIBBPF_OPTS(bpf_tracing_multi_opts, opts);
>>  	struct tracing_multi_bpf *skel = NULL;
>>  	int fds[ARRAY_SIZE(funcs)];
> 
> [ ... ]
> 
>> @@ -356,6 +367,16 @@ static void test_link_api_bpf_prog(void)
>>  	tracing_multi_bpf__destroy(skel);
>>  }
>>
>> +static void test_link_api_bpf_prog(void)
>> +{
>> +	__test_link_api_bpf_prog(false);
>> +}
>> +
>> +static void test_cookies_bpf_prog(void)
>> +{
>> +	__test_link_api_bpf_prog(true);
>> +}
> 
> This isn't a bug, but would calling __test_link_api_bpf_prog(false) and
> __test_link_api_bpf_prog(true) straight from the two subtest arms read more
> consistently? The neighbouring test_link_api_ids() is dispatched directly as
> test_link_api_ids(false) and test_link_api_ids(true) just a dozen lines
> down, without wrapper functions.
> 

Okay, will drop these two wrappers.

Thanks,
Leon

> [ ... ]
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31320427087