On Fri, Nov 18, 2022 at 11:32:21PM IST, Alexei Starovoitov wrote:
> 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.
>
Yep. I'll poke at it some more later.
> 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.
I did it a bit differently, but it does the same thing, and sent it out with the
s390x fix. PTAL.
