mbox series

[bpf-next,v4,0/3] Annotate kfuncs in .BTF_ids section

Message ID cover.1706491398.git.dxu@dxuuu.xyz
Headers show
Series Annotate kfuncs in .BTF_ids section | expand

Message

Daniel Xu Jan. 29, 2024, 1:24 a.m. UTC 
=== 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

=== Changelog ===

Changes from v3:
* Rebase to bpf-next and add missing annotation on new kfunc

Changes from v2:
* Only WARN() for vmlinux kfuncs

Changes from v1:
* Move WARN_ON() up a call level
* Also return error when kfunc set is not properly tagged
* Use BTF_KFUNCS_START/END instead of flags
* Rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS

Daniel Xu (3):
  bpf: btf: Support flags for BTF_SET8 sets
  bpf: btf: Add BTF_KFUNCS_START/END macro pair
  bpf: treewide: Annotate BPF kfuncs in BTF

 Documentation/bpf/kfuncs.rst                  |  8 +++----
 drivers/hid/bpf/hid_bpf_dispatch.c            |  8 +++----
 fs/verity/measure.c                           |  4 ++--
 include/linux/btf_ids.h                       | 21 +++++++++++++++----
 kernel/bpf/btf.c                              |  8 +++++++
 kernel/bpf/cpumask.c                          |  4 ++--
 kernel/bpf/helpers.c                          |  8 +++----
 kernel/bpf/map_iter.c                         |  4 ++--
 kernel/cgroup/rstat.c                         |  4 ++--
 kernel/trace/bpf_trace.c                      |  8 +++----
 net/bpf/test_run.c                            |  8 +++----
 net/core/filter.c                             | 20 +++++++++---------
 net/core/xdp.c                                |  4 ++--
 net/ipv4/bpf_tcp_ca.c                         |  4 ++--
 net/ipv4/fou_bpf.c                            |  4 ++--
 net/ipv4/tcp_bbr.c                            |  4 ++--
 net/ipv4/tcp_cubic.c                          |  4 ++--
 net/ipv4/tcp_dctcp.c                          |  4 ++--
 net/netfilter/nf_conntrack_bpf.c              |  4 ++--
 net/netfilter/nf_nat_bpf.c                    |  4 ++--
 net/xfrm/xfrm_interface_bpf.c                 |  4 ++--
 net/xfrm/xfrm_state_bpf.c                     |  4 ++--
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  8 +++----
 23 files changed, 87 insertions(+), 66 deletions(-)

Comments

Jiri Olsa Jan. 31, 2024, 9:47 a.m. UTC | #1 
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
> 
> === Changelog ===
> 
> Changes from v3:
> * Rebase to bpf-next and add missing annotation on new kfunc

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> 
> Changes from v2:
> * Only WARN() for vmlinux kfuncs
> 
> Changes from v1:
> * Move WARN_ON() up a call level
> * Also return error when kfunc set is not properly tagged
> * Use BTF_KFUNCS_START/END instead of flags
> * Rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS
> 
> Daniel Xu (3):
>   bpf: btf: Support flags for BTF_SET8 sets
>   bpf: btf: Add BTF_KFUNCS_START/END macro pair
>   bpf: treewide: Annotate BPF kfuncs in BTF
> 
>  Documentation/bpf/kfuncs.rst                  |  8 +++----
>  drivers/hid/bpf/hid_bpf_dispatch.c            |  8 +++----
>  fs/verity/measure.c                           |  4 ++--
>  include/linux/btf_ids.h                       | 21 +++++++++++++++----
>  kernel/bpf/btf.c                              |  8 +++++++
>  kernel/bpf/cpumask.c                          |  4 ++--
>  kernel/bpf/helpers.c                          |  8 +++----
>  kernel/bpf/map_iter.c                         |  4 ++--
>  kernel/cgroup/rstat.c                         |  4 ++--
>  kernel/trace/bpf_trace.c                      |  8 +++----
>  net/bpf/test_run.c                            |  8 +++----
>  net/core/filter.c                             | 20 +++++++++---------
>  net/core/xdp.c                                |  4 ++--
>  net/ipv4/bpf_tcp_ca.c                         |  4 ++--
>  net/ipv4/fou_bpf.c                            |  4 ++--
>  net/ipv4/tcp_bbr.c                            |  4 ++--
>  net/ipv4/tcp_cubic.c                          |  4 ++--
>  net/ipv4/tcp_dctcp.c                          |  4 ++--
>  net/netfilter/nf_conntrack_bpf.c              |  4 ++--
>  net/netfilter/nf_nat_bpf.c                    |  4 ++--
>  net/xfrm/xfrm_interface_bpf.c                 |  4 ++--
>  net/xfrm/xfrm_state_bpf.c                     |  4 ++--
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  8 +++----
>  23 files changed, 87 insertions(+), 66 deletions(-)
> 
> -- 
> 2.42.1
>
patchwork-bot+netdevbpf@kernel.org Jan. 31, 2024, 8:20 p.m. UTC | #2 
Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Sun, 28 Jan 2024 18:24:05 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v4,1/3] bpf: btf: Support flags for BTF_SET8 sets
    https://git.kernel.org/bpf/bpf-next/c/79b47344bbc5
  - [bpf-next,v4,2/3] bpf: btf: Add BTF_KFUNCS_START/END macro pair
    https://git.kernel.org/bpf/bpf-next/c/2747e0ee57c2
  - [bpf-next,v4,3/3] bpf: treewide: Annotate BPF kfuncs in BTF
    https://git.kernel.org/bpf/bpf-next/c/6e7769e6419f

