On Wed, Oct 5, 2022 at 9:25 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote: > > 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@xxxxxxxxxxxxx> > Reported-by: Andrea Terzolo <andrea.terzolo@xxxxxxxxx> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > --- > include/uapi/linux/bpf.h | 432 +++++++++++++++++---------------- > scripts/bpf_doc.py | 19 +- > tools/include/uapi/linux/bpf.h | 432 +++++++++++++++++---------------- > 3 files changed, 447 insertions(+), 436 deletions(-) > I should have mentioned that patch #1 hasn't changed since v1 at all. All the bpf_doc.py adjustments went into patch #2, as I wanted to keep patch #1 with as minimal amount of Python changes as possible to make it work with the upstream kernel. Patch #2 just further improves bpf_doc.py. [...]
