Re: [PATCH bpf-next 04/11] libbpf: implement basic split BTF support

[Song Liu <songliubraving@fb.com>]

> On Nov 2, 2020, at 9:02 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Mon, Nov 2, 2020 at 3:24 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@kernel.org> 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@kernel.org>
>> 
>> Acked-by: Song Liu <songliubraving@fb.com>
>> 
>> 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).

That sounds good. 

> 
>> 
>>> +     /* 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?

Yeah, I like this version. BTW, short comment for each condition will be
helpful.