Re: [PATCH bpf-next v4 0/3] Annotate kfuncs in .BTF_ids section

[Daniel Xu]

Hi Manu,

On Fri, Feb 02, 2024 at 03:09:05PM -0800, Manu Bretelle wrote:
> On Sun, Jan 28, 2024 at 06:24:05PM -0700, Daniel Xu wrote:
> > === Description ===
> > 
> > This is a bpf-treewide change that annotates all kfuncs as such inside
> > .BTF_ids. This annotation eventually allows us to automatically generate
> > kfunc prototypes from bpftool.
> > 
> > We store this metadata inside a yet-unused flags field inside struct
> > btf_id_set8 (thanks Kumar!). pahole will be taught where to look.
> > 
> > More details about the full chain of events are available in commit 3's
> > description.
> > 
> > The accompanying pahole and bpftool changes can be viewed
> > here on these "frozen" branches [0][1].
> > 
> > [0]: https://github.com/danobi/pahole/tree/kfunc_btf-v3-mailed
> > [1]: https://github.com/danobi/linux/tree/kfunc_bpftool-mailed
> 
> 
> I hit a similar issue to [0] on master
> 943b043aeecc ("selftests/bpf: Fix bench runner SIGSEGV")
>  when cross-compiling on x86_64 (LE) to s390x (BE).
> I do have CONFIG_DEBUG_INFO_BTF enable and the issue would not trigger if
> I disabled CONFIG_DEBUG_INFO_BTF (and with the fix mentioned in [0]).
> 
> What seems to happen is that `tools/resolve_btfids` is ran in the context of the
> host endianess and if I printk before the WARN_ON:
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ef380e546952..a9ed7a1a4936 100644
>   --- a/kernel/bpf/btf.c
>   +++ b/kernel/bpf/btf.c
>   @@ -8128,6 +8128,7 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
>            * WARN() for initcall registrations that do not check errors.
>            */
>           if (!(kset->set->flags & BTF_SET8_KFUNCS)) {
>   +        printk("Flag 0x%08X, expected 0x%08X\n", kset->set->flags, BTF_SET8_KFUNCS);
>                   WARN_ON(!kset->owner);
>                   return -EINVAL;
>           }
> 
> the boot logs would show:
>   Flag 0x01000000, expected 0x00000001
> 
> The issue did not happen prior to
> 6f3189f38a3e ("bpf: treewide: Annotate BPF kfuncs in BTF")
> has only 0 was written before.
> 
> It seems [1] will be addressing cross-compilation, but it did not fix it as is
> by just applying on top of master, so probably some of the changes will also need
> to be ported to `tools/include/linux/btf_ids.h`?
> 
> A hacky workaround to cross-compilation I have is to apply:
> 
>   diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
>   index 4b8079f294f6..b706e7ab066f 100644
>   --- a/tools/bpf/resolve_btfids/Makefile
>   +++ b/tools/bpf/resolve_btfids/Makefile
>   @@ -22,10 +22,10 @@ HOST_OVERRIDES := AR="$(HOSTAR)" CC="$(HOSTCC)" LD="$(HOSTLD)" ARCH="$(HOSTARCH)
>                     CROSS_COMPILE="" EXTRA_CFLAGS="$(HOSTCFLAGS)"
>    RM      ?= rm
>   -HOSTCC  ?= gcc
>   -HOSTLD  ?= ld
>   -HOSTAR  ?= ar
>   -CROSS_COMPILE =
>   +HOSTCC  = $(CC)
>   +HOSTLD  = $(LD)
>   +HOSTAR  = $(AR)
>   +#CROSS_COMPILE =
>    OUTPUT ?= $(srctree)/tools/bpf/resolve_btfids/
>   @@ -56,16 +56,16 @@ $(OUTPUT) $(OUTPUT)/libsubcmd $(LIBBPF_OUT):
>    $(SUBCMDOBJ): fixdep FORCE | $(OUTPUT)/libsubcmd
>           $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(SUBCMD_OUT) \
>   -                   DESTDIR=$(SUBCMD_DESTDIR) $(HOST_OVERRIDES) prefix= subdir= \
>   +                   DESTDIR=$(SUBCMD_DESTDIR) prefix= subdir= \
>                       $(abspath $@) install_headers
>    $(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUT)
>           $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(LIBBPF_OUT)    \
>   -                   DESTDIR=$(LIBBPF_DESTDIR) $(HOST_OVERRIDES) prefix= subdir= \
>   +                   DESTDIR=$(LIBBPF_DESTDIR) prefix= subdir= \
>                       $(abspath $@) install_headers
>   -LIBELF_FLAGS := $(shell $(HOSTPKG_CONFIG) libelf --cflags 2>/dev/null)
>   -LIBELF_LIBS  := $(shell $(HOSTPKG_CONFIG) libelf --libs 2>/dev/null || echo -lelf)
>   +LIBELF_FLAGS := $(shell $(PKG_CONFIG) libelf --cflags 2>/dev/null)
>   +LIBELF_LIBS  := $(shell $(PKG_CONFIG) libelf --libs 2>/dev/null || echo -lelf)
>    HOSTCFLAGS_resolve_btfids += -g \
>              -I$(srctree)/tools/include \
>   @@ -84,7 +84,7 @@ $(BINARY_IN): fixdep FORCE prepare | $(OUTPUT)
>    $(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN)
>           $(call msg,LINK,$@)
>   -       $(Q)$(HOSTCC) $(BINARY_IN) $(KBUILD_HOSTLDFLAGS) -o $@ $(BPFOBJ) $(SUBCMDOBJ) $(LIBS)
>   +       $(Q)$(CC) $(BINARY_IN) $(KBUILD_HOSTLDFLAGS) -o $@ $(BPFOBJ) $(SUBCMDOBJ) $(LIBS)
>    clean_objects := $(wildcard $(OUTPUT)/*.o                \
>                                $(OUTPUT)/.*.o.cmd           \
>   diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>   index a38a3001527c..5cd193c04448 100644
>   --- a/tools/testing/selftests/bpf/Makefile
>   +++ b/tools/testing/selftests/bpf/Makefile
>   @@ -171,7 +171,7 @@ INCLUDE_DIR := $(SCRATCH_DIR)/include
>    BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
>    ifneq ($(CROSS_COMPILE),)
>    HOST_BUILD_DIR         := $(BUILD_DIR)/host
>   -HOST_SCRATCH_DIR       := $(OUTPUT)/host-tools
>   +HOST_SCRATCH_DIR       := $(SCRATCH_DIR)
>    HOST_INCLUDE_DIR       := $(HOST_SCRATCH_DIR)/include
>    else
>    HOST_BUILD_DIR         := $(BUILD_DIR)
> 
> This causes `resolve_btfids` to be compiled in the target endianess and gets
> magically run provided that the hosts has `qemu-s390x-static` and a functional
> binfmt_misc [2] on the host, but having this using host architecture per [1]
> is likely better.

This is kinda surprising to me. I don't recall seeing any code inside
resolve_btfids that touches the set8 flags field -- only the ids in the
flexible array member. Would be interested to see what the value of the
set8 flags field is before resolve_btfids is run.

I'm a bit busy until Sunday afternoon but I'll try to take a look around
then. Might be a good opportunity to play with poke [0].

Thanks,
Daniel


[0]: http://www.jemarch.net/poke