Re: [PATCH bpf-next v4 8/8] selftests/bpf: Add a test to verify bpf_iter for global percpu data

Leon Hwang
2026-04-17T02:17:36Z

On 15/4/26 06:08, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 3 potential issue(s):
> - [Medium] The `dump_percpu_data` program is autoloaded by default, breaking existing subtests on older kernels.
> - [Medium] Checking `err == -EOPNOTSUPP` after skeleton load incorrectly handles unsupported kernels.
> - [Medium] Casting the global datasec to an anonymous C struct relies on fragile compiler layout assumptions.
> --
>
> commit 1e00e3e2b318918e67a2c7d2e251132e1b985484
> Author: Leon Hwang <leon.hwang@linux.dev>
>
> selftests/bpf: Add a test to verify bpf_iter for global percpu data
>
> Add a test to verify that it is OK to iter the percpu_array map used for
> global percpu data.
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data_init.c b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> index 0b0384b6515ed..9ff09ff1981c4 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>
> [ ... ]
>
>> @@ -269,6 +269,57 @@ static void test_global_percpu_data_verifier_failure(void)
>> +static void test_global_percpu_data_iter(void)
>> +{
>
> [ ... ]
>
>> + err = test_global_percpu_data__load(skel);
>> + if (err == -EOPNOTSUPP) {
>> + test__skip();
>> + goto out;
>> + }
>
> Will this correctly skip on older kernels? When the kernel verifier
> rejects an unsupported program type or feature during bpf_prog_load(),
> it typically returns -EINVAL (or -EACCES), not -EOPNOTSUPP.
>
> Libbpf passes this error code back to the caller, so this condition
> might evaluate to false, causing the test to fail on the subsequent
> ASSERT_OK(err) check instead of gracefully skipping.
>

This err check should be dropped, because this test won't run when the
kernel does not support global percpu data.

See previous patch:

+void test_global_percpu_data(void)
+{
+ if (!feat_supported(NULL, FEAT_PERCPU_DATA)) {
+ test__skip();
+ return;
+ }

> [ ... ]
>
>> diff --git a/tools/testing/selftests/bpf/progs/test_global_percpu_data.c b/tools/testing/selftests/bpf/progs/test_global_percpu_data.c
>> index 947721c21f30c..0ddb820509ebf 100644
>> --- a/tools/testing/selftests/bpf/progs/test_global_percpu_data.c
>> +++ b/tools/testing/selftests/bpf/progs/test_global_percpu_data.c
>
> [ ... ]
>
>> +SEC("iter/bpf_map_elem")
>> +__auxiliary
>> +int dump_percpu_data(struct bpf_iter__bpf_map_elem *ctx)
>
> Will this break existing subtests on older kernels? The bpf program
> dump_percpu_data is defined with SEC("iter/bpf_map_elem") without the
> ? prefix, meaning libbpf will attempt to autoload it whenever
> test_global_percpu_data__load() is called.
>
> Since the existing init and lskel subtests also load the full skeleton,
> they might inadvertently attempt to load this new iterator program.
> Should this use SEC("?iter/bpf_map_elem") to prevent autoloading by
> default, and only be explicitly autoloaded in the iter subtest?
>

Since older kernels do not have global percpu data support, this test
won't run on them. See aforementioned explanation.

>> +{
>> + struct {
>> + int data;
>> + char run;
>> + struct {
>> + char set;
>> + int i;
>> + int nums[7];
>> + } struct_data;
>> + int nums[7];
>> + } *pptr = ctx->value;
>
> Is it safe to cast the global datasec to an anonymous C struct like
> this?
>
> In the BPF source, these are declared as independent global variables.
> The layout of these variables in the .percpu ELF section is determined
> entirely by the compiler. While the current compiler version might emit
> the uninitialized nums array after the initialized variables (matching
> the struct layout), this relies on undocumented compiler behavior.
>
> If the compiler alters the layout, or if variables are added or
> initialized differently in the future, the field offsets will mismatch.
> This could cause the step calculation to diverge from the map's actual
> value_size, leading to out-of-bounds reads.
>
> Would it be safer to group these variables into a single explicit struct
> in the BPF code to guarantee their memory layout?
>
> [ ... ]
>

Correct, I should not use such an anonymous struct, even if it matches
the generated skeleton's embedded struct.

A better approach is to locate the int by offset, which will match its
offset in the generated skeleton's embedded struct.

Thanks,
Leon