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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.

> +	__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?