You are awesome, thank you!
Manu Bretelle Feb. 2, 2024, 11:09 p.m. UTC | #3 
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.

Here are steps to reproduce the issue on Ubuntu 23.10 and assuming
danobi/vmtest [3] is installed:

  XPLATFORM="s390x"
  XARCH="s390"
  # Set up repo for s390x
  cat <<EOF >> /etc/apt/sources.list.d/s390x.list
  deb [arch=s390x] http://ports.ubuntu.com/ubuntu-ports  mantic main restricted
  deb [arch=s390x] http://ports.ubuntu.com/ubuntu-ports  mantic-updates main restricted
  EOF
  sudo dpkg --add-architecture s390x
  
  apt install qemu-system-s390x qemu-user-static g{cc,++}-"${XARCH}-linux-gnu" {libelf-dev,libssl-dev,pkgconf}:s390x
  
  KBUILD_OUTPUT_DIR="/tmp/kbuild-${XPLATFORM}"
  mkdir "${KBUILD_OUTPUT_DIR}"
  cat tools/testing/selftests/bpf/config{,.vm,.${XPLATFORM}} > ${KBUILD_OUTPUT_DIR}/.config
  
  make ARCH="${XARCH}" CROSS_COMPILE="${XPLATFORM}-linux-gnu-" O="${KBUILD_OUTPUT_DIR}"  -j$((4 * $(nproc))) olddefconfig
  make ARCH="${XARCH}" CROSS_COMPILE="${XPLATFORM}-linux-gnu-" O="${KBUILD_OUTPUT_DIR}"  -j$((4 * $(nproc))) all
  
  # No need for a s390x ubuntu 23.10 rootfs, we only care about booting the kernel
  vmtest -k "${KBUILD_OUTPUT_DIR}/arch/s390/boot/bzImage" -a s390x "uname -m" | cat


For the chroot route, see [4].

[0] https://lore.kernel.org/linux-kernel/20240201155339.2b5936be@canb.auug.org.au/T/
[1] https://lore.kernel.org/bpf/cover.1706717857.git.vmalik@redhat.com/
[2] https://en.wikipedia.org/wiki/Binfmt_misc
[3] https://github.com/danobi/vmtest
[4] https://chantra.github.io/bpfcitools/bpf-cross-compile.html

Manu

