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.
> >> 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.
