Re: [PATCH bpf-next v2 1/4] selftests/bpf: map_kptr: expect BPF_ST reject msg on cpuv4 toolchains

On 8/7/26 1:53 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The patch conditionally alters the expected verifier error message for BPF_ST toolchains, which eliminates the test's intended coverage (verifying that `map_kptr_match_type()` rejects scalar registers) on modern compilers and introduces brittleness tied to compiler optimization levels. As acknowledged in the commit message, the test now bypasses the original validation path, and changes in optimization (e.g., compiling at -O0) will break the `#ifdef` logic because the compiler may still emit `BPF_STX`. A more robust approach is to force the compiler to emit `BPF_STX` (e.g., by using a 64-bit constant that cannot be encoded as a 32-bit immediate, or by using `barrier_var()`), which preserves the intended verifier coverage without relying on fragile macro checks.
> --
>
> --- Patch [1]: [PATCH bpf-next v2 1/4] selftests/bpf: map_kptr: expect BPF_ST reject msg on cpuv4 toolchains ---
> commit 302e7847f20b43d2cf543bac72a29e4a57633dfa
> Author: Vineet Gupta <[email protected]>
>
> selftests/bpf: map_kptr: expect BPF_ST reject msg on cpuv4 toolchains
>
> This commit modifies the reject_scalar_store_to_kptr test to expect a
> different verifier error message on toolchains supporting BPF_ST. This
> avoids a test failure when the compiler folds the constant into a
> BPF_ST store immediate instruction instead of using BPF_STX.
>
>> diff --git a/tools/testing/selftests/bpf/progs/map_kptr_fail.c b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
>> index f11848dfa78f9..5e25ca806060c 100644
>> --- a/tools/testing/selftests/bpf/progs/map_kptr_fail.c
>> +++ b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
>> @@ -386,7 +386,16 @@ int kptr_xchg_possibly_null(struct __sk_buff *ctx)
>>   }
>>   
>>   SEC("?tc")
>> +/*
>> + * A compiler with BPF_ST folds the constant into a store-immediate, which the
>> + * verifier rejects on a different path (and with a different message) than the
>> + * BPF_STX form.
>> + */
>> +#ifdef __BPF_FEATURE_ST
>> +__failure __msg("BPF_ST imm must be 0 when storing to kptr at off=8")
>> +#else
>>   __failure __msg("invalid kptr access, R")
>> +#endif
> [Severity: Medium]
> Does this conditionally altered expectation eliminate the test's intended
> coverage on modern toolchains?
>
> As noted in the commit message, the test now bypasses the original validation
> path on BPF_ST builds, meaning it no longer verifies that
> map_kptr_match_type() rejects scalar registers.
>
> This also appears to introduce brittleness. If the optimization level changes
> (such as compiling at -O0), the compiler might still emit BPF_STX, which would
> evaluate the wrong macro branch and fail the test.

The -O0 is not really working for most bpf programs. The current selftests are
using -O2 so let us stick with it (-O2).

>
> Would it be more robust to force the compiler to emit BPF_STX here, for
> example by using a 64-bit constant that cannot be encoded as a 32-bit
> immediate, or by using barrier_var()? This could preserve the intended
> verifier coverage across different toolchains and optimization levels.

The__BPF_FEATURE_ST macro in llvm is able to differentiate cpu v3 and v4. 
This should work for both gcc and clang if gcc follows clang macros.

>
>>   int reject_scalar_store_to_kptr(struct __sk_buff *ctx)
>>   {
>>   	struct map_value *v;