Re: [RFC bpf-next 2/8] libbpf: support handling of metadata section in BTF

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Jun 5, 2023 at 4:01 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, May 31, 2023 at 09:19:29PM +0100, Alan Maguire wrote:
> > support reading in metadata, fixing endian issues on reading;
> > also support writing metadata section to raw BTF object.
> > There is not yet an API to populate the metadata with meaningful
> > information.
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  tools/lib/bpf/btf.c | 141 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 111 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 8484b563b53d..036dc1505969 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/err.h>
> >  #include <linux/btf.h>
> >  #include <gelf.h>
> > +#include <zlib.h>
> >  #include "btf.h"
> >  #include "bpf.h"
> >  #include "libbpf.h"
> > @@ -39,36 +40,40 @@ struct btf {
> >
> >       /*
> >        * When BTF is loaded from an ELF or raw memory it is stored
> > -      * in a contiguous memory block. The hdr, type_data, and, strs_data
> > -      * point inside that memory region to their respective parts of BTF
> > -      * representation:
> > +      * in a contiguous memory block. The hdr, type_data, strs_data,
> > +      * and optional meta_data point inside that memory region to their
> > +      * respective parts of BTF representation:
> >        *
> > -      * +--------------------------------+
> > -      * |  Header  |  Types  |  Strings  |
> > -      * +--------------------------------+
> > -      * ^          ^         ^
> > -      * |          |         |
> > -      * hdr        |         |
> > -      * types_data-+         |
> > -      * strs_data------------+
> > +      * +--------------------------------+----------+
> > +      * |  Header  |  Types  |  Strings  | Metadata |
> > +      * +--------------------------------+----------+
> > +      * ^          ^         ^           ^
> > +      * |          |         |           |
> > +      * hdr        |         |           |
> > +      * types_data-+         |           |
> > +      * strs_data------------+           |
> > +      * meta_data------------------------+
> > +      *
> > +      * meta_data is optional.
> >        *
> >        * If BTF data is later modified, e.g., due to types added or
> >        * removed, BTF deduplication performed, etc, this contiguous
> > -      * representation is broken up into three independently allocated
> > -      * memory regions to be able to modify them independently.
> > +      * representation is broken up into three or four independently
> > +      * allocated memory regions to be able to modify them independently.
> >        * raw_data is nulled out at that point, but can be later allocated
> >        * and cached again if user calls btf__raw_data(), at which point
> > -      * raw_data will contain a contiguous copy of header, types, and
> > -      * strings:
> > +      * raw_data will contain a contiguous copy of header, types, strings
> > +      * and (again optionally) metadata:
> >        *
> > -      * +----------+  +---------+  +-----------+
> > -      * |  Header  |  |  Types  |  |  Strings  |
> > -      * +----------+  +---------+  +-----------+
> > -      * ^             ^            ^
> > -      * |             |            |
> > -      * hdr           |            |
> > -      * types_data----+            |
> > -      * strset__data(strs_set)-----+
> > +      * +----------+  +---------+  +-----------+  +----------+
> > +      * |  Header  |  |  Types  |  |  Strings  |  | Metadata |
> > +      * +----------+  +---------+  +-----------+  +---------_+
> > +      * ^             ^            ^              ^
> > +      * |             |            |              |
> > +      * hdr           |            |              |
> > +      * types_data----+            |              |
> > +      * strset__data(strs_set)-----+              |
> > +      * meta_data---------------------------------+
> >        *
> >        *               +----------+---------+-----------+
> >        *               |  Header  |  Types  |  Strings  |
> > @@ -116,6 +121,8 @@ struct btf {
> >       /* whether strings are already deduplicated */
> >       bool strs_deduped;
> >
> > +     void *meta_data;
> > +
> >       /* BTF object FD, if loaded into kernel */
> >       int fd;
> >
> > @@ -215,6 +222,11 @@ static void btf_bswap_hdr(struct btf_header *h)
> >       h->type_len = bswap_32(h->type_len);
> >       h->str_off = bswap_32(h->str_off);
> >       h->str_len = bswap_32(h->str_len);
> > +     if (h->hdr_len >= sizeof(struct btf_header)) {
> > +             h->meta_header.meta_off = bswap_32(h->meta_header.meta_off);
> > +             h->meta_header.meta_len = bswap_32(h->meta_header.meta_len);
> > +     }
> > +
> >  }
> >
> >  static int btf_parse_hdr(struct btf *btf)
> > @@ -222,14 +234,17 @@ static int btf_parse_hdr(struct btf *btf)
> >       struct btf_header *hdr = btf->hdr;
> >       __u32 meta_left;
> >
> > -     if (btf->raw_size < sizeof(struct btf_header)) {
> > +     if (btf->raw_size < sizeof(struct btf_header) - sizeof(struct btf_meta_header)) {
> >               pr_debug("BTF header not found\n");
> >               return -EINVAL;
> >       }
> >
> >       if (hdr->magic == bswap_16(BTF_MAGIC)) {
> > +             int swapped_len = bswap_32(hdr->hdr_len);
> > +
> >               btf->swapped_endian = true;
> > -             if (bswap_32(hdr->hdr_len) != sizeof(struct btf_header)) {
> > +             if (swapped_len != sizeof(struct btf_header) &&
> > +                 swapped_len != sizeof(struct btf_header) - sizeof(struct btf_meta_header)) {
> >                       pr_warn("Can't load BTF with non-native endianness due to unsupported header length %u\n",
> >                               bswap_32(hdr->hdr_len));
> >                       return -ENOTSUP;
> > @@ -285,6 +300,42 @@ static int btf_parse_str_sec(struct btf *btf)
> >       return 0;
> >  }
> >
> > +static void btf_bswap_meta(struct btf_metadata *meta, int len)
> > +{
> > +     struct btf_kind_meta *m = &meta->kind_meta[0];
> > +     struct btf_kind_meta *end = (void *)meta + len;
> > +
> > +     meta->flags = bswap_32(meta->flags);
> > +     meta->crc = bswap_32(meta->crc);
> > +     meta->base_crc = bswap_32(meta->base_crc);
> > +     meta->description_off = bswap_32(meta->description_off);
> > +
> > +     while (m < end) {
> > +             m->name_off = bswap_32(m->name_off);
> > +             m->flags = bswap_16(m->flags);
> > +             m++;
> > +     }
> > +}
> > +
> > +static int btf_parse_meta_sec(struct btf *btf)
> > +{
> > +     const struct btf_header *hdr = btf->hdr;
> > +
> > +     if (hdr->hdr_len < sizeof(struct btf_header) ||
> > +         !hdr->meta_header.meta_off || !hdr->meta_header.meta_len)
> > +             return 0;
>
> I'm trying to figure out how is the meta data optional, and it seems to be
> in here, right? but hdr->meta_header.meta_off or hdr->meta_header.meta_len
> must be set NULL or zero
>
> I'm getting crash when running btf test and it seems like correption when
> parsing BTF generated from clang, which does not have this meta support
>
> we do need clang support for this right? or bump version?

libbpf doesn't enforce that btf_header is all zeroes after last known
field (which until now was str_len). So the whole design of adding
fields to btf_header is backwards compatible from libbpf standpoint.

From the implementation standpoint, though, we should make our lives
simpler by not using btf->raw_data for btf_header, and instead do what
we do in kernel with bpf_attr. That is, have a local zero-initialized
copy, and overwrite it with data found in btf.


>
> thanks,
> jirka
>
>
> > +     if (hdr->meta_header.meta_len < sizeof(struct btf_metadata)) {
> > +             pr_debug("Invalid BTF metadata section\n");
> > +             return -EINVAL;
> > +     }
> > +     btf->meta_data = btf->raw_data + btf->hdr->hdr_len + btf->hdr->meta_header.meta_off;
> > +
> > +     if (btf->swapped_endian)
> > +             btf_bswap_meta(btf->meta_data, hdr->meta_header.meta_len);
> > +
> > +     return 0;
> > +}
> > +
> >  static int btf_type_size(const struct btf_type *t)
> >  {
> >       const int base_size = sizeof(struct btf_type);
> > @@ -904,6 +955,7 @@ static struct btf *btf_new(const void *data, __u32 size, struct btf *base_btf)
> >       err = err ?: btf_parse_type_sec(btf);
> >       if (err)
> >               goto done;
> > +     err = btf_parse_meta_sec(btf);
> >
> >  done:
> >       if (err) {
> > @@ -1267,6 +1319,11 @@ static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi
> >       }
> >
> >       data_sz = hdr->hdr_len + hdr->type_len + hdr->str_len;
> > +     if (btf->meta_data) {
> > +             data_sz = roundup(data_sz, 8);
> > +             data_sz += hdr->meta_header.meta_len;
> > +             hdr->meta_header.meta_off = roundup(hdr->type_len + hdr->str_len, 8);
> > +     }
> >       data = calloc(1, data_sz);
> >       if (!data)
> >               return NULL;
> > @@ -1293,8 +1350,21 @@ static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi
> >       p += hdr->type_len;
> >
> >       memcpy(p, btf_strs_data(btf), hdr->str_len);
> > -     p += hdr->str_len;
> > +     /* round up to 8 byte alignment to match offset above */
> > +     p = data + hdr->hdr_len + roundup(hdr->type_len + hdr->str_len, 8);
> > +
> > +     if (btf->meta_data) {
> > +             struct btf_metadata *meta = p;
> >
> > +             memcpy(p, btf->meta_data, hdr->meta_header.meta_len);
> > +             if (!swap_endian) {
> > +                     meta->crc = crc32(0L, (const Bytef *)&data, sizeof(data));
> > +                     meta->flags |= BTF_META_CRC_SET;
> > +             }
> > +             if (swap_endian)
> > +                     btf_bswap_meta(p, hdr->meta_header.meta_len);
> > +             p += hdr->meta_header.meta_len;
> > +     }
> >       *size = data_sz;
> >       return data;
> >  err_out:
> > @@ -1425,13 +1495,13 @@ static void btf_invalidate_raw_data(struct btf *btf)
> >       }
> >  }
> >
> > -/* Ensure BTF is ready to be modified (by splitting into a three memory
> > - * regions for header, types, and strings). Also invalidate cached
> > - * raw_data, if any.
> > +/* Ensure BTF is ready to be modified (by splitting into a three or four memory
> > + * regions for header, types, strings and optional metadata). Also invalidate
> > + * cached raw_data, if any.
> >   */
> >  static int btf_ensure_modifiable(struct btf *btf)
> >  {
> > -     void *hdr, *types;
> > +     void *hdr, *types, *meta = NULL;
> >       struct strset *set = NULL;
> >       int err = -ENOMEM;
> >
> > @@ -1446,9 +1516,17 @@ static int btf_ensure_modifiable(struct btf *btf)
> >       types = malloc(btf->hdr->type_len);
> >       if (!hdr || !types)
> >               goto err_out;
> > +     if (btf->hdr->hdr_len >= sizeof(struct btf_header)  &&
> > +         btf->hdr->meta_header.meta_off && btf->hdr->meta_header.meta_len) {
> > +             meta = calloc(1, btf->hdr->meta_header.meta_len);
> > +             if (!meta)
> > +                     goto err_out;
> > +     }
> >
> >       memcpy(hdr, btf->hdr, btf->hdr->hdr_len);
> >       memcpy(types, btf->types_data, btf->hdr->type_len);
> > +     if (meta)
> > +             memcpy(meta, btf->meta_data, btf->hdr->meta_header.meta_len);
> >
> >       /* build lookup index for all strings */
> >       set = strset__new(BTF_MAX_STR_OFFSET, btf->strs_data, btf->hdr->str_len);
> > @@ -1463,6 +1541,8 @@ static int btf_ensure_modifiable(struct btf *btf)
> >       btf->types_data_cap = btf->hdr->type_len;
> >       btf->strs_data = NULL;
> >       btf->strs_set = set;
> > +     btf->meta_data = meta;
> > +
> >       /* if BTF was created from scratch, all strings are guaranteed to be
> >        * unique and deduplicated
> >        */
> > @@ -1480,6 +1560,7 @@ static int btf_ensure_modifiable(struct btf *btf)
> >       strset__free(set);
> >       free(hdr);
> >       free(types);
> > +     free(meta);
> >       return err;
> >  }
> >
> > --
> > 2.31.1
> >