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

[Tony Ambardar <tony.ambardar@gmail.com>]

On Fri, Aug 30, 2024 at 05:15:06PM -0700, Eduard Zingerman wrote:
> 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.

Hi Eduard,

Sorry, I saw this earlier but dropped my reply by mistake I think. You're
right that btf_ext->data can't be null, and the awkwardness above is a
holdover from trying to use the btf_raw_data() code, where it _can_ be
null. I've rewritten it to be clearer for the next v6 series, which also
reuses existing info sec validation and drops the extra code you referred
to further above.

Thanks,
Tony

> 
> > +
> > +	*size = data_sz;
> > +	return data;
> > +}
> > +
> 
> [...]
>