Re: [RESEND PATCH bpf-next v2 1/4] bpf: Introduce global percpu data

[Leon Hwang <leon.hwang@linux.dev>]

On 2025/2/26 10:19, Hou Tao wrote:
> Hi,
> 

[...]

>> @@ -815,6 +850,8 @@ const struct bpf_map_ops percpu_array_map_ops = {
>>  	.map_get_next_key = array_map_get_next_key,
>>  	.map_lookup_elem = percpu_array_map_lookup_elem,
>>  	.map_gen_lookup = percpu_array_map_gen_lookup,
>> +	.map_direct_value_addr = percpu_array_map_direct_value_addr,
>> +	.map_direct_value_meta = percpu_array_map_direct_value_meta,
>>  	.map_update_elem = array_map_update_elem,
>>  	.map_delete_elem = array_map_delete_elem,
>>  	.map_lookup_percpu_elem = percpu_array_map_lookup_percpu_elem,
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 9971c03adfd5d..5682546b1193e 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -6810,6 +6810,8 @@ static int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val,
>>  	u64 addr;
>>  	int err;
>>  
>> +	if (map->map_type != BPF_MAP_TYPE_ARRAY)
>> +		return -EINVAL;
> 
> Is the check still necessary ? Because its caller has already added the
> check of map_type.

Yes. It should check here in order to make sure the code logic in
bpf_map_direct_read() is robust enough.

But in check_mem_access(), if map is a read-only percpu array map, it
should not track its contents as SCALAR_VALUE, because the read-only
.percpu, named .ropercpu, hasn't been supported yet.

Should we implement .ropercpu in this patch set, too?

>>  	err = map->ops->map_direct_value_addr(map, &addr, off);
>>  	if (err)
>>  		return err;
>> @@ -7322,6 +7324,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>>  			/* if map is read-only, track its contents as scalars */
>>  			if (tnum_is_const(reg->var_off) &&
>>  			    bpf_map_is_rdonly(map) &&
>> +			    map->map_type == BPF_MAP_TYPE_ARRAY &&
>>  			    map->ops->map_direct_value_addr) {
>>  				int map_off = off + reg->var_off.value;
>>  				u64 val = 0;
> 
> Do we also need to check in check_ld_imm() to ensure the dst_reg of
> bpf_ld_imm64 on a per-cpu array will not be treated as a map-value-ptr ?
No. The dst_reg of ld_imm64 for percpu array map must be treated as
map-value-ptr, just like the one for array map.

Global percpu variable is very similar to global variable.

From the point of compiler, global percpu variable is global variable
with SEC(".percpu").

Then libbpf converts global data with SEC(".percpu") to global percpu
data. And bpftool generates struct for global percpu data like for
global data when generate skeleton.

Finally, verifier inserts a mov64_percpu_reg insn after the ld_imm64 in
order to add this_cpu_off to the dst_reg of ld_imm64.

Therefore, in check_ld_imm(), it should mark the dst_reg of ld_imm64 for
percpu array map as map-value-ptr.

Thanks,
Leon