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

[Alan Maguire <alan.maguire@oracle.com>]

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.

> 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.

> 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.

Thanks!

Alan

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