Re: [PATCH v2 bpf-next 1/2] bpf: explicitly define BPF_FUNC_xxx integer values

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii@kernel.org> writes:

> Historically enum bpf_func_id's BPF_FUNC_xxx enumerators relied on
> implicit sequential values being assigned by compiler. This is
> convenient, as new BPF helpers are always added at the very end, but it
> also has its downsides, some of them being:
>
>   - with over 200 helpers now it's very hard to know what's each helper's ID,
>     which is often important to know when working with BPF assembly (e.g.,
>     by dumping raw bpf assembly instructions with llvm-objdump -d
>     command). it's possible to work around this by looking into vmlinux.h,
>     dumping /sys/btf/kernel/vmlinux, looking at libbpf-provided
>     bpf_helper_defs.h, etc. But it always feels like an unnecessary step
>     and one should be able to quickly figure this out from UAPI header.
>
>   - when backporting and cherry-picking only some BPF helpers onto older
>     kernels it's important to be able to skip some enum values for helpers
>     that weren't backported, but preserve absolute integer IDs to keep BPF
>     helper IDs stable so that BPF programs stay portable across upstream
>     and backported kernels.
>
> While neither problem is insurmountable, they come up frequently enough
> and are annoying enough to warrant improving the situation. And for the
> backporting the problem can easily go unnoticed for a while, especially
> if backport is done with people not very familiar with BPF subsystem overall.
>
> Anyways, it's easy to fix this by making sure that __BPF_FUNC_MAPPER
> macro provides explicit helper IDs. Unfortunately that would potentially
> break existing users that use UAPI-exposed __BPF_FUNC_MAPPER and are
> expected to pass macro that accepts only symbolic helper identifier
> (e.g., map_lookup_elem for bpf_map_lookup_elem() helper).
>
> As such, we need to introduce a new macro (___BPF_FUNC_MAPPER) which
> would specify both identifier and integer ID, but in such a way as to
> allow existing __BPF_FUNC_MAPPER be expressed in terms of new
> ___BPF_FUNC_MAPPER macro. And that's what this patch is doing. To avoid
> duplication and allow __BPF_FUNC_MAPPER stay *exactly* the same,
> ___BPF_FUNC_MAPPER accepts arbitrary "context" arguments, which can be
> used to pass any extra macros, arguments, and whatnot. In our case we
> use this to pass original user-provided macro that expects single
> argument and __BPF_FUNC_MAPPER is using it's own three-argument
> __BPF_FUNC_MAPPER_APPLY intermediate macro to impedance-match new and
> old "callback" macros.
>
> Once we resolve this, we use new ___BPF_FUNC_MAPPER to define enum
> bpf_func_id with explicit values. The other users of __BPF_FUNC_MAPPER
> in kernel (namely in kernel/bpf/disasm.c) are kept exactly the same both
> as demonstration that backwards compat works, but also to avoid
> unnecessary code churn.
>
> Note that new ___BPF_FUNC_MAPPER() doesn't forcefully insert comma
> between values, as that might not be appropriate in all possible cases
> where ___BPF_FUNC_MAPPER might be used by users. This doesn't reduce
> usability, as it's trivial to insert that comma inside "callback" macro.
>
> To validate all the manually specified IDs are exactly right, we used
> BTF to compare before and after values:
>
>   $ bpftool btf dump file ~/linux-build/default/vmlinux | rg bpf_func_id -A 211 > after.txt
>   $ git stash # stach UAPI changes
>   $ make -j90
>   ... re-building kernel without UAPI changes ...
>   $ bpftool btf dump file ~/linux-build/default/vmlinux | rg bpf_func_id -A 211 > before.txt
>   $ diff -u before.txt after.txt
>   --- before.txt  2022-10-05 10:48:18.119195916 -0700
>   +++ after.txt   2022-10-05 10:46:49.446615025 -0700
>   @@ -1,4 +1,4 @@
>   -[14576] ENUM 'bpf_func_id' encoding=UNSIGNED size=4 vlen=211
>   +[9560] ENUM 'bpf_func_id' encoding=UNSIGNED size=4 vlen=211
>           'BPF_FUNC_unspec' val=0
>           'BPF_FUNC_map_lookup_elem' val=1
>           'BPF_FUNC_map_update_elem' val=2
>
> As can be seen from diff above, the only thing that changed was resulting BTF
> type ID of ENUM bpf_func_id, not any of the enumerators, their names or integer
> values.
>
> The only other place that needed fixing was scripts/bpf_doc.py used to generate
> man pages and bpf_helper_defs.h header for libbpf and selftests. That script is
> tightly-coupled to exact shape of ___BPF_FUNC_MAPPER macro definition, so had
> to be trivially adapted.
>
> Cc: Quentin Monnet <quentin@isovalent.com>
> Reported-by: Andrea Terzolo <andrea.terzolo@polito.it>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Nice! Thanks for fixing this!

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>