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@xxxxxxxxxxxxx> 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:
signature.asc
Description: PGP signature
