On Tue, Jun 6, 2023 at 6:16 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Jun 6, 2023 at 9:50 AM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > Agreed, I don't think we can ever make BTF dedup work reliably with > > KINDs it doesn't understand. I wouldn't even try. I'd also say that > > kernel should keep being strict about this (even if we add > > "is-it-optional" field, kernel can't trust it). Libbpf and other > > libraries will have to keep sanitizing BTF anyways. > > Good point. "it-is-optional" flag should be for user space only. > > > > If we go this simple route I'm fine with hard coded crc and base_crc > > > fields. They probably should go to btf_header though. > > > > Yep, on btf_header fields. But I'd not hardcode "crc" name. If we are > > doing them as strings (which I think we should instead of dooming them > > to 32-bit integer crc32 value only), then can we just say generically > > that it's either "id" or "checksum"? > > > > But I guess crc32 would be fine in practice as well. So not something > > I strongly feel about. > > I still fail to see how generic string "id" helps. > We have to standardize on a way to checksum BTF-s. > Say, we pick crc32. > pahole/clang would have to use the same algorithm. > Then kernel during BTF_LOAD should check that crc32 matches > to make sure btf data didn't get corrupted between its creation > and loading into the kernel. > Just like btrfs uses crc32 to make sure data doesn't get corrupted by disk. > libbpf doing sanitization would need to tweak crc32 too. > So it's going to be hard coded semantics at every level. > id and especially string id would be cumbersome for all these layers > to deal with. Ok, that's totally fine with me. For me this whole checksumming was less about checksum and validating content of BTF wasn't corrupted. It was more about making sure that split BTF matches base BTF. And for that readers (libbpf, tools, etc) wouldn't recalculate checksums on their own. They'd get those checksums/IDs and just compare them. Whether it's opaque string or int is absolutely irrelevant for this use case (which was the main one for me). But as I said, I'm fine either way. Let's hard-code crc32, it's simpler to generate for sure. > > > > > We don't need "struct btf_metadata" as well. > > > It's making things sound beyond what it actually is. > > > btf_header can point to an array of struct btf_kind_description. > > > As simple as it can get. > > > > Agreed. Still, it's a third section, and we should at least have a > > count of those btf_kind_layout items somewhere. > > of course. > In btf_header we have > __u32 type_off; /* offset of type section */ > __u32 type_len; /* length of type section */ > I meant we add: > __u32 kind_layouts_off; > __u32 kind_layouts_len; ok, sounds good
