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

[sashiko-bot@kernel.org]

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.

[ ... ]

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

> +{
> +	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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260414132421.63409-1-leon.hwang@linux.dev?part=8