diff mbox series

[bpf] libbpf: btf dedup identical struct test needs check for nested structs/arrays

Message ID 1666622309-22289-1-git-send-email-alan.maguire@oracle.com (mailing list archive)
State Accepted
Commit f3c51fe02c55bd944662714e5b91b96dc271ad9f
Delegated to: BPF
Headers show
Series [bpf] libbpf: btf dedup identical struct test needs check for nested structs/arrays | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
netdev/tree_selection success Clearly marked for bpf
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-1 success Logs for build for s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-6 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-9 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-12 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for test_progs_no_alu32 on x86_64 with llvm-16

Commit Message

Alan Maguire Oct. 24, 2022, 2:38 p.m. UTC
When examining module BTF, it is common to see core kernel structures
such as sk_buff, net_device duplicated in the module.  After adding
debug messaging to BTF it turned out that much of the problem
was down to the identical struct test failing during deduplication;
sometimes the compiler adds identical structs.  However
it turns out sometimes that type ids of identical struct members
can also differ, even when the containing structs are still identical.

To take an example, for struct sk_buff, debug messaging revealed
that the identical struct matching was failing for the anon
struct "headers"; specifically for the first field:

__u8       __pkt_type_offset[0]; /*   128     0 */

Looking at the code in BTF deduplication, we have code that guards
against the possibility of identical struct definitions, down to
type ids, and identical array definitions.  However in this case
we have a struct which is being defined twice but does not have
identical type ids since each duplicate struct has separate type
ids for the above array member.   A similar problem (though not
observed) could occur for struct-in-struct.

The solution is to make the "identical struct" test check members
not just for matching ids, but to also check if they in turn are
identical structs or arrays.

The results of doing this are quite dramatic (for some modules
at least); I see the number of type ids drop from around 10000
to just over 1000 in one module for example.

For testing use latest pahole or apply [1], otherwise dedups
can fail for the reasons described there.

Also fix return type of btf_dedup_identical_arrays() as
suggested by Andrii to match boolean return type used
elsewhere.

Fixes: efdd3eb8015e ("libbpf: Accommodate DWARF/compiler bug with duplicated structs")
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

[1] https://lore.kernel.org/bpf/1666364523-9648-1-git-send-email-alan.maguire
---
 tools/lib/bpf/btf.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org Oct. 25, 2022, 11:30 p.m. UTC | #1
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Mon, 24 Oct 2022 15:38:29 +0100 you wrote:
> When examining module BTF, it is common to see core kernel structures
> such as sk_buff, net_device duplicated in the module.  After adding
> debug messaging to BTF it turned out that much of the problem
> was down to the identical struct test failing during deduplication;
> sometimes the compiler adds identical structs.  However
> it turns out sometimes that type ids of identical struct members
> can also differ, even when the containing structs are still identical.
> 
> [...]

Here is the summary with links:
  - [bpf] libbpf: btf dedup identical struct test needs check for nested structs/arrays
    https://git.kernel.org/bpf/bpf-next/c/f3c51fe02c55

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index d88647d..675a0df 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -3887,14 +3887,14 @@  static inline __u16 btf_fwd_kind(struct btf_type *t)
 }
 
 /* Check if given two types are identical ARRAY definitions */
-static int btf_dedup_identical_arrays(struct btf_dedup *d, __u32 id1, __u32 id2)
+static bool btf_dedup_identical_arrays(struct btf_dedup *d, __u32 id1, __u32 id2)
 {
 	struct btf_type *t1, *t2;
 
 	t1 = btf_type_by_id(d->btf, id1);
 	t2 = btf_type_by_id(d->btf, id2);
 	if (!btf_is_array(t1) || !btf_is_array(t2))
-		return 0;
+		return false;
 
 	return btf_equal_array(t1, t2);
 }
@@ -3918,7 +3918,9 @@  static bool btf_dedup_identical_structs(struct btf_dedup *d, __u32 id1, __u32 id
 	m1 = btf_members(t1);
 	m2 = btf_members(t2);
 	for (i = 0, n = btf_vlen(t1); i < n; i++, m1++, m2++) {
-		if (m1->type != m2->type)
+		if (m1->type != m2->type &&
+		    !btf_dedup_identical_arrays(d, m1->type, m2->type) &&
+		    !btf_dedup_identical_structs(d, m1->type, m2->type))
 			return false;
 	}
 	return true;