Re: [PATCH bpf-next v4 4/8] libbpf: Support BTF.ext loading and output in either endianness

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-08-30 at 00:29 -0700, Tony Ambardar wrote:

[...]

> @@ -3050,11 +3127,42 @@ static int btf_ext_parse_hdr(__u8 *data, __u32 data_size)
>  		return -ENOTSUP;
>  	}
>  
> -	if (data_size == hdr->hdr_len) {
> +	if (data_size < hdr_len) {
> +		pr_debug("BTF.ext header not found\n");
> +		return -EINVAL;
> +	} else if (data_size == hdr_len) {
>  		pr_debug("BTF.ext has no data\n");
>  		return -EINVAL;
>  	}
>  
> +	/* Verify mandatory hdr info details present */
> +	if (hdr_len < offsetofend(struct btf_ext_header, line_info_len)) {
> +		pr_warn("BTF.ext header missing func_info, line_info\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Keep hdr native byte-order in memory for introspection */
> +	if (btf_ext->swapped_endian)
> +		btf_ext_bswap_hdr(btf_ext, hdr_len);
> +
> +	/* Basic info section consistency checks*/
> +	info_size = btf_ext->data_size - hdr_len;
> +	if (info_size & 0x03) {
> +		pr_warn("BTF.ext info size not 4-byte multiple\n");
> +		return -EINVAL;
> +	}
> +	info_size -= hdr->func_info_len + hdr->line_info_len;
> +	if (hdr_len >= offsetofend(struct btf_ext_header, core_relo_len))
> +		info_size -= hdr->core_relo_len;

nit: Since we are checking this, maybe also check that sections do not overlap?
     Also, why disallowing gaps between sections?

> +	if (info_size) {
> +		pr_warn("BTF.ext info size mismatch with header data\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Keep infos native byte-order in memory for introspection */
> +	if (btf_ext->swapped_endian)
> +		btf_ext_bswap_info(btf_ext, !btf_ext->swapped_endian);
> +
>  	return 0;
>  }

[...]

> @@ -3119,15 +3223,71 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
>  	return btf_ext;
>  }
>  
> +static void *btf_ext_raw_data(const struct btf_ext *btf_ext_ro, __u32 *size,
> +			      bool swap_endian)
> +{
> +	struct btf_ext *btf_ext = (struct btf_ext *)btf_ext_ro;
> +	const __u32 data_sz = btf_ext->data_size;
> +	void *data;
> +
> +	data = swap_endian ? btf_ext->data_swapped : btf_ext->data;
> +	if (data) {
> +		*size = data_sz;
> +		return data;
> +	}
> +
> +	data = calloc(1, data_sz);
> +	if (!data)
> +		return NULL;
> +	memcpy(data, btf_ext->data, data_sz);
> +
> +	if (swap_endian) {
> +		btf_ext_bswap_info(btf_ext, true);
> +		btf_ext_bswap_hdr(btf_ext, btf_ext->hdr->hdr_len);
> +		btf_ext->data_swapped = data;
> +	}

Nit: I don't like how this function is organized:
     - if btf_ext->data can't be NULL swap_endian == true at this point;
     - if btf_ext->data can be NULL and swap_endian == false
       pointer to `data` would be lost.

     I assume that btf_ext->data can't be null, basing on the
     btf_ext__new(), but function body is a bit confusing.

> +
> +	*size = data_sz;
> +	return data;
> +}
> +

[...]