Re: [PATCH bpf-next v5 15/16] selftests/bpf: Add negative test for greater-than-8-byte kfunc stack argument

On 4/16/26 9:28 PM, [email protected] wrote:
> Sashiko AI review found 2 potential issue(s):
> - [Medium] The `prog_test_big_arg` struct uses `long`, which fails to test the >8 byte limit on 32-bit architectures and causes BTF size mismatches.
> - [Medium] Using `ASSERT_ERR(stack_arg_fail__load(skel))` masks the actual verifier error reason, allowing the negative test to pass for unrelated reasons.
> --
>
> commit 1ec644adfded3aa88befcb32fc73958c68b7e018
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add negative test for greater-than-8-byte kfunc stack argument
>
> This commit adds a selftest to ensure the verifier rejects kfunc calls where
> a stack argument exceeds the 8-byte register-sized slot limit. It introduces
> a new kfunc taking a struct argument and a BPF program that attempts to pass
> a large struct to it.
>
>> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> index 2c1cb118f8867..2a40f80b074a3 100644
>> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> @@ -50,6 +50,11 @@ struct prog_test_pass2 {
>>   	} x;
>>   };
>>   
>> +struct prog_test_big_arg {
>> +	long a;
>> +	long b;
>> +};
>> +
> Does this structure achieve the intended >8 byte size on 32-bit architectures?
>
> When the test kernel module is compiled on a 32-bit host, long is 32-bit,
> making the struct exactly 8 bytes. Since the BPF target always treats long
> as a 64-bit integer, this makes the struct 16 bytes in the BPF program.
>
> This size discrepancy between the BPF program's BTF and the host kernel's BTF
> might cause the verifier to reject the load for a BTF size mismatch rather
> than the intended stack argument limit.
>
> Could these fields use explicitly sized types like __u64 to ensure the struct
> is consistently larger than 8 bytes across all architectures?

Good point, using __64 is much better.

>
> [ ... ]
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/stack_arg_fail.c b/tools/testing/selftests/bpf/prog_tests/stack_arg_fail.c
>> new file mode 100644
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/stack_arg_fail.c
>> @@ -0,0 +1,24 @@
> [ ... ]
>> +	if (!skel->rodata->has_stack_arg) {
>> +		test__skip();
>> +		goto out;
>> +	}
>> +
>> +	ASSERT_ERR(stack_arg_fail__load(skel), "load_should_fail");
> Could this generic error assertion mask the actual verifier rejection reason?
>
> If the program fails to load due to an unrelated issue, such as a missing BTF,
> a syntax error, or the architecture-dependent size mismatch described above,
> the test will incorrectly report success because it only checks that the load
> operation failed.
>
> Would it be better to refactor this test to use the standard test_loader
> infrastructure (via RUN_TESTS) with __failure and __msg annotations
> in the BPF program? That would ensure the rejection is explicitly due to the
> argument size limit.

will do RUN_TESTS style for this test.

>
>> +
>> +out:
>> +	stack_arg_fail__destroy(skel);
>> +}