On Wed, Jan 11, 2023 at 2:57 PM Maxim Mikityanskiy <maxtram95@xxxxxxxxx> 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!
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.
iirc they had their own incompatible inline asm :(
It's a bigger issue.
