On Mon, Nov 2, 2020 at 3:24 PM Song Liu <songliubraving@xxxxxx> wrote:
>
>
>
> > On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
>
> [...]
>
> >
> > BTF deduplication is not yet supported for split BTF and support for it will
> > be added in separate patch.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Acked-by: Song Liu <songliubraving@xxxxxx>
>
> With a couple nits:
>
> > ---
> > tools/lib/bpf/btf.c | 205 ++++++++++++++++++++++++++++++---------
> > tools/lib/bpf/btf.h | 8 ++
> > tools/lib/bpf/libbpf.map | 9 ++
> > 3 files changed, 175 insertions(+), 47 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index db9331fea672..20c64a8441a8 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -78,10 +78,32 @@ struct btf {
> > void *types_data;
> > size_t types_data_cap; /* used size stored in hdr->type_len */
> >
> > - /* type ID to `struct btf_type *` lookup index */
> > + /* type ID to `struct btf_type *` lookup index
> > + * type_offs[0] corresponds to the first non-VOID type:
> > + * - for base BTF it's type [1];
> > + * - for split BTF it's the first non-base BTF type.
> > + */
> > __u32 *type_offs;
> > size_t type_offs_cap;
> > + /* number of types in this BTF instance:
> > + * - doesn't include special [0] void type;
> > + * - for split BTF counts number of types added on top of base BTF.
> > + */
> > __u32 nr_types;
>
> This is a little confusing. Maybe add a void type for every split BTF?
Agree about being a bit confusing. But I don't want VOID in every BTF,
that seems sloppy (there's no continuity). I'm currently doing similar
changes on kernel side, and so far everything also works cleanly with
start_id == 0 && nr_types including VOID (for base BTF), and start_id
== base_btf->nr_type && nr_types has all the added types (for split
BTF). That seems a bit more straightforward, so I'll probably do that
here as well (unless I'm missing something, I'll double check).
>
> > + /* if not NULL, points to the base BTF on top of which the current
> > + * split BTF is based
> > + */
>
> [...]
>
> >
> > @@ -252,12 +274,20 @@ static int btf_parse_str_sec(struct btf *btf)
> > const char *start = btf->strs_data;
> > const char *end = start + btf->hdr->str_len;
> >
> > - if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET ||
> > - start[0] || end[-1]) {
> > - pr_debug("Invalid BTF string section\n");
> > - return -EINVAL;
> > + if (btf->base_btf) {
> > + if (hdr->str_len == 0)
> > + return 0;
> > + if (hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1]) {
> > + pr_debug("Invalid BTF string section\n");
> > + return -EINVAL;
> > + }
> > + } else {
> > + if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET ||
> > + start[0] || end[-1]) {
> > + pr_debug("Invalid BTF string section\n");
> > + return -EINVAL;
> > + }
> > }
> > -
> > return 0;
>
> I found this function a little difficult to follow. Maybe rearrange it as
>
> /* too long, or not \0 terminated */
> if (hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1])
> goto err_out;
this won't work, if str_len == 0. Both str_len - 1 will underflow, and
end[-1] will be reading garbage
How about this:
if (btf->base_btf && hdr->str_len == 0)
return 0;
if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1])
return -EINVAL;
if (!btf->base_btf && start[0])
return -EINVAL;
return 0;
This seems more straightforward, right?
>
> /* for base btf, .... */
> if (!btf->base_btf && (!hdr->str_len || start[0]))
> goto err_out;
>
> return 0;
> err_out:
> pr_debug("Invalid BTF string section\n");
> return -EINVAL;
> }
> > }
> >
> > @@ -372,19 +402,9 @@ static int btf_parse_type_sec(struct btf *btf)
> > struct btf_header *hdr = btf->hdr;
> > void *next_type = btf->types_data;
> > void *end_type = next_type + hdr->type_len;
> > - int err, i = 0, type_size;
>
> [...]
>
