Re: [PATCH v2 bpf-next 2/9] libbpf: support handling of kind layout section in BTF

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

On Fri, Jun 16, 2023 at 10:17 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> support reading in kind layout fixing endian issues on reading;
> also support writing kind layout section to raw BTF object.
> There is not yet an API to populate the kind layout with meaningful
> information.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf.c | 132 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 99 insertions(+), 33 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 8484b563b53d..f9f919fdc728 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -39,40 +39,44 @@ struct btf {
>

[...]

>         struct btf_header *hdr;
>
> @@ -116,6 +120,8 @@ struct btf {
>         /* whether strings are already deduplicated */
>         bool strs_deduped;
>
> +       struct btf_kind_layout *kind_layout;
> +
>         /* BTF object FD, if loaded into kernel */
>         int fd;
>
> @@ -215,6 +221,13 @@ 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->kind_layout_off = bswap_32(h->kind_layout_off);
> +               h->kind_layout_len = bswap_32(h->kind_layout_len);
> +               h->crc = bswap_32(h->crc);
> +               h->base_crc = bswap_32(h->base_crc);
> +       }
> +

nit: unnecessary empty line

>  }
>
>  static int btf_parse_hdr(struct btf *btf)
> @@ -222,14 +235,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 < BTF_HEADER_MIN_LEN) {
>                 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 != BTF_HEADER_MIN_LEN) {
>                         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 +301,32 @@ static int btf_parse_str_sec(struct btf *btf)
>         return 0;
>  }
>
> +static void btf_bswap_kind_layout_sec(struct btf_kind_layout *k, int len)
> +{
> +       struct btf_kind_layout *end = (void *)k + len;
> +
> +       while (k < end) {
> +               k->flags = bswap_16(k->flags);
> +               k++;
> +       }
> +}
> +
> +static int btf_parse_kind_layout_sec(struct btf *btf)
> +{
> +       const struct btf_header *hdr = btf->hdr;
> +
> +       if (hdr->hdr_len < sizeof(struct btf_header) ||
> +           !hdr->kind_layout_off || !hdr->kind_layout_len)
> +               return 0;

instead of having to remember to check `hdr->hdr_len < sizeof(struct
btf_header)` before accessing kind_layout_off and kind_layout_len,
let's just allocate a copy of full btf_header on initialization, copy
min(sizeof(struct btf_header), hdr->len) into it, and then point
btf->hdr to this copy everywhere?

> +       if (hdr->kind_layout_len < sizeof(struct btf_kind_layout)) {

shouldn't it be the check that hdr->kind_layout_len is a multiple of
sizeof(struct btf_kind_layout) ?

> +               pr_debug("Invalid BTF kind layout section\n");
> +               return -EINVAL;
> +       }
> +       btf->kind_layout = btf->raw_data + btf->hdr->hdr_len + btf->hdr->kind_layout_off;
> +
> +       return 0;
> +}
> +
>  static int btf_type_size(const struct btf_type *t)
>  {
>         const int base_size = sizeof(struct btf_type);
> @@ -901,6 +943,7 @@ static struct btf *btf_new(const void *data, __u32 size, struct btf *base_btf)
>         btf->types_data = btf->raw_data + btf->hdr->hdr_len + btf->hdr->type_off;
>
>         err = btf_parse_str_sec(btf);
> +       err = err ?: btf_parse_kind_layout_sec(btf);
>         err = err ?: btf_parse_type_sec(btf);
>         if (err)
>                 goto done;
> @@ -1267,6 +1310,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->kind_layout) {
> +               data_sz = roundup(data_sz, 4);
> +               data_sz += hdr->kind_layout_len;
> +               hdr->kind_layout_off = roundup(hdr->type_len + hdr->str_len, 4);

can we avoid modifying hdr here? we expect const struct btf *, so it's
a bit iffy that we are adjusting header here

maybe we can just make sure that kind_layout_off is always maintained correctly?

> +       }
>         data = calloc(1, data_sz);
>         if (!data)
>                 return NULL;
> @@ -1293,8 +1341,15 @@ 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 4 byte alignment to match offset above */
> +       p = data + hdr->hdr_len + roundup(hdr->type_len + hdr->str_len, 4);

instead of reimplementing roundup logic, why not just use
hdr->kind_layout_off here?

>
> +       if (btf->kind_layout) {
> +               memcpy(p, btf->kind_layout, hdr->kind_layout_len);
> +               if (swap_endian)
> +                       btf_bswap_kind_layout_sec(p, hdr->kind_layout_len);
> +               p += hdr->kind_layout_len;
> +       }
>         *size = data_sz;
>         return data;
>  err_out:
> @@ -1425,13 +1480,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

nit: gmail suggests that "a" is not necessary before "three" here

> + * regions for header, types, strings and optional kind layout). Also invalidate
> + * cached raw_data, if any.
>   */
>  static int btf_ensure_modifiable(struct btf *btf)
>  {
> -       void *hdr, *types;
> +       void *hdr, *types, *kind_layout = NULL;
>         struct strset *set = NULL;
>         int err = -ENOMEM;
>
> @@ -1446,9 +1501,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->kind_layout_off && btf->hdr->kind_layout_len) {
> +               kind_layout = calloc(1, btf->hdr->kind_layout_len);
> +               if (!kind_layout)
> +                       goto err_out;
> +       }
>
>         memcpy(hdr, btf->hdr, btf->hdr->hdr_len);
>         memcpy(types, btf->types_data, btf->hdr->type_len);
> +       if (kind_layout)
> +               memcpy(kind_layout, btf->kind_layout, btf->hdr->kind_layout_len);
>

let's just emit kind_layout always, why making it optional on writing
out new BTF?

why not make it always present internally in libbpf, and either read
it from BTF, if it's present, or created it from scratch based on
libbpf's version and knowledge of all the kinds? This will be simpler,
the only place where we'd need to handle it optionally is during
initialization


>         /* build lookup index for all strings */
>         set = strset__new(BTF_MAX_STR_OFFSET, btf->strs_data, btf->hdr->str_len);
> @@ -1463,6 +1526,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->kind_layout = kind_layout;
> +
>         /* if BTF was created from scratch, all strings are guaranteed to be
>          * unique and deduplicated
>          */
> @@ -1480,6 +1545,7 @@ static int btf_ensure_modifiable(struct btf *btf)
>         strset__free(set);
>         free(hdr);
>         free(types);
> +       free(kind_layout);
>         return err;
>  }
>
> --
> 2.39.3
>