Hi,
On 2/26/2025 10:54 PM, Leon Hwang wrote:
>
> 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.
Er, I see. In my opinion, checking map_type == BPF_MAP_TYPE_ARRAY twice
(one in its only caller and one in itself) is a bit weird.
>
> 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 for the explanation. I mis-understood the code and my original
though was it was only trying to read somthing from the per-cpu array
map value.
>
> Thanks,
> Leon
