| Message ID | 20221118185938.2139616-2-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Follow ups for bpf-list set | expand |
On Fri, Nov 18, 2022 at 11:00 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Instead of checking bpf_global_ma_set at runtime on each allocation > inside bpf_obj_new_impl, simply disallow calling the kfunc in case > bpf_global_ma initialization failed during program verification. > > The error generated when bpf_global_ma initialization fails: > ... > 21: (18) r1 = 0x7 ; R1_w=7 > 23: (b7) r2 = 0 ; R2_w=0 > 24: (85) call bpf_obj_new_impl#36585 > bpf_global_ma initialization failed, can't call bpf_obj_new_impl > calling kernel function bpf_obj_new_impl is not allowed > > Suggested-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/helpers.c | 2 -- > kernel/bpf/verifier.c | 13 ++++++++++++- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 212e791d7452..bc02f55adc1f 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign) > u64 size = local_type_id__k; > void *p; > > - if (unlikely(!bpf_global_ma_set)) > - return NULL; > p = bpf_mem_alloc(&bpf_global_ma, size); > if (!p) > return NULL; > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 195d24316750..f04bee7934a8 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > return 0; > } > > +static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id) > +{ > + if (btf != btf_vmlinux) > + return false; > + if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) { > + verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n"); > + return true; > + } > + return false; > +} > + This is all just unnecessary code bloat for the case that cannot happen. When you do: meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl] just add if (!bpf_global_ma_set) return -ENOMEM; No need for verbose(). The users will never hit it. Also please get rid of special_kfunc_set and and btf_id_set_contains(&special_kfunc_set, meta.func_id) That additional check is unnecessary as well. special_kfunc_list is enough. I'm going to apply patch 2 to make CI green.
On Sat, Nov 19, 2022 at 01:47:25AM IST, Alexei Starovoitov wrote: > On Fri, Nov 18, 2022 at 11:00 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Instead of checking bpf_global_ma_set at runtime on each allocation > > inside bpf_obj_new_impl, simply disallow calling the kfunc in case > > bpf_global_ma initialization failed during program verification. > > > > The error generated when bpf_global_ma initialization fails: > > ... > > 21: (18) r1 = 0x7 ; R1_w=7 > > 23: (b7) r2 = 0 ; R2_w=0 > > 24: (85) call bpf_obj_new_impl#36585 > > bpf_global_ma initialization failed, can't call bpf_obj_new_impl > > calling kernel function bpf_obj_new_impl is not allowed > > > > Suggested-by: Alexei Starovoitov <ast@kernel.org> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/helpers.c | 2 -- > > kernel/bpf/verifier.c | 13 ++++++++++++- > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > index 212e791d7452..bc02f55adc1f 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign) > > u64 size = local_type_id__k; > > void *p; > > > > - if (unlikely(!bpf_global_ma_set)) > > - return NULL; > > p = bpf_mem_alloc(&bpf_global_ma, size); > > if (!p) > > return NULL; > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 195d24316750..f04bee7934a8 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > > return 0; > > } > > > > +static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id) > > +{ > > + if (btf != btf_vmlinux) > > + return false; > > + if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) { > > + verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n"); > > + return true; > > + } > > + return false; > > +} > > + > > This is all just unnecessary code bloat for the case > that cannot happen. > > When you do: > meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl] > just add > if (!bpf_global_ma_set) > return -ENOMEM; > > No need for verbose(). The users will never hit it. > Ok, makes sense, will fix and resend. > Also please get rid of special_kfunc_set and > and btf_id_set_contains(&special_kfunc_set, meta.func_id) > That additional check is unnecessary as well. > special_kfunc_list is enough. It provides an easy way to do 'btf == vmlinux && is_special_kfunc'. Otherwise if I drop it, every check matching func_id == special_kfunc_list will also have to do btf == vmlinux check (because eventually we want to drop to the other branches that should work for non-vmlinux BTF as well). What I mean is: if (btf == vmlinux && btf_id_set_contains(special_kfunc_set)) { if (func_id == special_kfunc_list) { } else if (func_id == special_kfunc_list) { } else { } } else if (!__btf_type_is_struct) { } else /* struct */ { } will become: if (btf == vmlinux && func_id == special_kfunc_list) { } else if (btf == vmlinux && func_id == special_kfunc_list) { } else if (!__btf_type_is_struct) { } else /* struct */ { } I think it's better to keep it. It also fails the kfunc call if the return type is not properly handled (either custom implementation or just falling through to default cases, w/e applies). But up to you.
On Mon, Nov 21, 2022 at 02:16:25AM +0530, Kumar Kartikeya Dwivedi wrote: > > > Also please get rid of special_kfunc_set and > > and btf_id_set_contains(&special_kfunc_set, meta.func_id) > > That additional check is unnecessary as well. > > special_kfunc_list is enough. > > It provides an easy way to do 'btf == vmlinux && is_special_kfunc'. > Otherwise if I drop it, every check matching func_id == special_kfunc_list will > also have to do btf == vmlinux check (because eventually we want to drop to the > other branches that should work for non-vmlinux BTF as well). > > What I mean is: > if (btf == vmlinux && btf_id_set_contains(special_kfunc_set)) { > if (func_id == special_kfunc_list) { > } else if (func_id == special_kfunc_list) { > } else { > } > } else if (!__btf_type_is_struct) { > } else /* struct */ { > } > > will become: > if (btf == vmlinux && func_id == special_kfunc_list) { > } else if (btf == vmlinux && func_id == special_kfunc_list) { > } else if (!__btf_type_is_struct) { > } else /* struct */ { > } One less indent looks better. Repeated btf == vmlinux doesn't bother me. There is no use for this special_kfunc_set. It just wastes memory. So I still think it's better to remove it. While at it please swap branches !__btf_type_is_struct to __btf_type_is_struct: else if (__btf_type_is_struct) { regs[BPF_REG_0].type = PTR_TO_BTF_ID } else { // handle 'void *' return aka r0_size would read better.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 212e791d7452..bc02f55adc1f 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign) u64 size = local_type_id__k; void *p; - if (unlikely(!bpf_global_ma_set)) - return NULL; p = bpf_mem_alloc(&bpf_global_ma, size); if (!p) return NULL; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 195d24316750..f04bee7934a8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ return 0; } +static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id) +{ + if (btf != btf_vmlinux) + return false; + if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) { + verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n"); + return true; + } + return false; +} + static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, int *insn_idx_p) { @@ -8773,7 +8784,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, func_proto = btf_type_by_id(desc_btf, func->type); kfunc_flags = btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog), func_id); - if (!kfunc_flags) { + if (!kfunc_flags || is_kfunc_disabled(env, desc_btf, func_id)) { verbose(env, "calling kernel function %s is not allowed\n", func_name); return -EACCES;
Instead of checking bpf_global_ma_set at runtime on each allocation inside bpf_obj_new_impl, simply disallow calling the kfunc in case bpf_global_ma initialization failed during program verification. The error generated when bpf_global_ma initialization fails: ... 21: (18) r1 = 0x7 ; R1_w=7 23: (b7) r2 = 0 ; R2_w=0 24: (85) call bpf_obj_new_impl#36585 bpf_global_ma initialization failed, can't call bpf_obj_new_impl calling kernel function bpf_obj_new_impl is not allowed Suggested-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/helpers.c | 2 -- kernel/bpf/verifier.c | 13 ++++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-)