On Fri, Nov 18, 2022 at 2:37 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Fri, Nov 18, 2022 at 09:04:15AM IST, Alexei Starovoitov wrote:
> > On Fri, Nov 18, 2022 at 07:26:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > const struct btf *btf, u32 func_id,
> > > struct bpf_reg_state *regs,
> > > bool ptr_to_mem_ok,
> > > - struct bpf_kfunc_arg_meta *kfunc_meta,
> > > bool processing_call)
> >
> > Something odd here.
> > Benjamin added the processing_call flag in
> > commit 95f2f26f3cac ("bpf: split btf_check_subprog_arg_match in two")
> > and we discussed to remove it.
> >
> > > } else if (ptr_to_mem_ok && processing_call) {
> >
> > since kfunc bit is gone from here the processing_call can be removed.
> > ptr_to_mem_ok and processing_call are two bool flags for the same thing, right?
> >
>
> I think so, I'll check it out and send a follow up patch.
>
> > > +static int process_kf_arg_ptr_to_kptr_strong(struct bpf_verifier_env *env,
> >
> > I fixed this bit while applying.
> >
>
> Thanks.
>
> > > +static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta)
> >
> > This function looks much better now.
> > The split of kfunc vs helper was long overdue.
> > Thank you for doing this.
> >
> > I'm not convinced that KF_ARG_* is necessary, but it's much better than before.
> > So it's a step forward.
> >
>
> Yes. Eventually we should be merging checks for both helpers and kfuncs, but
> that needs more work and would have been out of scope for this set. We can
> probably synthesize a bpf_func_proto for the kfunc from BTF and then offload to
> check_helper_call.
Yeah. If kfunc BTFs plus KF_ flags can be synthesized to bpf_func_proto
that would be the best. If such conversion is possible then it
should be possible to do it in resolve_btfid in user space.
One more thing that I forgot to mention earlier.
Could you follow up with a patch to get rid of bpf_global_ma_set
check in the run-time and variable itself?
If bpf_mem_alloc_init fails the boot fails too.
If we're paranoid we can add:
special_kfunc_list[KF_bpf_obj_new_impl] = 0;
to bpf_mem_alloc_init() to prevent bpf_obj_new to ever be called.
