Re: [Buildroot] [PATCH] package/libbpf: Don't remove bpf.h on host

[Lang Daniel via buildroot <buildroot@buildroot.org>]

Hi Arnout, Francis,

> Hi Daniel, Francis,
> 
> On 07/02/2023 21:10, Francis Laniel wrote:
>> Hi.
>> 
>> Le lundi 28 novembre 2022, 16:06:20 CET Lang Daniel a écrit :
>>> libbpf >1.0.0 defines libbpf_bpf_link_type_str(enum bpf_link_type) in
>>> src/libbpf.h, which is included by host-pahole.
>>> bpf_link_type is defined in linux/bpf.h, therefore the comment stating
>>> that pahole doesn't need bpf.h is no longer valid.
>>>
>>> Fixes:
>>> -
>>> http://autobuild.buildroot.net/results/d126a4b6eca786402dc362c86f8df3addec3
>>> d217/
>>>
>>> Signed-off-by: Daniel Lang <d.lang@abatec.at>
>>> ---
>>> I wasn't able to reproduce the mentioned compile error in the kernel.
>>> The mentioned file (tools/lib/bpf/strset.c) shouldn't be compiled when
>>> compiling the kernel as it would recompile libbpf.
>>> ---
>>>   package/libbpf/libbpf.mk | 17 +----------------
>>>   1 file changed, 1 insertion(+), 16 deletions(-)
>>>
>>> diff --git a/package/libbpf/libbpf.mk b/package/libbpf/libbpf.mk
>>> index 820f1dc4bf..0381fd833a 100644
>>> --- a/package/libbpf/libbpf.mk
>>> +++ b/package/libbpf/libbpf.mk
>>> @@ -39,26 +39,11 @@ define LIBBPF_INSTALL_TARGET_CMDS
>>>   		-C $(@D)/src install DESTDIR=$(TARGET_DIR)
>>>   endef
>>>
>>> -# We need to install_uapi_headers so we have btf.h to compile
>>> -# host-pahole.
>>> -# Nonetheless, this target adds bpf.h which generates a conflict when
>>> -# building the kernel:
>>> -# In file included from libbpf_internal.h:17:0, from strset.c:9:
>>> -# relo_core.h:10:6: error: nested redefinition of 'enum bpf_core_relo_kind'
>>> -# enum bpf_core_relo_kind {
>>> -# ^~~~~~~~~~~~~~~~~~
>>> -# relo_core.h:10:6: error: redeclaration of 'enum bpf_core_relo_kind'
>>> -# In file included from libbpf_legacy.h:13:0,
>>> -# 		from libbpf_internal.h:16,
>>> -# 		from strset.c:9:
>>> -# /home/francis/buildroot/output/host/include/linux/bpf.h:6497:6: note:
>>> originally defined here -# enum bpf_core_relo_kind {
>>> -# So, better to remove remove it now since we do not need it to build
>>> +# We need to install_uapi_headers so we have bpf.h and btf.h to compile
>>>   # host-pahole, the only user of host-libbpf.
>>>   define HOST_LIBBPF_INSTALL_CMDS
>>>   	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) \
>>>   		-C $(@D)/src install install_uapi_headers DESTDIR=$(HOST_DIR)
>>> -	rm $(HOST_DIR)/include/linux/bpf.h
>>>   endef
>>>
>>>   $(eval $(generic-package))
>> 
>> Thank you for this patch and sorry for my very late reply.
>> Sadly, I do not think the patch addresses the underlying issue.
>> Indeed, this is not pahole which does not need this file, as pahole relies on
>> <linux/bpf.h> to be built.
>> So, the whole point of this comment and the rm is to avoid a problem when
>> building the kernel when pahole was built before.
> 
>   The current solution, however, also doesn't work, which is why Franci's patch 
> [1] is needed to fix it again.
> 
>   However, we believe this is not the proper fix. The real issue is the following:
> 
> - libbpf installs headers which are possibly incompatible with the kernel's 
> headers. Normally this is not a problem, because you have either the kernel's or 
> libbpf's headers installed, not both.
> 
> - However, when we install libbpf headers in $(HOST_DIR)/include, the kernel 
> build itself picks up those headers instead of its own internal headers. That is 
> why bpf.h is removed here to begin with. If bpf.h is not removed (and the kernel 
> headers are indeed incompatible with the libbpf ones), then the kernel build 
> will fail.
> 
> - There is however no reason why the kernel should pick up $(HOST_DIR)/include 
> instead of its own internal headers. Normally it doesn't do that, because it 
> supplies -I options to its own headers, and these have precedence over the 
> system included /usr/include etc. However, we force HOSTCC="$(HOSTCC) 
> $(HOST_CFLAGS)" and HOST_CFLAGS has -I$(HOST_DIR)/include. This one will come 
> before all of the ones that the kernel adds and will have precedence.
> 
> Therefore, we believe that the solution is to use -isystem instead of -I in the 
> HOSTCC setting of the kernel. This is indeed what is already done for uboot:
> 
> 	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem 
> /,$(HOST_CFLAGS)))" \
> 
> Note that there is a risk that this by itself goes wrong as well. We tried at 
> some point to use -isystem instead of -I in HOST_CFLAGS:
> 
> commit 6f8162cf8c1abef7e0a4771fe0d6b26a28f5c2b6
> Author: David Raeman <draeman@bbn.com>
> Date:   Mon Jul 25 21:52:26 2016
> 
>      package/Makefile.in should grab HOST_DIR headers using -isystem instead of -I.
> 
> 
> but this was reverted a copule of days later:
> 
> commit 255b6f80d395ef048f46cfcf75dba690c56af657
> Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Date:   Sat Jul 30 18:10:18 2016
> 
>      Revert "package/Makefile.in should grab HOST_DIR headers using -isystem 
> instead of -I."
> 
> (Unfortunately, the commit has no further explanation of what went wrong).
> 
>   So, could you (Daniel and Francis) try to apply this patch and replace -I with 
> -isystem like is done for U-Boot, and do a bunch of kernel builds to see if it 
> breaks anything?

