| Message ID | 20240206220441.38311-3-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Introduce BPF arena. | expand |
On Tue, Feb 06, 2024 at 02:04:27PM -0800, Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@kernel.org> > > Recognize 'void *p__map' kfunc argument as 'struct bpf_map *p__map'. > It allows kfunc to have 'void *' argument for maps, since bpf progs > will call them as: > struct { > __uint(type, BPF_MAP_TYPE_ARENA); > ... > } arena SEC(".maps"); > > bpf_kfunc_with_map(... &arena ...); > > Underneath libbpf will load CONST_PTR_TO_MAP into the register via ld_imm64 insn. > If kfunc was defined with 'struct bpf_map *' it would pass > the verifier, but bpf prog would need to use '(void *)&arena'. > Which is not clean. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > kernel/bpf/verifier.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index d9c2dbb3939f..db569ce89fb1 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a > return __kfunc_param_match_suffix(btf, arg, "__ign"); > } > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) > +{ > + return __kfunc_param_match_suffix(btf, arg, "__map"); > +} > + > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) > { > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > @@ -11064,7 +11069,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, > return KF_ARG_PTR_TO_CONST_STR; > > if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { > - if (!btf_type_is_struct(ref_t)) { > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > return -EINVAL; > @@ -11660,6 +11665,13 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > if (kf_arg_type < 0) > return kf_arg_type; > > + if (is_kfunc_arg_map(btf, &args[i])) { > + /* If argument has '__map' suffix expect 'struct bpf_map *' */ > + ref_id = *reg2btf_ids[CONST_PTR_TO_MAP]; > + ref_t = btf_type_by_id(btf_vmlinux, ref_id); > + ref_tname = btf_name_by_offset(btf, ref_t->name_off); > + } This is fine, but given that this should only apply to KF_ARG_PTR_TO_BTF_ID, this seems a bit cleaner, wdyt? index ddaf09db1175..998da8b302ac 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a return __kfunc_param_match_suffix(btf, arg, "__ign"); } +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) +{ + return __kfunc_param_match_suffix(btf, arg, "__map"); +} + static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) { return __kfunc_param_match_suffix(btf, arg, "__alloc"); @@ -10910,6 +10915,7 @@ enum kfunc_ptr_arg_type { KF_ARG_PTR_TO_RB_NODE, KF_ARG_PTR_TO_NULL, KF_ARG_PTR_TO_CONST_STR, + KF_ARG_PTR_TO_MAP, /* pointer to a struct bpf_map */ }; enum special_kfunc_type { @@ -11064,12 +11070,12 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, return KF_ARG_PTR_TO_CONST_STR; if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { - if (!btf_type_is_struct(ref_t)) { + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", meta->func_name, argno, btf_type_str(ref_t), ref_tname); return -EINVAL; } - return KF_ARG_PTR_TO_BTF_ID; + return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; } if (is_kfunc_arg_callback(env, meta->btf, &args[argno])) @@ -11663,6 +11669,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ switch (kf_arg_type) { case KF_ARG_PTR_TO_NULL: continue; + case KF_ARG_PTR_TO_MAP: case KF_ARG_PTR_TO_ALLOC_BTF_ID: case KF_ARG_PTR_TO_BTF_ID: if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta)) @@ -11879,6 +11886,13 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (ret < 0) return ret; break; + case KF_ARG_PTR_TO_MAP: + /* If argument has '__map' suffix expect 'struct bpf_map *' */ + ref_id = *reg2btf_ids[CONST_PTR_TO_MAP]; + ref_t = btf_type_by_id(btf_vmlinux, ref_id); + ref_tname = btf_name_by_offset(btf, ref_t->name_off); + + fallthrough; case KF_ARG_PTR_TO_BTF_ID: /* Only base_type is checked, further checks are done here */ if ((base_type(reg->type) != PTR_TO_BTF_ID || > + > switch (kf_arg_type) { > case KF_ARG_PTR_TO_NULL: > continue; > -- > 2.34.1 > >
On Fri, Feb 09, 2024 at 10:57:45AM -0600, David Vernet wrote: > On Tue, Feb 06, 2024 at 02:04:27PM -0800, Alexei Starovoitov wrote: > > From: Alexei Starovoitov <ast@kernel.org> > > > > Recognize 'void *p__map' kfunc argument as 'struct bpf_map *p__map'. > > It allows kfunc to have 'void *' argument for maps, since bpf progs > > will call them as: > > struct { > > __uint(type, BPF_MAP_TYPE_ARENA); > > ... > > } arena SEC(".maps"); > > > > bpf_kfunc_with_map(... &arena ...); > > > > Underneath libbpf will load CONST_PTR_TO_MAP into the register via ld_imm64 insn. > > If kfunc was defined with 'struct bpf_map *' it would pass > > the verifier, but bpf prog would need to use '(void *)&arena'. > > Which is not clean. > > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > > --- > > kernel/bpf/verifier.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index d9c2dbb3939f..db569ce89fb1 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a > > return __kfunc_param_match_suffix(btf, arg, "__ign"); > > } > > > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) > > +{ > > + return __kfunc_param_match_suffix(btf, arg, "__map"); > > +} > > + > > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) > > { > > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > > @@ -11064,7 +11069,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, > > return KF_ARG_PTR_TO_CONST_STR; > > > > if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { > > - if (!btf_type_is_struct(ref_t)) { > > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > > verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", > > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > > return -EINVAL; > > @@ -11660,6 +11665,13 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > > if (kf_arg_type < 0) > > return kf_arg_type; > > > > + if (is_kfunc_arg_map(btf, &args[i])) { > > + /* If argument has '__map' suffix expect 'struct bpf_map *' */ > > + ref_id = *reg2btf_ids[CONST_PTR_TO_MAP]; > > + ref_t = btf_type_by_id(btf_vmlinux, ref_id); > > + ref_tname = btf_name_by_offset(btf, ref_t->name_off); > > + } > > This is fine, but given that this should only apply to KF_ARG_PTR_TO_BTF_ID, > this seems a bit cleaner, wdyt? > > index ddaf09db1175..998da8b302ac 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a > return __kfunc_param_match_suffix(btf, arg, "__ign"); > } > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) > +{ > + return __kfunc_param_match_suffix(btf, arg, "__map"); > +} > + > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) > { > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > @@ -10910,6 +10915,7 @@ enum kfunc_ptr_arg_type { > KF_ARG_PTR_TO_RB_NODE, > KF_ARG_PTR_TO_NULL, > KF_ARG_PTR_TO_CONST_STR, > + KF_ARG_PTR_TO_MAP, /* pointer to a struct bpf_map */ > }; > > enum special_kfunc_type { > @@ -11064,12 +11070,12 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, > return KF_ARG_PTR_TO_CONST_STR; > > if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { > - if (!btf_type_is_struct(ref_t)) { > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > return -EINVAL; > } > - return KF_ARG_PTR_TO_BTF_ID; > + return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; 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; + if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { - if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { + if (!btf_type_is_struct(ref_t)) { verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", meta->func_name, argno, btf_type_str(ref_t), ref_tname); return -EINVAL; } - return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; + return KF_ARG_PTR_TO_BTF_ID; } ?
On Fri, Feb 09, 2024 at 09:46:57AM -0800, Alexei Starovoitov wrote: > On Fri, Feb 09, 2024 at 10:57:45AM -0600, David Vernet wrote: > > On Tue, Feb 06, 2024 at 02:04:27PM -0800, Alexei Starovoitov wrote: > > > From: Alexei Starovoitov <ast@kernel.org> > > > > > > Recognize 'void *p__map' kfunc argument as 'struct bpf_map *p__map'. > > > It allows kfunc to have 'void *' argument for maps, since bpf progs > > > will call them as: > > > struct { > > > __uint(type, BPF_MAP_TYPE_ARENA); > > > ... > > > } arena SEC(".maps"); > > > > > > bpf_kfunc_with_map(... &arena ...); > > > > > > Underneath libbpf will load CONST_PTR_TO_MAP into the register via ld_imm64 insn. > > > If kfunc was defined with 'struct bpf_map *' it would pass > > > the verifier, but bpf prog would need to use '(void *)&arena'. > > > Which is not clean. > > > > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > > > --- > > > kernel/bpf/verifier.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index d9c2dbb3939f..db569ce89fb1 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a > > > return __kfunc_param_match_suffix(btf, arg, "__ign"); > > > } > > > > > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) > > > +{ > > > + return __kfunc_param_match_suffix(btf, arg, "__map"); > > > +} > > > + > > > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) > > > { > > > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > > > @@ -11064,7 +11069,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, > > > return KF_ARG_PTR_TO_CONST_STR; > > > > > > if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { > > > - if (!btf_type_is_struct(ref_t)) { > > > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > > > verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", > > > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > > > return -EINVAL; > > > @@ -11660,6 +11665,13 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > > > if (kf_arg_type < 0) > > > return kf_arg_type; > > > > > > + if (is_kfunc_arg_map(btf, &args[i])) { > > > + /* If argument has '__map' suffix expect 'struct bpf_map *' */ > > > + ref_id = *reg2btf_ids[CONST_PTR_TO_MAP]; > > > + ref_t = btf_type_by_id(btf_vmlinux, ref_id); > > > + ref_tname = btf_name_by_offset(btf, ref_t->name_off); > > > + } > > > > This is fine, but given that this should only apply to KF_ARG_PTR_TO_BTF_ID, > > this seems a bit cleaner, wdyt? > > > > index ddaf09db1175..998da8b302ac 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a > > return __kfunc_param_match_suffix(btf, arg, "__ign"); > > } > > > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) > > +{ > > + return __kfunc_param_match_suffix(btf, arg, "__map"); > > +} > > + > > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) > > { > > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > > @@ -10910,6 +10915,7 @@ enum kfunc_ptr_arg_type { > > KF_ARG_PTR_TO_RB_NODE, > > KF_ARG_PTR_TO_NULL, > > KF_ARG_PTR_TO_CONST_STR, > > + KF_ARG_PTR_TO_MAP, /* pointer to a struct bpf_map */ > > }; > > > > enum special_kfunc_type { > > @@ -11064,12 +11070,12 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, > > return KF_ARG_PTR_TO_CONST_STR; > > > > if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { > > - if (!btf_type_is_struct(ref_t)) { > > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > > verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", > > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > > return -EINVAL; > > } > > - return KF_ARG_PTR_TO_BTF_ID; > > + return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; > > 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; + } + return KF_ARG_PTR_TO_MAP; + } + > if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { > - if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > + if (!btf_type_is_struct(ref_t)) { > verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > return -EINVAL; > } > - return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; > + return KF_ARG_PTR_TO_BTF_ID; > } > > ? >
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
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().
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d9c2dbb3939f..db569ce89fb1 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a return __kfunc_param_match_suffix(btf, arg, "__ign"); } +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg) +{ + return __kfunc_param_match_suffix(btf, arg, "__map"); +} + static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg) { return __kfunc_param_match_suffix(btf, arg, "__alloc"); @@ -11064,7 +11069,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, return KF_ARG_PTR_TO_CONST_STR; if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { - if (!btf_type_is_struct(ref_t)) { + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", meta->func_name, argno, btf_type_str(ref_t), ref_tname); return -EINVAL; @@ -11660,6 +11665,13 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (kf_arg_type < 0) return kf_arg_type; + if (is_kfunc_arg_map(btf, &args[i])) { + /* If argument has '__map' suffix expect 'struct bpf_map *' */ + ref_id = *reg2btf_ids[CONST_PTR_TO_MAP]; + ref_t = btf_type_by_id(btf_vmlinux, ref_id); + ref_tname = btf_name_by_offset(btf, ref_t->name_off); + } + switch (kf_arg_type) { case KF_ARG_PTR_TO_NULL: continue;