diff mbox series

libbpf: checking the btf_type kind when fixing variable offsets

Message ID 20240616002958.2095829-1-dolinux.peng@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series libbpf: checking the btf_type kind when fixing variable offsets | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc

Commit Message

Donglin Peng June 16, 2024, 12:29 a.m. UTC 
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/

Fixes: 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
---
 tools/lib/bpf/linker.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Alan Maguire June 17, 2024, 11:29 a.m. UTC | #1 
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;
Eduard Zingerman June 17, 2024, 6:12 p.m. UTC | #2 
On Sat, 2024-06-15 at 17:29 -0700, 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/
> 
> Fixes: 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
> Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
> ---

Good catch, thank you for narrowing this down.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

(Although, I agree with notes from Alan, having a comment would be good).
Eduard Zingerman June 17, 2024, 8 p.m. UTC | #3 
On Mon, 2024-06-17 at 12:29 +0100, Alan Maguire wrote:

[...]

> 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.

I've added the following debug logging:
 
                        sym = find_sym_by_name(obj, sec->sec_idx, STT_OBJECT, var_name);
                        if (!sym) {
+                               const struct btf_type *nt;
                                pr_warn("failed to find symbol for variable '%s' in section '%s'\n", var_name, sec_name);
+                               nt = btf__type_by_id(obj->btf, vi->type + 1);
+                               pr_warn("  vi->type == %d\n", vi->type);
+                               pr_warn("  next id %d kind '%s', name '%s' off %d\n",
+                                       vi->type + 1,
+                                       btf_kind_str(nt),
+                                       btf__str_by_offset(obj->btf, nt->name_off), nt->name_off);
                                return -ENOENT;
                        }

The output is as follows:

  libbpf: failed to find symbol for variable 'bpf_dynptr_slice' in section '.ksyms'
  libbpf:   vi->type == 17
  libbpf:   next id 18 kind 'struct', name 'bpf_nf_ctx' off 1

This matches your analysis and hits the unlikely situation when
name_off of the next type is 1.
Andrii Nakryiko June 17, 2024, 8:51 p.m. UTC | #4 
On Mon, Jun 17, 2024 at 4:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> 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()).

Let's go a half-step further and validate that we see either VAR or
FUNC and for anything unexpected error out loudly, so this is brought
to someone's attention sooner?

Something like:

if (btf_is_func(vt))
    continue; /* FUNC doesn't need adjustment */
if (!btf_is_var(vt)) {
    pr_warn(<helpful and descriptive enough message about unexpected type>);
    return -EINVAL;
}

(use btf_kind_str() for error string, btw)

pw-bot: cr

>
>
> > +                             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;
Donglin Peng June 19, 2024, 5:02 a.m. UTC | #5 
On Mon, Jun 17, 2024 at 7:29 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> 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.

Yes, I agree.

>
> > 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>

Thanks.

>
> > ---
> >  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 */

Thanks, I will update in v2.

>
> 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;
Donglin Peng June 19, 2024, 5:03 a.m. UTC | #6 
On Tue, Jun 18, 2024 at 2:12 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Sat, 2024-06-15 at 17:29 -0700, 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/
> >
> > Fixes: 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
> > Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
> > ---
>
> Good catch, thank you for narrowing this down.
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Thanks, I will update in v2.

>
> (Although, I agree with notes from Alan, having a comment would be good).
diff mbox series

Patch

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)
+				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;