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

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]

On Fri, Feb 09, 2024 at 10:59:57AM -0800, Alexei Starovoitov wrote:
> 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;
> +}

Ah, I see, so this would be a PTR_TO_BTF_ID then. Fair enough, we can
leave that restriction off and rely on the check in
process_kf_arg_ptr_to_btf_id().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]