On 2025/2/26 23:31, Alexei Starovoitov wrote:
> On Wed, Feb 26, 2025 at 6:54 AM Leon Hwang <leon.hwang@xxxxxxxxx> 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.
>>
>> 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?
>
> Absolutely not and not tomorrow either. There is no use case
> for readonly percpu data. It's only a waste of memory.
>
Yeah. I realize it absolutely wastes memory for read-only percpu data
after sending this message.
>>>> 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.
>
> I suspect what Hou is hinting at that if percpu array rejected
> BPF_F_RDONLY_PROG in map_alloc_check() there would be no need
> to special case everything but "+ map->map_type == BPF_MAP_TYPE_ARRAY"
> here.
We could reject BPF_F_RDONLY_PROG in array_map_check_btf() instead, as
it can recognize when a percpu array is used for .percpu.
However, can we completely eliminate all map->map_type checks except for
this one? I have my doubts. Those checks are in place to prevent the
misuse of percpu data.
