Re: bpf helpers freeze. Was: [PATCH v2 bpf-next 0/6] Dynptr convenience helpers

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

> On Wed, Jan 11, 2023 at 2:57 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
>>
>> On Tue, Jan 03, 2023 at 03:51:07PM -0800, Alexei Starovoitov wrote:
>> > On Tue, Jan 03, 2023 at 12:43:58PM +0100, Daniel Borkmann wrote:
>> > > Discoverability plus being able to know semantics from a user PoV to figure out when
>> > > workarounds for older/newer kernels are required to be able to support both kernels.
>> >
>> > Sounds like your concern is that there could be a kfunc that changed it semantics,
>> > but kept exact same name and arguments? Yeah. That would be bad, but we should prevent
>> > such patches from landing. It's up to us to define sane and user
>> > friendly deprecation of kfuncs.
>>
>> I would advocate for adding versioning to BPF API (be it helpers or
>> "stable" kfuncs). Right now we have two extremes: helpers that can't be
>> changed/fixed/deprecated ever, and kfuncs that can be changed at any
>> time, so the end users can't be sure new kernel won't break their stuff.
>> Detecting and fixing the breakage can also be tricky: end users have to
>> write different probes on a case-by-case basis, and sometimes it's not
>> just a matter of checking the number of function parameters or presence
>> of some definition (such difficulties happen when backporting drivers to
>> older kernels, so I assume it may be an issue for BPF programs as well).
>>
>> Let's say we add a version number to the kernel, and the BPF program
>> also has an API version number it's compiled for. Whenever something
>> changes in the stable API on the kernel side, the version number is
>> increased. At the same time, compatibility on the kernel side is
>> preserved for some reasonable period of time (2 years, 5 years,
>> whatever), which means that if the kernel loads a BPF program with an
>> older version number, and that version is within the supported period of
>> time, the kernel will behave in the old way, i.e. verify the old
>> signature of a function, preserve the old behavior, etc.
>
> Right. I think somebody proposed a version scheme for kfuncs already.
> There were so many replies I've lost track.
> But yes it's definitely on the table and
> we should consider it.
> Something like libbpf.map
> We can declare which stable features are supported in which "version".
>
>> This approach has the following upsides:
>>
>> 1. End users can stop worrying that some function changes unexpectedly,
>> and they can have a smoother migration plan.
>>
>> 2. Clear deprecation schedule.
>>
>> 3. Easy way to probe for needed functionality, it's just a matter of
>> comparing numbers: the BPF program loader checks that the kernel is new
>> enough, and the kernel checks that the BPF program's API is not too old.
>>
>> 4. Kernel maintainers will have a deprecation strategy.
>
> +1
>
>> Cons:
>>
>> 1. Arguably a maintainance burden to preserve compatibility on the
>> kernel side, but I would say it's a balance between helpers (which are
>> maintainance burden forever) and kfuncs (which can be changed in every
>> kernel version without keeping any compatibility). "Kfunc that changed
>> its semantics is bad, we should prevent such patches" are just words,
>> but if the developer needs to keep both versions for a while, it will
>> serve as a calm-down mechanism to prevent changes that aren't really
>> necessary. At the same time, the dead code will stop accumulating,
>> because it can be removed according to the schedule.
>
> That sounds like 'pro' instead of 'con' to me :)
>
>> 2. Having a single version number complicates backporting features to
>> older kernels, it would require backporting all previous features
>> chronologically, even if there is no direct dependency. Having multiple
>> version numbers (per feature) is cumbersome for the BPF program to
>> declare. However, this issue is not new, it's already the case for BPF
>> helpers (you can't backport new helpers skipping some other, because the
>> numbers in the list must match).
>
> yeah. I recall amazon linux or something else backported
> helpers out of order and that screwed up bpf progs.
> That was the reason we added numbers to the FN macro in uapi/bpf.h
> That will hopefully prevent such mistakes.
>
> But practically speaking...
> The distro that does out-of-order backporting and skips
> certain helpers is saying: I'm defining my own kABI equivalent
> for bpf progs.
> In that sense there is zero difference between helpers and kfuncs
> from distro point of view and from point of view of their customers.
> Both helpers and kfuncs are neither stable nor unstable.
>
> This discussion is only about pros and cons of the upstream kernel
> and bpf progs that consume upstream kernel.
>
> If we include hyperscalers in the discussion then all
> helpers and all kfuncs immediately become stable from
> point of view of their engineers.
> Big datacenters can maintain kernels with whatever helpers
> and kfuncs they need.
>
>>
>> The above description intentionally doesn't specify whether it should be
>> applied to helpers or kfuncs, because it's a universal concept, about
>> which I would like to hear opinions about versioning without bias to
>> helpers or kfuncs.
>>
>> Regarding freezing helpers, I think there should be a solution for
>> deprecating obsolete stuff. There are historical examples of removing
>> things from UAPI: removing i386 support, ipchains, devfs, IrDA
>> subsystem, even a few architectures [1]. If we apply the versioning
>> approach to helpers, we can make long-waiting incompatible changes in
>> v1, keeping the current set of helpers as v0, used for programs that
>> don't declare a version. Eventually (in 5 years, in 10 years, whatever
>> sounds reasonable) we can drop v0 and remove the support for unversioned
>> BPF programs altogether, similar to how other big things were removed
>> from the kernel. Does it sound feasible?
>
> Not to me. Breaking uapi in whichever way with whatever excuse
> is not on the table.
> We've documented our rules long ago:
>
> Q: Does BPF have a stable ABI?
> ------------------------------
> A: YES. BPF instructions, arguments to BPF programs, set of helper
> functions and their arguments, recognized return codes are all part
> of ABI.
>
>> > "Proper BPF helper" model is broken.
>> > static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>> >
>> > is a hack that works only when compiler optimizes the code.
>>
>> What if we replace codegen for helpers, so that it becomes something
>> like this?
>>
>> static inline void *bpf_map_lookup_elem(void *map, const void *key)
>> {
>>         // pseudocode alert!
>>         asm("call 1" : : "r1"(map), "r2"(key));
>> }
>>
>> I.e. can we just throw in some inline BPF assembly that prepares
>> registers and invokes a call instruction with the helper number? That
>> should be portable across clang and gcc, allowing to stop relying on
>> optimizations.
>
> Great idea!

+1

> It needs "=r" to capture R0 into the 'ret' variable and then it should work.
> clang may have issues with such asm, but should be fixable.
> gcc is less clear.

That inline assembly should work with GCC as it is now.  Both compilers
use the same syntax for the `call' instruction.

> iirc they had their own incompatible inline asm :(
> It's a bigger issue.

We are taking care of that, by adding support to the GNU assembler to
also understand the pseudo-C syntax used by llvm.  This covers both .s
files specified in the compilation line, and inline asm statements.

Should be ready soon.