Re: [PATCH v2 bpf-next 5/9] libbpf: add kind layout encoding, crc support

[Yonghong Song <yhs@meta.com>]

On 6/20/23 2:09 AM, Alan Maguire wrote:
> On 20/06/2023 00:24, Yonghong Song wrote:
>>
>>
>> On 6/16/23 10:17 AM, Alan Maguire wrote:
>>> Support encoding of BTF kind layout data and crcs via
>>> btf__new_empty_opts().
>>>
>>> Current supported opts are base_btf, add_kind_layout and
>>> add_crc.
>>>
>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>> ---
>>>    tools/lib/bpf/btf.c      | 99 ++++++++++++++++++++++++++++++++++++++--
>>>    tools/lib/bpf/btf.h      | 11 +++++
>>>    tools/lib/bpf/libbpf.map |  1 +
>>>    3 files changed, 108 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>> index 457997c2a43c..060a93809f64 100644
>>> --- a/tools/lib/bpf/btf.c
>>> +++ b/tools/lib/bpf/btf.c
>>> @@ -16,6 +16,7 @@
>>>    #include <linux/err.h>
>>>    #include <linux/btf.h>
>>>    #include <gelf.h>
>>> +#include <zlib.h>
>>>    #include "btf.h"
>>>    #include "bpf.h"
>>>    #include "libbpf.h"
>>> @@ -882,8 +883,58 @@ void btf__free(struct btf *btf)
>>>        free(btf);
>>>    }
>>>    -static struct btf *btf_new_empty(struct btf *base_btf)
>>> +static void btf_add_kind_layout(struct btf *btf, __u8 kind,
>>> +                __u16 flags, __u8 info_sz, __u8 elem_sz)
>>>    {
>>> +    struct btf_kind_layout *k = &btf->kind_layout[kind];
>>> +
>>> +    k->flags = flags;
>>> +    k->info_sz = info_sz;
>>> +    k->elem_sz = elem_sz;
>>> +    btf->hdr->kind_layout_len += sizeof(*k);
>>> +}
>>> +
>>> +static int btf_ensure_modifiable(struct btf *btf);
>>> +
>>> +static int btf_add_kind_layouts(struct btf *btf, struct btf_new_opts
>>> *opts)
>>> +{
>>> +    if (btf_ensure_modifiable(btf))
>>> +        return libbpf_err(-ENOMEM);
>>> +
>>> +    btf->kind_layout = calloc(NR_BTF_KINDS, sizeof(struct
>>> btf_kind_layout));
>>> +
>>> +    if (!btf->kind_layout)
>>> +        return -ENOMEM;
>>> +
>>> +    /* all supported kinds should describe their layout here. */
>>> +    btf_add_kind_layout(btf, BTF_KIND_UNKN, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_INT, 0, sizeof(__u32), 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_PTR, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_ARRAY, 0, sizeof(struct
>>> btf_array), 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_STRUCT, 0, 0, sizeof(struct
>>> btf_member));
>>> +    btf_add_kind_layout(btf, BTF_KIND_UNION, 0, 0, sizeof(struct
>>> btf_member));
>>> +    btf_add_kind_layout(btf, BTF_KIND_ENUM, 0, 0, sizeof(struct
>>> btf_enum));
>>> +    btf_add_kind_layout(btf, BTF_KIND_FWD, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_TYPEDEF, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_VOLATILE, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_CONST, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_RESTRICT, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_FUNC, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_FUNC_PROTO, 0, 0, sizeof(struct
>>> btf_param));
>>> +    btf_add_kind_layout(btf, BTF_KIND_VAR, 0, sizeof(struct btf_var),
>>> 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_DATASEC, 0, 0, sizeof(struct
>>> btf_var_secinfo));
>>> +    btf_add_kind_layout(btf, BTF_KIND_FLOAT, 0, 0, 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_DECL_TAG,
>>> BTF_KIND_LAYOUT_OPTIONAL,
>>> +                            sizeof(struct btf_decl_tag), 0);
>>> +    btf_add_kind_layout(btf, BTF_KIND_TYPE_TAG,
>>> BTF_KIND_LAYOUT_OPTIONAL, 0, 0);
>>
>> BTF_KIND_TYPE_TAG cannot be optional. For example,
>>    ptr -> type_tag -> const -> int
>>
>> if type_tag becomes optional, the whole type chain cannot be parsed
>> properly.
>>
> 
> Ah, I missed that, thanks! You're absolutely right.
> 
> There are two separate concerns I think:
> 
> 1. if an unknown kind (unknown to libbpf/kernel but present in the kind
>     layout) is ever pointed at by another kind, regardless of optional
>     status, the BTF must be rejected on the grounds that we don't really
>     have a way to understand what the BTF means. That catches the case
>     above.
> 2. however if an unknown kind exists in BTF and _is_ optional _and_
>     is not pointed at by any known kinds, that is fine.
> 
> In other words it's logically possible for us to want to either
> accept or reject BTF when we encounter unknown kinds that fall outside
> of the existing type graph relations; the optional flag tells us which
> to do.
> 
> I think for meta checking, the right way to handle 1 is to
> reject BTF in the kind-specific meta checking for any known
> kinds that can refer to other kinds; if the kind referred to
> is > KIND_MAX, we reject the BTF.

I agree with your above analysis.

> 
>> Also, in Patch 3, we have
>>
>> +static int btf_type_size(const struct btf *btf, const struct btf_type *t)
>>   {
>>       const int base_size = sizeof(struct btf_type);
>>       __u16 vlen = btf_vlen(t);
>> @@ -363,8 +391,7 @@ static int btf_type_size(const struct btf_type *t)
>>       case BTF_KIND_DECL_TAG:
>>           return base_size + sizeof(struct btf_decl_tag);
>>       default:
>> -        pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
>> -        return -EINVAL;
>> +        return btf_type_size_unknown(btf, t);
>>       }
>>   }
>>
>> Clearly even if we mark decl_tag as optional, it still handled properly
>> in the above. So decl_tag does not need BTF_KIND_LAYOUT_OPTIONAL, right?
>>
> But in btf_type_size_unknown() we have:
> 
>         if (!(k->flags & BTF_KIND_LAYOUT_OPTIONAL)) {
>                  /* a required kind, and we do not know about it.. */
>                  pr_debug("unknown but required kind: %u\n", kind);
>                  return -EINVAL;
>          }
> 
> The problem however I think is that we need to spot reference
> types that refer to unknown kinds regardless of optional status
> as described above.

What I mean is there is no need to tag decl_tag with
BTF_KIND_LAYOUT_OPTIONAL since for any btf with kind_layout,
decl_tag is already there. But BTF_KIND_LAYOUT_OPTIONAL is
still necessary for future kinds.

> 
>> I guess what we really want to test is in the selftest:
>>    - Add a couple of new kinds for testing purpose, e.g.,
>>        BTF_KIND_OPTIONAL, BTF_KIND_NOT_OPTIONAL,
>>      generate two btf's which uses BTF_KIND_OPTIONAL
>>      and BTF_KIND_NOT_OPTIONAL respectively.
>>    - test these two btf's with this patch set to see whether it
>>      works as expected or not.
>>
>> Does this make sense?
>>
> 
> There's a test that does this currently for libbpf only (since we
> need a struct btf to load into the kernel), but nothing to cover the
> case where a reference type points at a kind we don't know about.
> I'll update the tests to use reference types too.

Sounds good. thanks for adding additional tests.

> 
> Thanks!
> 
> Alan
> 
>>> +    btf_add_kind_layout(btf, BTF_KIND_ENUM64, 0, 0, sizeof(struct
>>> btf_enum64));
>>> +
>>> +    return 0;
>>> +}
>>> +
>> [...]