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@xxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > 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.
