Re: [PATCH bpf-next v2 08/18] libbpf: Add enum64 sanitization

[Yonghong Song <yhs@fb.com>]

On 5/17/22 4:25 PM, Andrii Nakryiko wrote:
> On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> When old kernel does not support enum64 but user space btf
>> contains non-zero enum kflag or enum64, libbpf needs to
>> do proper sanitization so modified btf can be accepted
>> by the kernel.
>>
>> Sanitization for enum kflag can be achieved by clearing
>> the kflag bit. For enum64, the type is replaced with an
>> union of integer member types and the integer member size
>> must be smaller than enum64 size. If such an integer
>> type cannot be found, a new type is created and used
>> for union members.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/btf.h             |  3 +-
>>   tools/lib/bpf/libbpf.c          | 53 +++++++++++++++++++++++++++++++--
>>   tools/lib/bpf/libbpf_internal.h |  2 ++
>>   3 files changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
>> index 7da6970b8c9f..d4fe1300ed33 100644
>> --- a/tools/lib/bpf/btf.h
>> +++ b/tools/lib/bpf/btf.h
>> @@ -395,9 +395,10 @@ btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
>>   #ifndef BTF_KIND_FLOAT
>>   #define BTF_KIND_FLOAT         16      /* Floating point       */
>>   #endif
>> -/* The kernel header switched to enums, so these two were never #defined */
>> +/* The kernel header switched to enums, so the following were never #defined */
>>   #define BTF_KIND_DECL_TAG      17      /* Decl Tag */
>>   #define BTF_KIND_TYPE_TAG      18      /* Type Tag */
>> +#define BTF_KIND_ENUM64                19      /* Enum for up-to 64bit values */
>>
>>   static inline __u16 btf_kind(const struct btf_type *t)
>>   {
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 4867a930628b..f54e70b9953d 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -2114,6 +2114,7 @@ static const char *__btf_kind_str(__u16 kind)
>>          case BTF_KIND_FLOAT: return "float";
>>          case BTF_KIND_DECL_TAG: return "decl_tag";
>>          case BTF_KIND_TYPE_TAG: return "type_tag";
>> +       case BTF_KIND_ENUM64: return "enum64";
>>          default: return "unknown";
>>          }
>>   }
>> @@ -2642,9 +2643,10 @@ static bool btf_needs_sanitization(struct bpf_object *obj)
>>          bool has_func = kernel_supports(obj, FEAT_BTF_FUNC);
>>          bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
>>          bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
>> +       bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
>>
>>          return !has_func || !has_datasec || !has_func_global || !has_float ||
>> -              !has_decl_tag || !has_type_tag;
>> +              !has_decl_tag || !has_type_tag || !has_enum64;
>>   }
>>
>>   static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>> @@ -2655,9 +2657,25 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>>          bool has_func = kernel_supports(obj, FEAT_BTF_FUNC);
>>          bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
>>          bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
>> +       bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
>> +       int min_int_size = 32, min_enum64_size = 32, min_int_tid = 0;
>>          struct btf_type *t;
>>          int i, j, vlen;
>>
>> +       if (!has_enum64) {
>> +               for (i = 1; i < btf__type_cnt(btf); i++) {
>> +                       t = (struct btf_type *)btf__type_by_id(btf, i);
>> +                       if (btf_is_int(t) && t->size < min_int_size) {
>> +                               min_int_size = t->size;
>> +                               min_int_tid = i;
>> +                       } else if (btf_is_enum64(t) && t->size < min_enum64_size) {
>> +                               min_enum64_size = t->size;
>> +                       }
>> +               }
>> +               if (min_int_size > min_enum64_size)
>> +                       min_int_tid = btf__add_int(btf, "char", 1,  BTF_INT_SIGNED);
>> +       }
>> +
> 
> we do this search even if bpf_object's BTF doesn't have enum64, which
> seems overly pessimistic. How about we just lazily (but
> unconditionally) add new BTF_KIND_INT on first encountered enum64 and
> remember it's id (see below)
> 
>>          for (i = 1; i < btf__type_cnt(btf); i++) {
>>                  t = (struct btf_type *)btf__type_by_id(btf, i);
>>
>> @@ -2717,7 +2735,20 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>>                          /* replace TYPE_TAG with a CONST */
>>                          t->name_off = 0;
>>                          t->info = BTF_INFO_ENC(BTF_KIND_CONST, 0, 0);
>> -               }
>> +               } else if (!has_enum64 && btf_is_enum(t)) {
>> +                       /* clear the kflag */
>> +                       t->info = btf_type_info(btf_kind(t), btf_vlen(t), false);
>> +               } else if (!has_enum64 && btf_is_enum64(t)) {
>> +                       /* replace ENUM64 with a union */
>> +                       struct btf_member *m = btf_members(t);
>> +
> 
> so here we just
> 
> if (enum64_placeholder_id == 0) {
>      enum64_placeholder_id = btf__add_int(btf, "enum64_placeholder", t->size, 0);
>      if (enum64_placeholder_id < 0) /* pr_warn and exit with error */
> }
> 
> and then just use enum64_placeholder_id for each field type?
> 
> It seems much simpler than trying to find matching int (especially
> given potentially non-8-byte size), so it seems better to just add our
> own type.

Besides this implementation, I had another idea to just add one
int type unconditionally for enum64->union member type. To avoid adding
this type unconditionally, I added the search part.

Let me try the approach to add the new int type in the fly when
needed.

> 
> Please make sure to re-initialize t and m after that because
> btf__add_int() invalidates underlying memory, so you need to re-fetch
> btf__type_by_id().
> 
>> +                       vlen = btf_vlen(t);
>> +                       t->info = BTF_INFO_ENC(BTF_KIND_UNION, 0, vlen);
>> +                       for (j = 0; j < vlen; j++, m++) {
>> +                               m->type = min_int_tid;
>> +                               m->offset = 0;
>> +                       }
>> +                }
>>          }
>>   }
>>
[...]