Re: [PATCH bpf-next v6 15/17] selftests/bpf: Add tests for BPF function stack arguments

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Medium] The `test_async_cb_many` test uses an extremely short `usleep(50)` to wait for a BPF timer callback to execute, leading to potential flakiness.
- [Low] The block comment for `global_two_callees` incorrectly refers to the function as `bar`.
- [Low] The `bpf_iter_testmod_seq_next` kfunc is incorrectly declared with an `int *` return type in `stack_arg_kfunc.c`.
--

commit 63c1c9ef99d02448833e84a39ecb212ccaaf5b80
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add tests for BPF function stack arguments
    
This commit adds selftests covering stack argument passing for both BPF-to-BPF
subprog calls and kfunc calls with more than 5 arguments.

> diff --git a/tools/testing/selftests/bpf/prog_tests/stack_arg.c b/tools/testing/selftests/bpf/prog_tests/stack_arg.c
> new file mode 100644
> index 0000000000000..d61bac33f8092
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/stack_arg.c

[ ... ]

> +static void test_async_cb_many(void)
> +{
> +	struct stack_arg *skel;
> +
> +	skel = stack_arg__open();
> +	if (!ASSERT_OK_PTR(skel, "open"))
> +		return;
> +
> +	if (!skel->rodata->has_stack_arg) {
> +		test__skip();
> +		goto out;
> +	}
> +
> +	if (!ASSERT_OK(stack_arg__load(skel), "load"))
> +		goto out;
> +
> +	run_subtest(skel->progs.test_async_cb_many_args, 0);
> +
> +	/* Wait for the timer callback to fire and verify the result.
> +	 * 10+20+30+40+50+60+70+80 = 360
> +	 */
> +	usleep(50);
> +	ASSERT_EQ(skel->bss->timer_result, 360, "timer_result");

Is 50 microseconds enough time to reliably wait for the timer callback to
execute?

In CI environments or heavily loaded virtualization systems, might the
scheduling and softirq execution be delayed by more than a fraction of a
millisecond? If the timer does not fire in exactly that short window, the
test would incorrectly read a timer_result of 0 and fail.

Would it be more robust to use a polling loop or a significantly larger
timeout to avoid flaky tests?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/stack_arg.c b/tools/testing/selftests/bpf/progs/stack_arg.c
> new file mode 100644
> index 0000000000000..8c198ee952ff4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/stack_arg.c

[ ... ]

> +/* bar calls foo1 (3 stack args) and foo2 (5 stack args).
> + * The outgoing stack arg area is sized for foo2 (the larger callee).
> + * Stores for foo1 are a subset of the area used by foo2.
> + * Result: foo1(1,2,3,4,5,6,7,8) + foo2(1,2,3,4,5,6,7,8,9,10) = 36 + 55 = 91
> + *
> + * Pass a-e through so the compiler can't constant-fold the stack args away.
> + */
> +__noinline int global_two_callees(int a, int b, int c, int d, int e)

This isn't a bug, but the comment refers to a function named bar, while the
function being described is actually named global_two_callees.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/stack_arg_kfunc.c b/tools/testing/selftests/bpf/progs/stack_arg_kfunc.c
> new file mode 100644
> index 0000000000000..6cc404d578639
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/stack_arg_kfunc.c

[ ... ]

> +#if defined(__TARGET_ARCH_x86) && defined(__BPF_FEATURE_STACK_ARGUMENT)
> +
> +const volatile bool has_stack_arg = true;
> +
> +struct bpf_iter_testmod_seq {
> +	u64 :64;
> +	u64 :64;
> +};
> +
> +extern int bpf_iter_testmod_seq_new(struct bpf_iter_testmod_seq *it, s64 value, int cnt) __ksym;
> +extern int *bpf_iter_testmod_seq_next(struct bpf_iter_testmod_seq *it) __ksym;

Does bpf_iter_testmod_seq_next() actually return an int pointer?

Looking at the corresponding kfunc definition in bpf_testmod.c, the function
appears to return an s64 pointer. Although this function isn't actively called
in this test file (which prevents a BTF verifier failure here), could the
mismatched return type declaration cause confusion or subtle issues in the
future?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260419163316.731019-1-yonghong.song@linux.dev?part=15