I didn't apply the patch for package/Makefile.in as that would change the behavour
for all packages. I did however copy the logic used in the U-Boot.

-HOSTCC="$(HOSTCC) $(HOST_CFLAGS) $(HOST_LDFLAGS)" \
+HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS))) $(HOST_LDFLAGS)" \

And applied this patch to keep bpf.h

I tested the following config:

BR2_aarch64=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y
BR2_TOOLCHAIN_EXTERNAL_BOOTLIN=y
BR2_TOOLCHAIN_EXTERNAL_BOOTLIN_AARCH64_GLIBC_STABLE=y
BR2_LINUX_KERNEL=y
BR2_LINUX_KERNEL_CUSTOM_VERSION=y
BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="{VERSION}"
BR2_LINUX_KERNEL_USE_ARCH_DEFAULT_CONFIG=y
BR2_LINUX_KERNEL_CONFIG_FRAGMENT_FILES="pahole-kernel.config"
BR2_LINUX_KERNEL_NEEDS_HOST_OPENSSL=y
BR2_LINUX_KERNEL_NEEDS_HOST_PAHOLE=y

and with a fragment for linux:

CONFIG_BPF_SYSCALL=y
CONFIG_BPF_UNPRIV_DEFAULT_OFF=n
CONFIG_DEBUG_INFO_REDUCED=n
CONFIG_DEBUG_INFO_COMPRESSED=n
CONFIG_DEBUG_INFO_BTF=y
CONFIG_SYSTEM_TRUSTED_KEYRING=y

where VERSION is one of:
5.2.21 5.3.18 5.4.231 5.5.19 5.6.19 5.7.19 5.8.18 5.9.16 5.10.167 5.11.22
5.12.19 5.13.19 5.14.21 5.15.93 5.16.20 5.17.15 5.18.19 5.19.17 6.0.19 6.1.11

Version 5.2, as far as I could work it out, is the version that introduced
the pahole dependency when CONFIG_DEBUG_INFO_BTF is set.

None-LTS versions after 5.10 and before 6.0 fail with:

  LD      vmlinux.o
  MODPOST vmlinux.symvers
  MODINFO modules.builtin.modinfo
  GEN     modules.builtin
  LD      .tmp_vmlinux.btf
  BTF     .btf.vmlinux.bin.o
  LD      .tmp_vmlinux.kallsyms1
  KSYMS   .tmp_vmlinux.kallsyms1.S
  AS      .tmp_vmlinux.kallsyms1.S
  LD      .tmp_vmlinux.kallsyms2
  KSYMS   .tmp_vmlinux.kallsyms2.S
  AS      .tmp_vmlinux.kallsyms2.S
  LD      vmlinux
  BTFIDS  vmlinux
FAILED: load BTF from vmlinux: Invalid argument
make[2]: *** [Makefile:1177: vmlinux] Error 255
make[1]: *** [package/pkg-generic.mk:293: /home/d.lang/ws/other/buildroot/output/build/linux-5.11.22/.stamp_built] Error 2
make: *** [Makefile:82: _all] Error 2

But that should be unrelated to the change in package/libbpf and HOSTCC.
I will try to work out the reason behind that error.

Should I create a patch to change HOSTCC or is additional testing required?

> 
>   For now, I've marked this patch as Changes Requested (it can only be applied 
> if the bpf.h problem is solved in some other way), and marked Francis's patch 
> [1] as Superseded.
> 
>   Regards,
>   Arnout
> 
> 
> [1] 
> https://patchwork.ozlabs.org/project/buildroot/patch/20220610165441.84812-2-flaniel@linux.microsoft.com/
> 
> 
>> 
>> Sadly, I tested your patch and I hit the problem described in the comment
>> regarding redefinition of bpf_core_relo_kind.
>> 
>> Did you also hit it or you were able to build the kernel with
>> BR2_LINUX_KERNEL_NEEDS_HOST_PAHOLE?

It seems like I was tested with a too recent kernel and originally preparing
this patch and therefore didn't hit this error.

>> 
>> 
>> Best regards and thank you in advance.
>> 
>> 
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot