2022-10-06 05:24:51 GMT+0100 ~ Andrii Nakryiko <andrii@xxxxxxxxxx>
> 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.
I've also been using bpftool :)
$ bpftool -j feature list_builtins helpers | \
jq 'index("bpf_get_current_task_btf")'
158
(But agreed, UAPI header would be the best place to look at.)
>
> - 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@xxxxxxxxxxxxx>
> Reported-by: Andrea Terzolo <andrea.terzolo@xxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
Looks good to me. The legacy __BPF_FUNC_MAPPER() is also still working
well in bpftool.
Reviewed-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
