> On Nov 2, 2020, at 9:02 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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).
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.
