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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, &param);
> >  }
> >
> > -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
> >





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux