On Thu, Aug 22, 2024 at 4:36 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> I ran out of time looking through this, I'll try to get back to this
> patch set later today or tomorrow. So please don't repost, but see my
> comments below.
>
> On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@xxxxxxxxx>
> >
> > Support for handling BTF data of either endianness was added in [1], but
> > did not include BTF.ext data for lack of use cases. Later, support for
> > static linking [2] provided a use case, but this feature and later ones
> > were restricted to native-endian usage.
> >
> > Add support for BTF.ext handling in either endianness. Convert BTF.ext data
> > to native endianness when read into memory for further processing, and
> > support raw data access that restores the original byte-order for output.
> >
> > Add new API functions btf_ext__endianness() and btf_ext__set_endianness()
> > for query and setting byte-order, as already exist for BTF data.
> >
> > [1]:commit 3289959b97ca ("libbpf: Support BTF loading and raw data output in both endianness")
> > [2]:commit 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
>
> Is this some standard syntax to refer to commit? If not, maybe drop
> ":commit" parts?
>
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@xxxxxxxxx>
> > ---
> > tools/lib/bpf/btf.c | 163 ++++++++++++++++++++++++++++++--
> > tools/lib/bpf/btf.h | 3 +
> > tools/lib/bpf/libbpf.map | 2 +
> > tools/lib/bpf/libbpf_internal.h | 2 +
> > 4 files changed, 161 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index cf4f7bd7ff5c..cb4ee4ed4b5c 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -2884,6 +2884,52 @@ int btf__add_decl_tag(struct btf *btf, const char *value, int ref_type_id,
> > return btf_commit_type(btf, sz);
> > }
> >
> > +/*
> > + * Swap endianness of the info segment in a BTF.ext data section:
> > + * - requires BTF.ext header data in native byte order
> > + * - expects info record fields are 32-bit
> > + */
> > +static int btf_ext_bswap_info(void *data, const __u32 data_size)
>
> pass btf_ext_header (in native endianness) as input argument
>
> > +{
> > + const struct btf_ext_header *hdr = data;
> > + __u32 info_size, sum_len, i, *p;
> > +
> > + if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
> > + pr_warn("BTF.ext initial header not found\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (data_size < hdr->hdr_len) {
> > + pr_warn("BTF.ext header not found\n");
> > + return -EINVAL;
> > + }
>
> and we already checked this, no?
>
> > +
> > + if (hdr->hdr_len < offsetofend(struct btf_ext_header, line_info_len)) {
> > + pr_warn("BTF.ext header missing func_info, line_info\n");
> > + return -EINVAL;
> > + }
>
> we have this check in btf_ext__new() already, do the swap after we
> validated this and keep this function simpler and more focused. You
> are mixing sanity checks with byte swapping, it's confusing.
>
> > +
> > + sum_len = hdr->func_info_len + hdr->line_info_len;
> > + if (hdr->hdr_len >= offsetofend(struct btf_ext_header, core_relo_len))
> > + sum_len += hdr->core_relo_len;
> > +
> > + info_size = data_size - hdr->hdr_len;
> > + if (info_size != sum_len) {
> > + pr_warn("BTF.ext info size mismatch with header data\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (info_size && info_size % sizeof(__u32)) {
> > + pr_warn("BTF.ext info size not 32-bit multiple\n");
> > + return -EINVAL;
> > + }
>
> same as above, all this has nothing to do with endianness, it's all
> sanity checks. Let's extract them into a separate validation function
> that can happen before byte swap.
>
> > +
> > + p = data + hdr->hdr_len;
> > + for (i = 0; i < info_size / sizeof(__u32); i++, p++)
> > + *p = bswap_32(*p);
>
> this is very suspicious, it might work right now because all fields
> are u32, but I'm uncomfortable making these decision.
>
> I think for byte swapping case we need to make sure that BTF version
> is exactly known to us (i.e., all record length is knowable for us),
> and then do more verbose, but correct byte swap based on
> bpf_func_info/bpf_line_info/etc records. Just treating all this as u32
> is not great.
>
> > + return 0;
> > +}
> > +
> > struct btf_ext_sec_setup_param {
> > __u32 off;
> > __u32 len;
> > @@ -3023,24 +3069,56 @@ static int btf_ext_setup_core_relos(struct btf_ext *btf_ext)
> > return btf_ext_setup_info(btf_ext, ¶m);
> > }
> >
> > -static int btf_ext_parse_hdr(__u8 *data, __u32 data_size)
> > +/* Swap byte-order of BTF.ext header */
> > +static void btf_ext_bswap_hdr(struct btf_ext_header *h)
> > {
> > - const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
> > + __u32 hdr_len = h->hdr_len; /* need native byte-order */
> >
> > - if (data_size < offsetofend(struct btf_ext_header, hdr_len) ||
> > - data_size < hdr->hdr_len) {
> > - pr_debug("BTF.ext header not found\n");
> > + if (h->magic == bswap_16(BTF_MAGIC))
> > + hdr_len = bswap_32(hdr_len);
>
> why so complicated? just pass hdr_len as input argument into
> btf_ext_bswap_hdr(), and then uniformly swap everything, no
> double-checking of decisions we already made
>
> > +
> > + h->magic = bswap_16(h->magic);
> > + h->hdr_len = bswap_32(h->hdr_len);
> > + h->func_info_off = bswap_32(h->func_info_off);
> > + h->func_info_len = bswap_32(h->func_info_len);
> > + h->line_info_off = bswap_32(h->line_info_off);
> > + h->line_info_len = bswap_32(h->line_info_len);
> > +
> > + if (hdr_len < offsetofend(struct btf_ext_header, core_relo_len))
> > + return;
> > +
> > + h->core_relo_off = bswap_32(h->core_relo_off);
> > + h->core_relo_len = bswap_32(h->core_relo_len);
> > +}
> > +
> > +static int btf_ext_parse_hdr(struct btf_ext *btf_ext)
> > +{
> > + struct btf_ext_header *hdr = btf_ext->hdr;
> > + __u32 hdr_len, data_size = btf_ext->data_size;
> > +
> > + if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
> > + pr_debug("BTF.ext initial header not found\n");
>
> "BTF.ext header too short" ?
>
> > return -EINVAL;
> > }
> >
> > + hdr_len = hdr->hdr_len;
> > if (hdr->magic == bswap_16(BTF_MAGIC)) {
> > - pr_warn("BTF.ext in non-native endianness is not supported\n");
> > - return -ENOTSUP;
> > + btf_ext->swapped_endian = true;
> > + hdr_len = bswap_32(hdr_len);
> > } else if (hdr->magic != BTF_MAGIC) {
> > pr_debug("Invalid BTF.ext magic:%x\n", hdr->magic);
> > return -EINVAL;
> > }
> >
> > + if (data_size < hdr_len) {
> > + pr_debug("BTF.ext header not found\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Maintain native byte-order in memory for introspection */
> > + if (btf_ext->swapped_endian)
> > + btf_ext_bswap_hdr(hdr);
> > +
> > if (hdr->version != BTF_VERSION) {
> > pr_debug("Unsupported BTF.ext version:%u\n", hdr->version);
> > return -ENOTSUP;
> > @@ -3067,6 +3145,7 @@ void btf_ext__free(struct btf_ext *btf_ext)
> > free(btf_ext->line_info.sec_idxs);
> > free(btf_ext->core_relo_info.sec_idxs);
> > free(btf_ext->data);
> > + free(btf_ext->data_swapped);
> > free(btf_ext);
> > }
> >
> > @@ -3087,7 +3166,12 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
> > }
> > memcpy(btf_ext->data, data, size);
> >
> > - err = btf_ext_parse_hdr(btf_ext->data, size);
> > + err = btf_ext_parse_hdr(btf_ext);
> > + if (err)
> > + goto done;
> > +
> > + if (btf_ext->swapped_endian)
> > + err = btf_ext_bswap_info(btf_ext->data, btf_ext->data_size);
> > if (err)
> > goto done;
>
> nit:
>
> if (btf_ext->swapped_endian) {
> err = ...;
> if (err)
> goto done;
> }
>
> i.e., keep error handling close and at the same nesting level
>
> >
> > @@ -3120,15 +3204,76 @@ 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;
> > + int err;
> > +
> > + 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) {
> > + err = btf_ext_bswap_info(data, data_sz);
> > + if (err) {
> > + free(data);
> > + return NULL;
> > + }
> > + btf_ext_bswap_hdr(data);
> > + btf_ext->data_swapped = data;
> > + }
> > +
> > + *size = data_sz;
> > + return data;
> > +}
> > +
> > const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size)
> > {
> > + __u32 data_sz;
> > + void *data;
> > +
> > + data = btf_ext_raw_data(btf_ext, &data_sz, btf_ext->swapped_endian);
> > + if (!data)
> > + return errno = ENOMEM, NULL;
> > +
> > *size = btf_ext->data_size;
> > - return btf_ext->data;
> > + return data;
> > }
> >
> > __attribute__((alias("btf_ext__raw_data")))
> > const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext, __u32 *size);
> >
> > +enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext)
> > +{
> > + return (is_host_big_endian() != btf_ext->swapped_endian) ?
> > + BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
> > +}
nit: this looks correct, but I'd prefer consistency with
btf__endianness() in terms of implementation, if you don't mind
> > +
> > +int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
> > +{
> > + bool is_tgt_big_endian = (endian == BTF_BIG_ENDIAN);
> > +
> > + if (endian != BTF_LITTLE_ENDIAN && endian != BTF_BIG_ENDIAN)
> > + return libbpf_err(-EINVAL);
> > +
> > + btf_ext->swapped_endian = (is_host_big_endian() != is_tgt_big_endian);
nit: any reason you didn't just stick to the same style as in
btf__set_endianness()? you clearly took that as an inspiration. Were
you worried about crossing the 80 characters boundary? Don't be, the
limit is 100 characters. Anyways, unnecessary () and I'd inline
is_tgt_big_endian given it's single use.
> > +
> > + if (!btf_ext->swapped_endian) {
> > + free(btf_ext->data_swapped);
> > + btf_ext->data_swapped = NULL;
> > + }
> > + return 0;
> > +}
> >
> > struct btf_dedup;
> >
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index b68d216837a9..e3cf91687c78 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -167,6 +167,9 @@ LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
> > LIBBPF_API struct btf_ext *btf_ext__new(const __u8 *data, __u32 size);
> > LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
> > LIBBPF_API const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size);
> > +LIBBPF_API enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext);
> > +LIBBPF_API int btf_ext__set_endianness(struct btf_ext *btf_ext,
> > + enum btf_endianness endian);
> >
> > LIBBPF_API int btf__find_str(struct btf *btf, const char *s);
> > LIBBPF_API int btf__add_str(struct btf *btf, const char *s);
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 8f0d9ea3b1b4..5c17632807b6 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -421,6 +421,8 @@ LIBBPF_1.5.0 {
> > global:
> > btf__distill_base;
> > btf__relocate;
> > + btf_ext__endianness;
> > + btf_ext__set_endianness;
> > bpf_map__autoattach;
> > bpf_map__set_autoattach;
> > bpf_program__attach_sockmap;
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 8cda511a1982..6b0270c83537 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -484,6 +484,8 @@ struct btf_ext {
> > struct btf_ext_header *hdr;
> > void *data;
> > };
> > + void *data_swapped;
> > + bool swapped_endian;
> > struct btf_ext_info func_info;
> > struct btf_ext_info line_info;
> > struct btf_ext_info core_relo_info;
> > --
> > 2.34.1
> >
