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

On 15/4/26 06:08, [email protected] 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 <[email protected]>
> 
> 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