> 
> === Changelog ===
> 
> Changes from v3:
> * Rebase to bpf-next and add missing annotation on new kfunc
> 
> Changes from v2:
> * Only WARN() for vmlinux kfuncs
> 
> Changes from v1:
> * Move WARN_ON() up a call level
> * Also return error when kfunc set is not properly tagged
> * Use BTF_KFUNCS_START/END instead of flags
> * Rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS
> 
> Daniel Xu (3):
>   bpf: btf: Support flags for BTF_SET8 sets
>   bpf: btf: Add BTF_KFUNCS_START/END macro pair
>   bpf: treewide: Annotate BPF kfuncs in BTF
> 
>  Documentation/bpf/kfuncs.rst                  |  8 +++----
>  drivers/hid/bpf/hid_bpf_dispatch.c            |  8 +++----
>  fs/verity/measure.c                           |  4 ++--
>  include/linux/btf_ids.h                       | 21 +++++++++++++++----
>  kernel/bpf/btf.c                              |  8 +++++++
>  kernel/bpf/cpumask.c                          |  4 ++--
>  kernel/bpf/helpers.c                          |  8 +++----
>  kernel/bpf/map_iter.c                         |  4 ++--
>  kernel/cgroup/rstat.c                         |  4 ++--
>  kernel/trace/bpf_trace.c                      |  8 +++----
>  net/bpf/test_run.c                            |  8 +++----
>  net/core/filter.c                             | 20 +++++++++---------
>  net/core/xdp.c                                |  4 ++--
>  net/ipv4/bpf_tcp_ca.c                         |  4 ++--
>  net/ipv4/fou_bpf.c                            |  4 ++--
>  net/ipv4/tcp_bbr.c                            |  4 ++--
>  net/ipv4/tcp_cubic.c                          |  4 ++--
>  net/ipv4/tcp_dctcp.c                          |  4 ++--
>  net/netfilter/nf_conntrack_bpf.c              |  4 ++--
>  net/netfilter/nf_nat_bpf.c                    |  4 ++--
>  net/xfrm/xfrm_interface_bpf.c                 |  4 ++--
>  net/xfrm/xfrm_state_bpf.c                     |  4 ++--
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  8 +++----
>  23 files changed, 87 insertions(+), 66 deletions(-)
> 
> -- 
> 2.42.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Daniel Xu Feb. 3, 2024, 1:34 a.m. UTC | #4 
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
Jiri Olsa Feb. 3, 2024, 2:40 p.m. UTC | #5 
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`?

the fix in [1] is fixing flags in set8's pairs, but not the global flags

it looks like Viktor's fix should now also swap that as well? like in the
change below on top of Viktor's changes (untested)

jirka


---
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index d01603ef6283..c44d57fec390 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -706,6 +706,8 @@ static int sets_patch(struct object *obj)
 			 * correctly translate everything.
 			 */
 			if (need_bswap) {
+				set8->flags = bswap_32(set8->flags);
+
 				for (i = 0; i < cnt; i++) {
 					set8->pairs[i].flags =
 						bswap_32(set8->pairs[i].flags);
Manu Bretelle Feb. 3, 2024, 6:45 p.m. UTC | #6 
On Sat, Feb 03, 2024 at 03:40:24PM +0100, Jiri Olsa wrote:
> 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`?
> 
> the fix in [1] is fixing flags in set8's pairs, but not the global flags
> 
> it looks like Viktor's fix should now also swap that as well? like in the
> change below on top of Viktor's changes (untested)
> 
> jirka
> 
> 
> ---
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index d01603ef6283..c44d57fec390 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -706,6 +706,8 @@ static int sets_patch(struct object *obj)
>  			 * correctly translate everything.
>  			 */
>  			if (need_bswap) {
> +				set8->flags = bswap_32(set8->flags);
> +
>  				for (i = 0; i < cnt; i++) {
>  					set8->pairs[i].flags =
>  						bswap_32(set8->pairs[i].flags);
> 

That should work. Here are a few tests I ran:

$ md5sum /tmp/kbuild-s390x/vmlinux.*
eb658e51e089f3c5b2c8909a29dc9997  /tmp/kbuild-s390x/vmlinux.a
# plain vmlinux before running resolv_btfids (all 0s)
ea907cd46a1a73b8276b5f2a82af00ca  /tmp/kbuild-s390x/vmlinux.before_resolv
# x86_64 resolv_btfids on master without Viktor's patch
980a40c3a3ff563d1c2d1ebdd5071a23  /tmp/kbuild-s390x/vmlinux.resolv_native
# x86_64 resolv_btfids on master with Viktor's patch
b986d19e242719ebea41c578235da662  /tmp/kbuild-s390x/vmlinux.resolv_native_patch_viktor
# x86_64 resolv_btfids on master with Viktor's patch and your suggested patch
4edd8752ff01129945bd442689b1927b  /tmp/kbuild-s390x/vmlinux.resolv_native_patch_viktor_patched
# s390x resolv_btfids run with qemu-s390x-static
4edd8752ff01129945bd442689b1927b  /tmp/kbuild-s390x/vmlinux.resolv_s390x


and some hexdiff of those binaries:


# difference between master's native build and s390x build.... has byte swapping for set8 and others
diff -ruN <(xxd /tmp/kbuild-s390x/vmlinux.resolv_s390x) <(xxd /tmp/kbuild-s390x/vmlinux.resolv_native) > diff_s390x_native.diff
https://gist.github.com/chantra/c3d58637a08a6f7340953dc155bb18cc

# difference betwee Viktor's version and  s390x build.... squinting my eyes I only see the global set8 is missing
diff -ruN <(xxd /tmp/kbuild-s390x/vmlinux.resolv_s390x) <(xxd /tmp/kbuild-s390x/vmlinux.resolv_native_patch_viktor) > diff_s390x_native_viktor.diff
https://gist.github.com/chantra/61cfff02b456ae72d3c0161ce1897097

Have a good weekend all!

Manu