Re: [PATCH] libbpf: checking the btf_type kind when fixing variable offsets

[Alan Maguire <alan.maguire@oracle.com>]

On 16/06/2024 01:29, Donglin Peng wrote:
> I encountered an issue when building the test_progs using the repository[1]:
> 
> $ clang --version
> Ubuntu clang version 17.0.6 (++20231208085846+6009708b4367-1~exp1~20231208085949.74)
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin
> 
> $ pwd
> /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/
> 
> $ make test_progs V=1
> ...
> /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/tools/sbin/bpftool
> gen object
> /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked2.o
> /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked1.o
> libbpf: failed to find symbol for variable 'bpf_dynptr_slice' in section
> '.ksyms'
> Error: failed to link
> '/work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked1.o':
> No such file or directory (2)
> make: *** [Makefile:656:
> /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.skel.h]
> Error 254
> 
> After investigation, I found that the btf_types in the '.ksyms' section have a kind of
> BTF_KIND_FUNC instead of BTF_KIND_VAR:
> 
> $ bpftool btf dump file ./ip_check_defrag.bpf.linked1.o
> ...
> [2] DATASEC '.ksyms' size=0 vlen=2
>         type_id=16 offset=0 size=0 (FUNC 'bpf_dynptr_from_skb')
>         type_id=17 offset=0 size=0 (FUNC 'bpf_dynptr_slice')
> ...
> [16] FUNC 'bpf_dynptr_from_skb' type_id=82 linkage=extern
> [17] FUNC 'bpf_dynptr_slice' type_id=85 linkage=extern
> ...
> 
> To fix this, we can a add check for the kind.
> 
> [1] https://github.com/eddyz87/bpf/tree/binsort-btf-dedup
> Link: https://lore.kernel.org/all/4f551dc5fc792936ca364ce8324c0adea38162f1.camel@gmail.com/
> 

The fix makes sense; what I was trying to figure out is why we're only
seeing this now with the above repo.

So as I understand it, the reason the kfuncs end up in the .ksyms
section is due to the "__weak __ksym" tagging recently added to
vmlinux.h construction from BTF via

770abbb5a25a ("bpftool: Support dumping kfunc prototypes from BTF")

We see as noted

[112] DATASEC '.ksyms' size=0 vlen=2
	type_id=84 offset=0 size=0 (FUNC 'bpf_dynptr_from_skb')
	type_id=90 offset=0 size=0 (FUNC 'bpf_dynptr_slice')

So that makes sense, but prior to the above series, we also tagged
kfuncs in this way before via bpf_kfuncs.h. So there should be no
difference there.

And with an upstream kernel I don't run into this problem.
	
The only thing I could come up with is we were usually lucky; when we
misinterpreted the func as a var and looked its type up, we got

		int var_linkage = btf_var(vt)->linkage;

...and were lucky it never equalled 1 (BTF_VAR_GLOBAL_ALLOCATED):
	
		/* no need to patch up static or extern vars */
                if (var_linkage != BTF_VAR_GLOBAL_ALLOCATED)
			continue;

In the case of a function, the above btf_var(vt) would really be
pointing at the struct btf_type immediately after the relevant
function's struct btf_type (since unlike variables, functions don't have
metadata following them). So the only way we'd trip this bug would be if
the struct btf_type following the func was had a name_off value that
happened to equal 1 (BTF_VAR_GLOBAL_ALLOCATED).

So maybe the sorting changes to BTF order resulted in us tripping on
this bug, but regardless the fix seems right to me.

> Fixes: 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
> Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>

A few small things below, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  tools/lib/bpf/linker.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 0d4be829551b..7f5fc9ac4ad6 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -2213,10 +2213,17 @@ static int linker_fixup_btf(struct src_obj *obj)
>  		vi = btf_var_secinfos(t);
>  		for (j = 0, m = btf_vlen(t); j < m; j++, vi++) {
>  			const struct btf_type *vt = btf__type_by_id(obj->btf, vi->type);
> -			const char *var_name = btf__str_by_offset(obj->btf, vt->name_off);
> -			int var_linkage = btf_var(vt)->linkage;
> +			const char *var_name;
> +			int var_linkage;
>  			Elf64_Sym *sym;
>  
> +			/* should be a variable */
> +			if (btf_kind(vt) != BTF_KIND_VAR)

nit: could use if (!btf_is_var(vt)) here instead. It might also be worth
reworking the comment to acknowledge that we can legitimately have a
function in this section; i.e. something like
			
			/* could be a variable or function */

We handle the func case elsewhere in libbpf (see add_dummy_ksym_var()).


> +				continue;
> + 
> +			var_name = btf__str_by_offset(obj->btf, vt->name_off);
> +			var_linkage = btf_var(vt)->linkage;
> +
>  			/* no need to patch up static or extern vars */
>  			if (var_linkage != BTF_VAR_GLOBAL_ALLOCATED)
>  				continue;