Re: [RFC bpf-next 1/8] btf: add kind metadata encoding to UAPI

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

On 01/06/2023 04:53, Alexei Starovoitov wrote:
> On Wed, May 31, 2023 at 09:19:28PM +0100, Alan Maguire wrote:
>> BTF kind metadata provides information to parse BTF kinds.
>> By separating parsing BTF from using all the information
>> it provides, we allow BTF to encode new features even if
>> they cannot be used.  This is helpful in particular for
>> cases where newer tools for BTF generation run on an
>> older kernel; BTF kinds may be present that the kernel
>> cannot yet use, but at least it can parse the BTF
>> provided.  Meanwhile userspace tools with newer libbpf
>> may be able to use the newer information.
>>
>> The intent is to support encoding of kind metadata
>> optionally so that tools like pahole can add this
>> information.  So for each kind we record
>>
>> - a kind name string
>> - kind-related flags
>> - length of singular element following struct btf_type
>> - length of each of the btf_vlen() elements following
>>
>> In addition we make space in the metadata for
>> CRC32s computed over the BTF along with a CRC for
>> the base BTF; this allows split BTF to identify
>> a mismatch explicitly.  Finally we provide an
>> offset for an optional description string.
>>
>> The ideas here were discussed at [1] hence
>>
>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>
>> [1] https://lore.kernel.org/bpf/CAEf4BzYjWHRdNNw4B=eOXOs_ONrDwrgX4bn=Nuc1g8JPFC34MA@mail.gmail.com/
>> ---
>>  include/uapi/linux/btf.h       | 29 +++++++++++++++++++++++++++++
>>  tools/include/uapi/linux/btf.h | 29 +++++++++++++++++++++++++++++
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
>> index ec1798b6d3ff..94c1f4518249 100644
>> --- a/include/uapi/linux/btf.h
>> +++ b/include/uapi/linux/btf.h
>> @@ -8,6 +8,34 @@
>>  #define BTF_MAGIC	0xeB9F
>>  #define BTF_VERSION	1
>>  
>> +/* is this information required? If so it cannot be sanitized safely. */
>> +#define BTF_KIND_META_OPTIONAL		(1 << 0)
>> +
>> +struct btf_kind_meta {
>> +	__u32 name_off;		/* kind name string offset */
>> +	__u16 flags;		/* see BTF_KIND_META_* values above */
>> +	__u8 info_sz;		/* size of singular element after btf_type */
>> +	__u8 elem_sz;		/* size of each of btf_vlen(t) elements */
>> +};
>> +
>> +/* for CRCs for BTF, base BTF to be considered usable, flags must be set. */
>> +#define BTF_META_CRC_SET		(1 << 0)
>> +#define BTF_META_BASE_CRC_SET		(1 << 1)
>> +
>> +struct btf_metadata {
>> +	__u8	kind_meta_cnt;		/* number of struct btf_kind_meta */
>> +	__u32	flags;
>> +	__u32	description_off;	/* optional description string */
>> +	__u32	crc;			/* crc32 of BTF */
>> +	__u32	base_crc;		/* crc32 of base BTF */
>> +	struct btf_kind_meta kind_meta[];
>> +};
>> +
>> +struct btf_meta_header {
>> +	__u32	meta_off;	/* offset of metadata section */
>> +	__u32	meta_len;	/* length of metadata section */
>> +};
>> +
>>  struct btf_header {
>>  	__u16	magic;
>>  	__u8	version;
>> @@ -19,6 +47,7 @@ struct btf_header {
>>  	__u32	type_len;	/* length of type section	*/
>>  	__u32	str_off;	/* offset of string section	*/
>>  	__u32	str_len;	/* length of string section	*/
>> +	struct btf_meta_header meta_header;
>>  };
>>  
>>  /* Max # of type identifier */
>> diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
>> index ec1798b6d3ff..94c1f4518249 100644
>> --- a/tools/include/uapi/linux/btf.h
>> +++ b/tools/include/uapi/linux/btf.h
>> @@ -8,6 +8,34 @@
>>  #define BTF_MAGIC	0xeB9F
>>  #define BTF_VERSION	1
>>  
>> +/* is this information required? If so it cannot be sanitized safely. */
>> +#define BTF_KIND_META_OPTIONAL		(1 << 0)
>> +
>> +struct btf_kind_meta {
>> +	__u32 name_off;		/* kind name string offset */
>> +	__u16 flags;		/* see BTF_KIND_META_* values above */
>> +	__u8 info_sz;		/* size of singular element after btf_type */
>> +	__u8 elem_sz;		/* size of each of btf_vlen(t) elements */
>> +};
>> +
>> +/* for CRCs for BTF, base BTF to be considered usable, flags must be set. */
>> +#define BTF_META_CRC_SET		(1 << 0)
>> +#define BTF_META_BASE_CRC_SET		(1 << 1)
>> +
>> +struct btf_metadata {
>> +	__u8	kind_meta_cnt;		/* number of struct btf_kind_meta */
> 
> Overall, looks great.
> Few small nits:
> I'd make kind_meta_cnt u32, since padding we won't be able to reuse anyway
> and would bump the BTF_VERSION to 2 to make it a 'milestone'.
> v2 -> self described.

sure, sounds good. One other change perhaps worth making; currently
we assume that the kind metadata is at the end of the struct
btf_metadata, but if we ever wanted to add metadata fields in the
future, we'd want so support both the current metadata structure and
any future structure which had additional fields.

With that in mind, it might make sense to go with something like

struct btf_metadata {
	__u32	kind_meta_cnt;
	__u32	kind_meta_offset;	/* kind_meta_cnt instances of struct
btf_kind_meta start here */
	__u32	flags;
	__u32	description_off;	/* optional description string*/
	__u32	crc;			/* crc32 of BTF */
	__u32	base_crc;		/* crc32 of base BTF */
};

For the original version, kind_meta_offset would just be
at meta_off + sizeof(struct btf_metadata), but if we had multiple
versions of the btf_metadata header to handle, they could all rely on
the kind_meta_offset being where kind metadata is stored.
For validation we'd have to make sure kind_meta_offset was within
the the metadata header range.

> 
>> +	__u32	flags;
>> +	__u32	description_off;	/* optional description string */
>> +	__u32	crc;			/* crc32 of BTF */
>> +	__u32	base_crc;		/* crc32 of base BTF */
> 
> Hard coded CRC also gives me a pause.
> Should it be an optional KIND like btf tags?

The goal of the CRC is really just to provide a unique identifier that
we can use for things like checking if there's a mismatch between
base and module BTF. If we want to ever do CRC validation (not sure
if there's a case for that) we probably need to think about cases like
BTF sanitization of BPF program BTF; this would likely only be an
issue if metadata support is added to BPF compilers.

The problem with adding it via a kind is that if we first compute
the CRC over the entire BTF object and then add the kind, the addition
of the kind breaks the CRC; as a result I _think_ we're stuck with
having to have it in the header.

That said I don't think CRC is necessarily the only identifier
we could use, and we don't even need to identify it as a
CRC in the UAPI, just as a "unique identifier"; that would deal
with issues about breaking the CRC during sanitization. All
depends on whether we ever see a need to verify BTF via CRC
really.

One note; I found that the changes in patch 4 break kernel BTF
parsing when using the original BTF header, so if anyone is trying this
out, make sure the dwarves changes to generate BTF metadata are in
place. I've got a fix and will send it with v2 but don't want to spam
the list with a whole new series, so the following diff will fix it:

--- kernel/bpf/btf.c.original	2023-06-01 11:20:39.418807425 +0100
+++ kernel/bpf/btf.c	2023-06-01 11:20:57.012807358 +0100
@@ -5260,13 +5260,13 @@
 		secs[i] = *(struct btf_sec_info *)((void *)hdr +
 						   btf_sec_info_offset[i]);

-	sort(secs, ARRAY_SIZE(btf_sec_info_offset),
+	sort(secs, nr_secs,
 	     sizeof(struct btf_sec_info), btf_sec_info_cmp, NULL);

 	/* Check for gaps and overlap among sections */
 	total = 0;
 	expected_total = btf_data_size - hdr->hdr_len;
-	for (i = 0; i < ARRAY_SIZE(btf_sec_info_offset); i++) {
+	for (i = 0; i < nr_secs; i++) {
 		if (expected_total < secs[i].off) {
 			btf_verifier_log(env, "Invalid section offset");
 			return -EINVAL;