Re: [PATCH bpf-next 02/16] bpf: Recognize '__map' suffix in kfunc arguments

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Feb 9, 2024 at 10:11 AM David Vernet <void@manifault.com> wrote:
> >
> > Makes sense, but then should I add the following on top:
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e970d9fd7f32..b524dc168023 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -11088,13 +11088,16 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
> >         if (is_kfunc_arg_const_str(meta->btf, &args[argno]))
> >                 return KF_ARG_PTR_TO_CONST_STR;
> >
> > +       if (is_kfunc_arg_map(meta->btf, &args[argno]))
> > +               return KF_ARG_PTR_TO_MAP;
> > +
>
> Yeah, it's probably cleaner to pull it out of that block, which is
> already a bit of a mess.
>
> Only thing is that it doesn't make sense to invoke is_kfunc_arg_map() on
> something that doesn't have base_type(reg->type) == CONST_PTR_TO_MAP
> right? We sort of had that covered in the below block beacuse of the
> reg2btf_ids[base_type(reg->type)] check, but even then it was kind of
> sketchy because we could have base_type(reg->type) == PTR_TO_BTF_ID or
> some other base_type with a nonzero btf ID and still treat it as a
> KF_ARG_PTR_TO_MAP depending on how the kfunc was named. So maybe
> something like this would be yet another improvement on top of both
> proposals that would avoid any weird edge cases or confusion on the part
> of the kfunc author?
>
> + if (is_kfunc_arg_map(meta->btf, &args[argno])) {
> +         if (base_type(reg->type) != CONST_PTR_TO_MAP) {
> +                 verbose(env, "kernel function %s map arg#%d %s reg was not type %s\n",
> +                         meta->func_name, argno, ref_name, reg_type_str(env, CONST_PTR_TO_MAP));
> +                 return -EINVAL;
> +         }

This would be an unnecessary restriction.
We should allow this to work:

+SEC("iter.s/bpf_map")
+__success __log_level(2)
+int iter_maps(struct bpf_iter__bpf_map *ctx)
+{
+       struct bpf_map *map = ctx->map;
+
+       if (!map)
+               return 0;
+       bpf_arena_alloc_pages(map, NULL, map->max_entries, NUMA_NO_NODE, 0);
+       return 0;
+}

verifier log:
0: R1=ctx() R10=fp0
; struct bpf_map *map = ctx->map;
0: (79) r1 = *(u64 *)(r1 +8)          ; R1_w=trusted_ptr_or_null_bpf_map(id=1)
; if (map == (void *)0)
1: (15) if r1 == 0x0 goto pc+5        ; R1_w=trusted_ptr_bpf_map()
; bpf_arena_alloc_pages(map, NULL, map->max_entries, NUMA_NO_NODE, 0);
2: (61) r3 = *(u32 *)(r1 +36)         ; R1_w=trusted_ptr_bpf_map()
R3_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
; bpf_arena_alloc_pages(map, NULL, map->max_entries, NUMA_NO_NODE, 0);
3: (b7) r2 = 0                        ; R2_w=0
4: (b4) w4 = -1                       ; R4_w=0xffffffff
5: (b7) r5 = 0                        ; R5_w=0
6: (85) call bpf_arena_alloc_pages#42141      ; R0=scalar()

the following two tests fail as expected:

1.
int iter_maps(struct bpf_iter__bpf_map *ctx)
{
  struct seq_file *seq = ctx->meta->seq;
  struct bpf_map *map = ctx->map;

  bpf_arena_alloc_pages((void *)seq, NULL, map->max_entries, NUMA_NO_NODE, 0);

kernel function bpf_arena_alloc_pages args#0 expected pointer to
STRUCT bpf_map but R1 has a pointer to STRUCT seq_file

2.
  bpf_arena_alloc_pages(map->inner_map_meta, NULL, map->max_entries,
NUMA_NO_NODE, 0);

(79) r1 = *(u64 *)(r1 +8)          ; R1_w=untrusted_ptr_bpf_map()
R1 must be referenced or trusted