Re: [PATCH v2 bpf-next 01/13] bpf: Support multiple arg regs w/ ref_obj_id for kfuncs

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Dave Marchevsky <davemarchevsky@meta.com>
Cc: David Vernet <void@manifault.com>,
	Dave Marchevsky <davemarchevsky@fb.com>,
	bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Kernel Team <kernel-team@fb.com>,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH v2 bpf-next 01/13] bpf: Support multiple arg regs w/ ref_obj_id for kfuncs
Date: Tue, 17 Jan 2023 09:36:46 -0800	[thread overview]
Message-ID: <CAADnVQJLnLpLjWgWqDPc96Jgk+OHZTeNux+iiyFjCcy+mQK5HA@mail.gmail.com> (raw)
In-Reply-To: <afcae0f4-97a0-b06e-0a4e-7955ca7dbc7c@meta.com>

On Tue, Jan 17, 2023 at 9:26 AM Dave Marchevsky <davemarchevsky@meta.com> wrote:
>
> In another thread you also mentioned that hypothetical "kfunc writer" persona
> shouldn't have to understand kfunc flags in order to add their simple kfunc, and
> I think your comments here are also presupposing a "kfunc writer" persona that
> doesn't look at the verifier. Having such a person able to add kfuncs without
> understanding the verifier is a good goal, but doesn't reflect current
> reality when the kfunc needs to have any special semantics.

agree on that goal.

> Regardless, I'd expect that anyone adding further new-style Graph
> datastructures, old-style maps, or new datastructures unrelated to either,
> will be closer to "verifier expert" than "random person adding a few kfuncs".

also agree, since it's a reality right now.

> >> Here we're paving the way for graph (aka new gen data structs)
> >> and so far not only kfuncs, but their arg types have to have
> >> special handling inside the verifier.
> >> There is not much yet to generalize and expose as generic KF_
> >> flag or as a name suffix.
> >> Therefore I think it's more appropriate to implement them
> >> with minimal verifier changes and minimal complexity.
> >
> > Agreed
> >
>
> 'Generalize' was addressed in Patch 2's thread.
>
> >> There is no 3rd graph algorithm on the horizon after link list
> >> and rbtree. Instead there is a big todo list for
> >> 'multi owner graph node' and 'bpf_refcount_t'.
> >
> > In this case my point in [0] of the only option for generalizing being
> > to have something like KF_GRAPH_INSERT / KF_GRAPH_REMOVE is just not the
> > way forward (which I also said was my opinion when I pointed it out as
> > an option). Let's just special-case these kfuncs. There's already a
> > precedence for doing that in the verifier anyways. Minimal complexity,
> > minimal API changes. It's a win-win.
> >
> > [0]: https://lore.kernel.org/all/Y63GLqZil9l1NzY4@maniforge.lan/
> >
>
> There's certainly precedent for adding special-case "kfunc_id == KFUNC_whatever"
> all over the verifier. It's a bad precedent, though, for reasons discussed in
> [0].
>
> To specifically address your points here, I don't buy the argument that
> special-casing based on func id is "minimal complexity, minimal API changes".
> Re: 'complexity': the logic implementing the complicated semantic will be
> added regardless, it just won't have a name that's easily referenced in docs
> and mailing list discussions.
>
> Similarly, re: 'API changes': if by 'API' here you mean "API that's exposed
> to folks adding kfuncs" - see my comments about "kfunc writer" persona above.
> We can think of the verifier itself as an API too - with a single bpf_check
> function. That API's behavior is indeed changed here, regardless of whether
> the added semantics are gated by a kfunc flag or special-case checks. I don't
> think that hiding complexity behind special-case checks when there could be
> a named flag simplifies anything. The complexity is added regardless, question
> is how many breadcrumbs and pointers we want to leave for folks trying to make
> sense of it in the future.
>
>   [0]: https://lore.kernel.org/bpf/9763aed7-0284-e400-b4dc-ed01718d8e1e@meta.com/

I could have agreed to this as well if I didn't go and remove
all the new KF_*OWN* flags.
imo the resulting diff of mine vs your initial patch is easier to
follow and reason about.
So for this case "kfunc_id == KFUNC_whatever" is cleaner.
It doesn't mean that it will be the case in other situations.