On Fri, Oct 21, 2022 at 2:56 PM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> When examining module BTF, we often 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 compilation units contain 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 potentially occur for a struct-in-a-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, and kernel
> module types are no longer duplicated.
>
> For testing with latest pahole, applying [1] is required,
> otherwise dedups can fail for the reasons described there.
>
> All BTF-related selftests passed with this change.
>
> RFC for bpf-next rather than patch for bpf tree because while
> this resolves dedup issues for me using gcc 9 and 11,
> these things seem to be quite compiler-sensitive, so would
> be good to ensure it works for others too. Presuming it
> does, should probably specify:
>
> Fixes: efdd3eb8015e ("libbpf: Accommodate DWARF/compiler bug with duplicated structs")
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> [1] https://lore.kernel.org/bpf/1666364523-9648-1-git-send-email-alan.maguire@xxxxxxxxxx/
> ---
> tools/lib/bpf/btf.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index d88647d..b7d7f19 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -3918,8 +3918,11 @@ 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)
> - return false;
> + if (m1->type == m2->type ||
> + btf_dedup_identical_structs(d, m1->type, m2->type) ||
> + btf_dedup_identical_arrays(d, m1->type, m2->type))
> + continue;
> + return false;
this makes a lot of sense and I don't see why this would be incorrect.
Please submit this as non-RFC patch. I'd just keep the overall "return
false on mismatch" approach:
if (m1->type != m2->type &&
!btf_dedup_identical_arrays(d, m1->type, m2->type) &&
!btf_dedup_identical_structs(d, m1->type, m2->type))
return;
oh, can you please also change btf_dedup_identical_arrays() signature
to return bool, no idea why it returns int (0 or 1).
> }
> return true;
> }
> --
> 1.8.3.1
>
