Re: [PATCH bpf-next v10 11/24] bpf: Rewrite kfunc argument handling

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Dave Marchevsky <davemarchevsky@meta.com>
Subject: Re: [PATCH bpf-next v10 11/24] bpf: Rewrite kfunc argument handling
Date: Sat, 19 Nov 2022 00:30:12 +0530	[thread overview]
Message-ID: <20221118190012.cfhavzxw3ssil6nh@apollo> (raw)
In-Reply-To: <CAADnVQLxkVKggTwXQJN48yvi4mh9o8qGoF4M4VGifHzygfq+cw@mail.gmail.com>

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@gmail.com> 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.