Re: [PATCH bpf-next v12 04/10] libbpf: Probe percpu data feature

On 13/8/26 23:38, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The feature probe `probe_kern_percpu_data` incorrectly returns a negative error code and logs a warning when the kernel lacks support for `BPF_MAP_TYPE_PERCPU_ARRAY`. Feature probes in libbpf should silently return 0 when a feature is missing to gracefully indicate its absence without polluting stderr.
> --
> 
> --- Patch [4]: [PATCH bpf-next v12 04/10] libbpf: Probe percpu data feature ---
> commit 783c3759ac5b8bbe1c5368930abe9953ff13c465
> Author: Leon Hwang <[email protected]>
> 
>     libbpf: Probe percpu data feature
>     
>     This commit adds a feature probe for global percpu data support. It allows
>     libbpf to detect if the kernel supports percpu data early and make capability
>     decisions accordingly.
> 
>> diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
>> index b7e388f99d0bb..ef9581c113035 100644
>> --- a/tools/lib/bpf/features.c
>> +++ b/tools/lib/bpf/features.c
> [ ... ]
>> +static int probe_kern_percpu_data(int token_fd)
>> +{
>> +	struct bpf_insn insns[] = {
>> +		BPF_LD_MAP_VALUE(BPF_REG_1, 0, 0),
>> +		BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 0),
>> +		BPF_EXIT_INSN(),
>> +	};
>> +	LIBBPF_OPTS(bpf_map_create_opts, map_opts,
>> +		.token_fd = token_fd,
>> +		.map_flags = token_fd ? BPF_F_TOKEN_FD : 0,
>> +	);
>> +	LIBBPF_OPTS(bpf_prog_load_opts, prog_opts,
>> +		.token_fd = token_fd,
>> +		.prog_flags = token_fd ? BPF_F_TOKEN_FD : 0,
>> +	);
>> +	int ret, map, insn_cnt = ARRAY_SIZE(insns);
>> +
>> +	map = bpf_map_create(BPF_MAP_TYPE_PERCPU_ARRAY, "libbpf_percpu", sizeof(int), 8, 1,
>> +			     &map_opts);
>> +	if (map < 0) {
>> +		pr_warn("Error in %s(): %s. Couldn't create simple percpu_array map.\n",
>> +			__func__, errstr(map));
>> +		return map;
>> +	}
> 
> [Severity: Medium]
> Will this cause spurious warnings when probing environments that don't support
> BPF_MAP_TYPE_PERCPU_ARRAY?
> 
> Since feat_supported() expects feature probes to return 0 when a feature is
> missing, returning a negative error code here causes feat_supported() to log
> an additional failure warning.
> 
> Should this return 0 instead of map (and perhaps omit the pr_warn) to
> gracefully indicate the feature's absence without polluting stderr?
> 

No.

Since percpu_array map is a basic map in bpf, this map creation should
likely succeed. If it fails, the kernels are too old to use such new libbpf.

Thanks,
Leon