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.
>
> 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.

That was exactly in v1 and reviewers suggested this approach !

Thx,
-Vineet