Re: [PATCH bpf-next 5/9] libbpf: allow modification of BTF and add btf__add_str API

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

On Thu, Sep 24, 2020 at 8:56 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > Allow internal BTF representation to switch from default read-only mode, in
> > which raw BTF data is a single non-modifiable block of memory with BTF header,
> > types, and strings layed out sequentially and contiguously in memory, into
> > a writable representation with types and strings data split out into separate
> > memory regions, that can be dynamically expanded.
> >
> > Such writable internal representation is transparent to users of libbpf APIs,
> > but allows to append new types and strings at the end of BTF, which is
> > a typical use case when generating BTF programmatically. All the basic
> > guarantees of BTF types and strings layout is preserved, i.e., user can get
> > `struct btf_type *` pointer and read it directly. Such btf_type pointers might
> > be invalidated if BTF is modified, so some care is required in such mixed
> > read/write scenarios.
> >
> > Switch from read-only to writable configuration happens automatically the
> > first time when user attempts to modify BTF by either adding a new type or new
> > string. It is still possible to get raw BTF data, which is a single piece of
> > memory that can be persisted in ELF section or into a file as raw BTF. Such
> > raw data memory is also still owned by BTF and will be freed either when BTF
> > object is freed or if another modification to BTF happens, as any modification
> > invalidates BTF raw representation.
> >
> > This patch adds the first BTF writing API: btf__add_str(), which allows to
> > add arbitrary strings to BTF string section. All the added strings are
> > automatically deduplicated. This is achieved by maintaining an additional
> > string lookup index for all unique strings. Such index is built when BTF is
> > switched to modifiable mode. If at that time BTF strings section contained
> > duplicate strings, they are not de-duplicated. This is done specifically to
> > not modify the existing content of BTF (types, their string offsets, etc),
> > which can cause confusion and is especially important property if there is
> > struct btf_ext associated with struct btf. By following this "imperfect
> > deduplication" process, btf_ext is kept consitent and correct. If
> > deduplication of strings is necessary, it can be forced by doing BTF
> > deduplication, at which point all the strings will be eagerly deduplicated and
> > all string offsets both in struct btf and struct btf_ext will be updated.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
>
> [...]
>
> > +/* 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.
> > + */
> > +static int btf_ensure_modifiable(struct btf *btf)
> > +{
> > +     void *hdr, *types, *strs, *strs_end, *s;
> > +     struct hashmap *hash = NULL;
> > +     long off;
> > +     int err;
> > +
> > +     if (btf_is_modifiable(btf)) {
> > +             /* any BTF modification invalidates raw_data */
> > +             if (btf->raw_data) {
>
> I missed why this case is needed? Just being cautious? It looks like
> we get btf->hdr != btf->raw_data (aka btf_is_modifiable) below, but
> by the tiime we do this set it looks like we will always null btf->raw_data
> as well. Again doesn't appear harmful just seeing if I missed a path.

It's because of btf__get_raw_data() (it's currently used by pahole for
BTF dedup). raw_data is cached in struct btf and is owned by it, so
when we attempt modification, we have to invalidate a single-blob
representation, as it is immediately invalid. This is mostly to
preserve existing semantics, but also not to keep allocating new
memory if caller created BTF and then accesses raw_data few times.

>
> > +                     free(btf->raw_data);
> > +                     btf->raw_data = NULL;
> > +             }
> > +             return 0;
> > +     }
> > +
> > +     /* split raw data into three memory regions */
> > +     hdr = malloc(btf->hdr->hdr_len);
> > +     types = malloc(btf->hdr->type_len);
> > +     strs = malloc(btf->hdr->str_len);
> > +     if (!hdr || !types || !strs)
> > +             goto err_out;
> > +
> > +     memcpy(hdr, btf->hdr, btf->hdr->hdr_len);
> > +     memcpy(types, btf->types_data, btf->hdr->type_len);
> > +     memcpy(strs, btf->strs_data, btf->hdr->str_len);
> > +
> > +     /* build lookup index for all strings */
> > +     hash = hashmap__new(strs_hash_fn, strs_hash_equal_fn, btf);
> > +     if (IS_ERR(hash)) {
> > +             err = PTR_ERR(hash);
> > +             hash = NULL;
> > +             goto err_out;
> > +     }
> > +
>
> [...]
>
> Thanks,
> John