| Message ID | 20220501190007.2576808-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add 64bit enum value support | expand |
On 5/1/22 3:00 PM, Yonghong Song wrote: > Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM. > But in kernel, some enum indeed has 64bit values, e.g., > in uapi bpf.h, we have > enum { > BPF_F_INDEX_MASK = 0xffffffffULL, > BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK, > BPF_F_CTXLEN_MASK = (0xfffffULL << 32), > }; > In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK > as 0, which certainly is incorrect. > > This patch added a new btf kind, BTF_KIND_ENUM64, which permits > 64bit value to cover the above use case. The BTF_KIND_ENUM64 has > the following three bytes followed by the common type: > struct bpf_enum64 { > __u32 nume_off; > __u32 hi32; > __u32 lo32; > }; > Currently, btf type section has an alignment of 4 as all element types > are u32. Representing the value with __u64 will introduce a pad > for bpf_enum64 and may also introduce misalignment for the 64bit value. > Hence, two members of hi32 and lo32 are chosen to avoid these issues. > > The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64 > to indicate whether the value is signed or unsigned. The kflag intends > to provide consistent output of BTF C fortmat with the original > source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff. > The format C has two choices, print out 0xffffffff or -1 and current libbpf > prints out as unsigned value. But if the signedness is preserved in btf, > the value can be printed the same as the original source code. > > The new BTF_KIND_ENUM64 is intended to support the enum value represented as > 64bit value. But it can represent all BTF_KIND_ENUM values as well. > The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent. > The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has > to be represented with 64 bits. > > [1] https://reviews.llvm.org/D124641 > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > include/linux/btf.h | 18 ++++- > include/uapi/linux/btf.h | 17 ++++- > kernel/bpf/btf.c | 132 ++++++++++++++++++++++++++++++--- > tools/include/uapi/linux/btf.h | 17 ++++- > 4 files changed, 168 insertions(+), 16 deletions(-) [...] > diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h > index a9162a6c0284..2aac226a27b2 100644 > --- a/include/uapi/linux/btf.h > +++ b/include/uapi/linux/btf.h > @@ -36,10 +36,10 @@ struct btf_type { > * bits 24-28: kind (e.g. int, ptr, array...etc) > * bits 29-30: unused > * bit 31: kind_flag, currently used by > - * struct, union and fwd > + * struct, union, enum, fwd and enum64 > */ > __u32 info; > - /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC. > + /* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64. > * "size" tells the size of the type it is describing. > * > * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, > @@ -63,7 +63,7 @@ enum { > BTF_KIND_ARRAY = 3, /* Array */ > BTF_KIND_STRUCT = 4, /* Struct */ > BTF_KIND_UNION = 5, /* Union */ > - BTF_KIND_ENUM = 6, /* Enumeration */ > + BTF_KIND_ENUM = 6, /* Enumeration for int/unsigned int values */ nit: Maybe it would be more clear to say something like "Enumeration representable in <= 32 bits", and something similar for ENUM64? This applies to docs/bpf patch as well. I don't feel strongly about it. > BTF_KIND_FWD = 7, /* Forward */ > BTF_KIND_TYPEDEF = 8, /* Typedef */ > BTF_KIND_VOLATILE = 9, /* Volatile */ > @@ -76,6 +76,7 @@ enum { > BTF_KIND_FLOAT = 16, /* Floating point */ > BTF_KIND_DECL_TAG = 17, /* Decl Tag */ > BTF_KIND_TYPE_TAG = 18, /* Type Tag */ > + BTF_KIND_ENUM64 = 19, /* Enumeration for long/unsigned long values */ > > NR_BTF_KINDS, > BTF_KIND_MAX = NR_BTF_KINDS - 1, > @@ -186,4 +187,14 @@ struct btf_decl_tag { > __s32 component_idx; > }; [...] > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 2f0b0440131c..17e24b362d3d 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -307,6 +307,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = { > [BTF_KIND_FLOAT] = "FLOAT", > [BTF_KIND_DECL_TAG] = "DECL_TAG", > [BTF_KIND_TYPE_TAG] = "TYPE_TAG", > + [BTF_KIND_ENUM64] = "ENUM64", > }; > > const char *btf_type_str(const struct btf_type *t) > @@ -664,6 +665,7 @@ static bool btf_type_has_size(const struct btf_type *t) > case BTF_KIND_ENUM: > case BTF_KIND_DATASEC: > case BTF_KIND_FLOAT: > + case BTF_KIND_ENUM64: > return true; > } > [...] > @@ -1832,6 +1840,7 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type, > case BTF_KIND_UNION: > case BTF_KIND_ENUM: > case BTF_KIND_FLOAT: > + case BTF_KIND_ENUM64: > size = type->size; > goto resolved; Is it possible to replace this check w/ btf_type_has_size that you also updated? Looks like case's match, aside from BTF_KIND_DATASEC.
On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: > > Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM. > But in kernel, some enum indeed has 64bit values, e.g., > in uapi bpf.h, we have > enum { > BPF_F_INDEX_MASK = 0xffffffffULL, > BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK, > BPF_F_CTXLEN_MASK = (0xfffffULL << 32), > }; > In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK > as 0, which certainly is incorrect. > > This patch added a new btf kind, BTF_KIND_ENUM64, which permits > 64bit value to cover the above use case. The BTF_KIND_ENUM64 has > the following three bytes followed by the common type: you probably meant three fields, not bytes > struct bpf_enum64 { > __u32 nume_off; > __u32 hi32; > __u32 lo32; I'd like to nitpick on name here, as hi/lo of what? Maybe val_hi32 and val_lo32? Can we also reverse the order here? For x86 you'll be able to use &lo32 to get value directly if you really want, without a local copy. It also just logically seems better to have something low first, then high next. > }; > Currently, btf type section has an alignment of 4 as all element types > are u32. Representing the value with __u64 will introduce a pad > for bpf_enum64 and may also introduce misalignment for the 64bit value. > Hence, two members of hi32 and lo32 are chosen to avoid these issues. > > The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64 > to indicate whether the value is signed or unsigned. The kflag intends > to provide consistent output of BTF C fortmat with the original > source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff. > The format C has two choices, print out 0xffffffff or -1 and current libbpf > prints out as unsigned value. But if the signedness is preserved in btf, > the value can be printed the same as the original source code. > > The new BTF_KIND_ENUM64 is intended to support the enum value represented as > 64bit value. But it can represent all BTF_KIND_ENUM values as well. > The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent. > The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has > to be represented with 64 bits. > > [1] https://reviews.llvm.org/D124641 > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > include/linux/btf.h | 18 ++++- > include/uapi/linux/btf.h | 17 ++++- > kernel/bpf/btf.c | 132 ++++++++++++++++++++++++++++++--- > tools/include/uapi/linux/btf.h | 17 ++++- > 4 files changed, 168 insertions(+), 16 deletions(-) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 2611cea2c2b6..280c33c9414a 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -174,7 +174,8 @@ static inline bool btf_type_is_small_int(const struct btf_type *t) > > static inline bool btf_type_is_enum(const struct btf_type *t) > { > - return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM; > + return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM || > + BTF_INFO_KIND(t->info) == BTF_KIND_ENUM64; > } a bit hesitant about this change, there is no helper to check for ENUM vs ENUM64. Inside the kernel this change seems to be correct as we don't care, but for libbpf I'd probably keep btf_is_enum() unchanged (you can't really work with them in the same generic way in user-space, as their memory layout is very different, so it's better not to generalize them unnecessarily) > > static inline bool str_is_empty(const char *s) > @@ -192,6 +193,16 @@ static inline bool btf_is_enum(const struct btf_type *t) > return btf_kind(t) == BTF_KIND_ENUM; > } > > +static inline bool btf_is_enum64(const struct btf_type *t) > +{ > + return btf_kind(t) == BTF_KIND_ENUM64; > +} > + > +static inline u64 btf_enum64_value(const struct btf_enum64 *e) > +{ > + return (u64)e->hi32 << 32 | e->lo32; this might be correct but () around bit shift would make it more obvious > +} > + > static inline bool btf_is_composite(const struct btf_type *t) > { > u16 kind = btf_kind(t); > @@ -332,6 +343,11 @@ static inline struct btf_enum *btf_enum(const struct btf_type *t) > return (struct btf_enum *)(t + 1); > } > > +static inline struct btf_enum64 *btf_enum64(const struct btf_type *t) > +{ > + return (struct btf_enum64 *)(t + 1); > +} > + > static inline const struct btf_var_secinfo *btf_type_var_secinfo( > const struct btf_type *t) > { > diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h > index a9162a6c0284..2aac226a27b2 100644 > --- a/include/uapi/linux/btf.h > +++ b/include/uapi/linux/btf.h > @@ -36,10 +36,10 @@ struct btf_type { > * bits 24-28: kind (e.g. int, ptr, array...etc) > * bits 29-30: unused > * bit 31: kind_flag, currently used by > - * struct, union and fwd > + * struct, union, enum, fwd and enum64 see comment below on kflag for enum itself, but reading this I realized that we don't really describe the meaning of kind_flag for different kinds. Should it be done here? > */ > __u32 info; > - /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC. > + /* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64. > * "size" tells the size of the type it is describing. > * > * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, > @@ -63,7 +63,7 @@ enum { > BTF_KIND_ARRAY = 3, /* Array */ > BTF_KIND_STRUCT = 4, /* Struct */ > BTF_KIND_UNION = 5, /* Union */ > - BTF_KIND_ENUM = 6, /* Enumeration */ > + BTF_KIND_ENUM = 6, /* Enumeration for int/unsigned int values */ nit: "Enumeration for up to 32-bit values" ? > BTF_KIND_FWD = 7, /* Forward */ > BTF_KIND_TYPEDEF = 8, /* Typedef */ > BTF_KIND_VOLATILE = 9, /* Volatile */ > @@ -76,6 +76,7 @@ enum { > BTF_KIND_FLOAT = 16, /* Floating point */ > BTF_KIND_DECL_TAG = 17, /* Decl Tag */ > BTF_KIND_TYPE_TAG = 18, /* Type Tag */ > + BTF_KIND_ENUM64 = 19, /* Enumeration for long/unsigned long values */ and then "for 64-bit values" (or maybe up to 64 bit values, but in practice we won't do that, right?) > > NR_BTF_KINDS, > BTF_KIND_MAX = NR_BTF_KINDS - 1, > @@ -186,4 +187,14 @@ struct btf_decl_tag { > __s32 component_idx; > }; > [...] > @@ -3716,7 +3721,8 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env, > > if (env->log.level == BPF_LOG_KERNEL) > continue; > - btf_verifier_log(env, "\t%s val=%d\n", > + fmt_str = btf_type_kflag(t) ? "\t%s val=%u\n" : "\t%s val=%d\n"; > + btf_verifier_log(env, fmt_str, > __btf_name_by_offset(btf, enums[i].name_off), > enums[i].val); > } > @@ -3757,7 +3763,10 @@ static void btf_enum_show(const struct btf *btf, const struct btf_type *t, > return; > } > > - btf_show_type_value(show, "%d", v); > + if (btf_type_kflag(t)) libbpf's assumption right now and most common case is unsigned enum, so it seems more desirable to have kflag == 0 mean unsigned, with kflag == 1 being signed. It will make most existing enum definitions not change but also make it easy for libbpf's btf_dumper to use kflag if it's set, but otherwise have the same unsigned printing whether enum is really unsigned or Clang is too old to emit the kflag for enum. WDYT? > + btf_show_type_value(show, "%u", v); > + else > + btf_show_type_value(show, "%d", v); you didn't got with ternary operator for fmt string selector like in previous case? I have mild preference for keeping it consistent (and keeping btf_type_kflag(t) ? "fmt1" : "fmt2" inline) > btf_show_end_type(show); > } > > @@ -3770,6 +3779,109 @@ static struct btf_kind_operations enum_ops = { > .show = btf_enum_show, > }; > > +static s32 btf_enum64_check_meta(struct btf_verifier_env *env, > + const struct btf_type *t, > + u32 meta_left) > +{ > + const struct btf_enum64 *enums = btf_type_enum64(t); > + struct btf *btf = env->btf; > + const char *fmt_str; > + u16 i, nr_enums; > + u32 meta_needed; > + > + nr_enums = btf_type_vlen(t); > + meta_needed = nr_enums * sizeof(*enums); > + > + if (meta_left < meta_needed) { > + btf_verifier_log_basic(env, t, > + "meta_left:%u meta_needed:%u", > + meta_left, meta_needed); > + return -EINVAL; > + } > + > + if (t->size != 8) { technically there is nothing wrong with using enum64 for smaller sizes, right? Any particular reason to prevent this? We can just define that 64-bit value is sign-extended if enum is signed and has size < 8? > + btf_verifier_log_type(env, t, "Unexpected size"); > + return -EINVAL; > + } > + > + /* enum type either no name or a valid one */ > + if (t->name_off && > + !btf_name_valid_identifier(env->btf, t->name_off)) { > + btf_verifier_log_type(env, t, "Invalid name"); > + return -EINVAL; > + } > + [...]
On 5/9/22 3:29 PM, Andrii Nakryiko wrote: > On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: >> >> Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM. >> But in kernel, some enum indeed has 64bit values, e.g., >> in uapi bpf.h, we have >> enum { >> BPF_F_INDEX_MASK = 0xffffffffULL, >> BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK, >> BPF_F_CTXLEN_MASK = (0xfffffULL << 32), >> }; >> In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK >> as 0, which certainly is incorrect. >> >> This patch added a new btf kind, BTF_KIND_ENUM64, which permits >> 64bit value to cover the above use case. The BTF_KIND_ENUM64 has >> the following three bytes followed by the common type: > > you probably meant three fields, not bytes correct. > >> struct bpf_enum64 { >> __u32 nume_off; >> __u32 hi32; >> __u32 lo32; > > I'd like to nitpick on name here, as hi/lo of what? Maybe val_hi32 and > val_lo32? Can we also reverse the order here? For x86 you'll be able > to use &lo32 to get value directly if you really want, without a local > copy. It also just logically seems better to have something low first, > then high next. I can go with val_hi32, val_lo32 and put val_lo32 before val_hi32. I don't have any preference for the ordering of these two fields. > > >> }; >> Currently, btf type section has an alignment of 4 as all element types >> are u32. Representing the value with __u64 will introduce a pad >> for bpf_enum64 and may also introduce misalignment for the 64bit value. >> Hence, two members of hi32 and lo32 are chosen to avoid these issues. >> >> The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64 >> to indicate whether the value is signed or unsigned. The kflag intends >> to provide consistent output of BTF C fortmat with the original >> source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff. >> The format C has two choices, print out 0xffffffff or -1 and current libbpf >> prints out as unsigned value. But if the signedness is preserved in btf, >> the value can be printed the same as the original source code. >> >> The new BTF_KIND_ENUM64 is intended to support the enum value represented as >> 64bit value. But it can represent all BTF_KIND_ENUM values as well. >> The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent. >> The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has >> to be represented with 64 bits. >> >> [1] https://reviews.llvm.org/D124641 >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> include/linux/btf.h | 18 ++++- >> include/uapi/linux/btf.h | 17 ++++- >> kernel/bpf/btf.c | 132 ++++++++++++++++++++++++++++++--- >> tools/include/uapi/linux/btf.h | 17 ++++- >> 4 files changed, 168 insertions(+), 16 deletions(-) >> >> diff --git a/include/linux/btf.h b/include/linux/btf.h >> index 2611cea2c2b6..280c33c9414a 100644 >> --- a/include/linux/btf.h >> +++ b/include/linux/btf.h >> @@ -174,7 +174,8 @@ static inline bool btf_type_is_small_int(const struct btf_type *t) >> >> static inline bool btf_type_is_enum(const struct btf_type *t) >> { >> - return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM; >> + return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM || >> + BTF_INFO_KIND(t->info) == BTF_KIND_ENUM64; >> } > > a bit hesitant about this change, there is no helper to check for ENUM > vs ENUM64. Inside the kernel this change seems to be correct as we > don't care, but for libbpf I'd probably keep btf_is_enum() unchanged > (you can't really work with them in the same generic way in > user-space, as their memory layout is very different, so it's better > not to generalize them unnecessarily) Let me introduce a new helper called btf_type_is_any_enum(...) to check both BTF_KIND_ENUM or BTF_KIND_ENUM64. > >> >> static inline bool str_is_empty(const char *s) >> @@ -192,6 +193,16 @@ static inline bool btf_is_enum(const struct btf_type *t) >> return btf_kind(t) == BTF_KIND_ENUM; >> } >> >> +static inline bool btf_is_enum64(const struct btf_type *t) >> +{ >> + return btf_kind(t) == BTF_KIND_ENUM64; >> +} >> + >> +static inline u64 btf_enum64_value(const struct btf_enum64 *e) >> +{ >> + return (u64)e->hi32 << 32 | e->lo32; > > this might be correct but () around bit shift would make it more obvious I can do this. > >> +} >> + >> static inline bool btf_is_composite(const struct btf_type *t) >> { >> u16 kind = btf_kind(t); >> @@ -332,6 +343,11 @@ static inline struct btf_enum *btf_enum(const struct btf_type *t) >> return (struct btf_enum *)(t + 1); >> } >> >> +static inline struct btf_enum64 *btf_enum64(const struct btf_type *t) >> +{ >> + return (struct btf_enum64 *)(t + 1); >> +} >> + >> static inline const struct btf_var_secinfo *btf_type_var_secinfo( >> const struct btf_type *t) >> { >> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h >> index a9162a6c0284..2aac226a27b2 100644 >> --- a/include/uapi/linux/btf.h >> +++ b/include/uapi/linux/btf.h >> @@ -36,10 +36,10 @@ struct btf_type { >> * bits 24-28: kind (e.g. int, ptr, array...etc) >> * bits 29-30: unused >> * bit 31: kind_flag, currently used by >> - * struct, union and fwd >> + * struct, union, enum, fwd and enum64 > > see comment below on kflag for enum itself, but reading this I > realized that we don't really describe the meaning of kind_flag for > different kinds. Should it be done here? We have detailed description in Documentation/bpf/btf.rst. Hopefully it will be enough if people wants to understand what kflag means for each kind. > >> */ >> __u32 info; >> - /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC. >> + /* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64. >> * "size" tells the size of the type it is describing. >> * >> * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, >> @@ -63,7 +63,7 @@ enum { >> BTF_KIND_ARRAY = 3, /* Array */ >> BTF_KIND_STRUCT = 4, /* Struct */ >> BTF_KIND_UNION = 5, /* Union */ >> - BTF_KIND_ENUM = 6, /* Enumeration */ >> + BTF_KIND_ENUM = 6, /* Enumeration for int/unsigned int values */ > > nit: "Enumeration for up to 32-bit values" ? This should work. > >> BTF_KIND_FWD = 7, /* Forward */ >> BTF_KIND_TYPEDEF = 8, /* Typedef */ >> BTF_KIND_VOLATILE = 9, /* Volatile */ >> @@ -76,6 +76,7 @@ enum { >> BTF_KIND_FLOAT = 16, /* Floating point */ >> BTF_KIND_DECL_TAG = 17, /* Decl Tag */ >> BTF_KIND_TYPE_TAG = 18, /* Type Tag */ >> + BTF_KIND_ENUM64 = 19, /* Enumeration for long/unsigned long values */ > > and then "for 64-bit values" (or maybe up to 64 bit values, but in > practice we won't do that, right?) We can do "up to 64-bit values". In practice, from llvm and pahole, we will only encode 64-bit values in ENUM64. > >> >> NR_BTF_KINDS, >> BTF_KIND_MAX = NR_BTF_KINDS - 1, >> @@ -186,4 +187,14 @@ struct btf_decl_tag { >> __s32 component_idx; >> }; >> > > [...] > >> @@ -3716,7 +3721,8 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env, >> >> if (env->log.level == BPF_LOG_KERNEL) >> continue; >> - btf_verifier_log(env, "\t%s val=%d\n", >> + fmt_str = btf_type_kflag(t) ? "\t%s val=%u\n" : "\t%s val=%d\n"; >> + btf_verifier_log(env, fmt_str, >> __btf_name_by_offset(btf, enums[i].name_off), >> enums[i].val); >> } >> @@ -3757,7 +3763,10 @@ static void btf_enum_show(const struct btf *btf, const struct btf_type *t, >> return; >> } >> >> - btf_show_type_value(show, "%d", v); >> + if (btf_type_kflag(t)) > > libbpf's assumption right now and most common case is unsigned enum, > so it seems more desirable to have kflag == 0 mean unsigned, with > kflag == 1 being signed. It will make most existing enum definitions > not change but also make it easy for libbpf's btf_dumper to use kflag > if it's set, but otherwise have the same unsigned printing whether > enum is really unsigned or Clang is too old to emit the kflag for > enum. WDYT? Right, libbpf assumption is unsigned enum and the kernel prints as signed. I agree that default unsigned should cover more cases. Will change that in the next revision. > >> + btf_show_type_value(show, "%u", v); >> + else >> + btf_show_type_value(show, "%d", v); > > you didn't got with ternary operator for fmt string selector like in > previous case? I have mild preference for keeping it consistent (and > keeping btf_type_kflag(t) ? "fmt1" : "fmt2" inline) The reason I didn't do it is the line is a little long. But I can do it. > >> btf_show_end_type(show); >> } >> >> @@ -3770,6 +3779,109 @@ static struct btf_kind_operations enum_ops = { >> .show = btf_enum_show, >> }; >> >> +static s32 btf_enum64_check_meta(struct btf_verifier_env *env, >> + const struct btf_type *t, >> + u32 meta_left) >> +{ >> + const struct btf_enum64 *enums = btf_type_enum64(t); >> + struct btf *btf = env->btf; >> + const char *fmt_str; >> + u16 i, nr_enums; >> + u32 meta_needed; >> + >> + nr_enums = btf_type_vlen(t); >> + meta_needed = nr_enums * sizeof(*enums); >> + >> + if (meta_left < meta_needed) { >> + btf_verifier_log_basic(env, t, >> + "meta_left:%u meta_needed:%u", >> + meta_left, meta_needed); >> + return -EINVAL; >> + } >> + >> + if (t->size != 8) { > > technically there is nothing wrong with using enum64 for smaller > sizes, right? Any particular reason to prevent this? We can just > define that 64-bit value is sign-extended if enum is signed and has > size < 8? My original idea is to support 64-bit enum only for ENUM64 kind. But it is certainly possible to encode 32-bit enums as well for ENUM64. So I will remove this restriction. The dwarf only generates sizes 4 (for up-to 32 bit values) and 8 (for 64 bit values). But BTF_KIND_ENUM supports 1/2/4/8 sizes, so BTF_KIND_ENUM64 will also support 1/2/4/8 sizes. > >> + btf_verifier_log_type(env, t, "Unexpected size"); >> + return -EINVAL; >> + } >> + >> + /* enum type either no name or a valid one */ >> + if (t->name_off && >> + !btf_name_valid_identifier(env->btf, t->name_off)) { >> + btf_verifier_log_type(env, t, "Invalid name"); >> + return -EINVAL; >> + } >> + > > [...]
On Tue, May 10, 2022 at 3:06 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 5/9/22 3:29 PM, Andrii Nakryiko wrote: > > On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM. > >> But in kernel, some enum indeed has 64bit values, e.g., > >> in uapi bpf.h, we have > >> enum { > >> BPF_F_INDEX_MASK = 0xffffffffULL, > >> BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK, > >> BPF_F_CTXLEN_MASK = (0xfffffULL << 32), > >> }; > >> In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK > >> as 0, which certainly is incorrect. > >> > >> This patch added a new btf kind, BTF_KIND_ENUM64, which permits > >> 64bit value to cover the above use case. The BTF_KIND_ENUM64 has > >> the following three bytes followed by the common type: > > > > you probably meant three fields, not bytes > > correct. > > > > >> struct bpf_enum64 { > >> __u32 nume_off; > >> __u32 hi32; > >> __u32 lo32; > > > > I'd like to nitpick on name here, as hi/lo of what? Maybe val_hi32 and > > val_lo32? Can we also reverse the order here? For x86 you'll be able > > to use &lo32 to get value directly if you really want, without a local > > copy. It also just logically seems better to have something low first, > > then high next. > > I can go with val_hi32, val_lo32 and put val_lo32 before val_hi32. > I don't have any preference for the ordering of these two fields. > > > > > > >> }; > >> Currently, btf type section has an alignment of 4 as all element types > >> are u32. Representing the value with __u64 will introduce a pad > >> for bpf_enum64 and may also introduce misalignment for the 64bit value. > >> Hence, two members of hi32 and lo32 are chosen to avoid these issues. > >> > >> The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64 > >> to indicate whether the value is signed or unsigned. The kflag intends > >> to provide consistent output of BTF C fortmat with the original > >> source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff. > >> The format C has two choices, print out 0xffffffff or -1 and current libbpf > >> prints out as unsigned value. But if the signedness is preserved in btf, > >> the value can be printed the same as the original source code. > >> > >> The new BTF_KIND_ENUM64 is intended to support the enum value represented as > >> 64bit value. But it can represent all BTF_KIND_ENUM values as well. > >> The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent. > >> The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has > >> to be represented with 64 bits. > >> > >> [1] https://reviews.llvm.org/D124641 > >> > >> Signed-off-by: Yonghong Song <yhs@fb.com> > >> --- > >> include/linux/btf.h | 18 ++++- > >> include/uapi/linux/btf.h | 17 ++++- > >> kernel/bpf/btf.c | 132 ++++++++++++++++++++++++++++++--- > >> tools/include/uapi/linux/btf.h | 17 ++++- > >> 4 files changed, 168 insertions(+), 16 deletions(-) > >> > >> diff --git a/include/linux/btf.h b/include/linux/btf.h > >> index 2611cea2c2b6..280c33c9414a 100644 > >> --- a/include/linux/btf.h > >> +++ b/include/linux/btf.h > >> @@ -174,7 +174,8 @@ static inline bool btf_type_is_small_int(const struct btf_type *t) > >> > >> static inline bool btf_type_is_enum(const struct btf_type *t) > >> { > >> - return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM; > >> + return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM || > >> + BTF_INFO_KIND(t->info) == BTF_KIND_ENUM64; > >> } > > > > a bit hesitant about this change, there is no helper to check for ENUM > > vs ENUM64. Inside the kernel this change seems to be correct as we > > don't care, but for libbpf I'd probably keep btf_is_enum() unchanged > > (you can't really work with them in the same generic way in > > user-space, as their memory layout is very different, so it's better > > not to generalize them unnecessarily) > > Let me introduce a new helper called > btf_type_is_any_enum(...) to check both > BTF_KIND_ENUM or BTF_KIND_ENUM64. > > > > >> > >> static inline bool str_is_empty(const char *s) > >> @@ -192,6 +193,16 @@ static inline bool btf_is_enum(const struct btf_type *t) > >> return btf_kind(t) == BTF_KIND_ENUM; > >> } > >> > >> +static inline bool btf_is_enum64(const struct btf_type *t) > >> +{ > >> + return btf_kind(t) == BTF_KIND_ENUM64; > >> +} > >> + > >> +static inline u64 btf_enum64_value(const struct btf_enum64 *e) > >> +{ > >> + return (u64)e->hi32 << 32 | e->lo32; > > > > this might be correct but () around bit shift would make it more obvious > > I can do this. > > > > >> +} > >> + > >> static inline bool btf_is_composite(const struct btf_type *t) > >> { > >> u16 kind = btf_kind(t); > >> @@ -332,6 +343,11 @@ static inline struct btf_enum *btf_enum(const struct btf_type *t) > >> return (struct btf_enum *)(t + 1); > >> } > >> > >> +static inline struct btf_enum64 *btf_enum64(const struct btf_type *t) > >> +{ > >> + return (struct btf_enum64 *)(t + 1); > >> +} > >> + > >> static inline const struct btf_var_secinfo *btf_type_var_secinfo( > >> const struct btf_type *t) > >> { > >> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h > >> index a9162a6c0284..2aac226a27b2 100644 > >> --- a/include/uapi/linux/btf.h > >> +++ b/include/uapi/linux/btf.h > >> @@ -36,10 +36,10 @@ struct btf_type { > >> * bits 24-28: kind (e.g. int, ptr, array...etc) > >> * bits 29-30: unused > >> * bit 31: kind_flag, currently used by > >> - * struct, union and fwd > >> + * struct, union, enum, fwd and enum64 > > > > see comment below on kflag for enum itself, but reading this I > > realized that we don't really describe the meaning of kind_flag for > > different kinds. Should it be done here? > > We have detailed description in Documentation/bpf/btf.rst. > Hopefully it will be enough if people wants to understand > what kflag means for each kind. > > > > > >> */ > >> __u32 info; > >> - /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC. > >> + /* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64. > >> * "size" tells the size of the type it is describing. > >> * > >> * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, > >> @@ -63,7 +63,7 @@ enum { > >> BTF_KIND_ARRAY = 3, /* Array */ > >> BTF_KIND_STRUCT = 4, /* Struct */ > >> BTF_KIND_UNION = 5, /* Union */ > >> - BTF_KIND_ENUM = 6, /* Enumeration */ > >> + BTF_KIND_ENUM = 6, /* Enumeration for int/unsigned int values */ > > > > nit: "Enumeration for up to 32-bit values" ? > > This should work. > > > > >> BTF_KIND_FWD = 7, /* Forward */ > >> BTF_KIND_TYPEDEF = 8, /* Typedef */ > >> BTF_KIND_VOLATILE = 9, /* Volatile */ > >> @@ -76,6 +76,7 @@ enum { > >> BTF_KIND_FLOAT = 16, /* Floating point */ > >> BTF_KIND_DECL_TAG = 17, /* Decl Tag */ > >> BTF_KIND_TYPE_TAG = 18, /* Type Tag */ > >> + BTF_KIND_ENUM64 = 19, /* Enumeration for long/unsigned long values */ > > > > and then "for 64-bit values" (or maybe up to 64 bit values, but in > > practice we won't do that, right?) > > We can do "up to 64-bit values". In practice, from llvm and pahole, > we will only encode 64-bit values in ENUM64. > > > > >> > >> NR_BTF_KINDS, > >> BTF_KIND_MAX = NR_BTF_KINDS - 1, > >> @@ -186,4 +187,14 @@ struct btf_decl_tag { > >> __s32 component_idx; > >> }; > >> > > > > [...] > > > >> @@ -3716,7 +3721,8 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env, > >> > >> if (env->log.level == BPF_LOG_KERNEL) > >> continue; > >> - btf_verifier_log(env, "\t%s val=%d\n", > >> + fmt_str = btf_type_kflag(t) ? "\t%s val=%u\n" : "\t%s val=%d\n"; > >> + btf_verifier_log(env, fmt_str, > >> __btf_name_by_offset(btf, enums[i].name_off), > >> enums[i].val); > >> } > >> @@ -3757,7 +3763,10 @@ static void btf_enum_show(const struct btf *btf, const struct btf_type *t, > >> return; > >> } > >> > >> - btf_show_type_value(show, "%d", v); > >> + if (btf_type_kflag(t)) > > > > libbpf's assumption right now and most common case is unsigned enum, > > so it seems more desirable to have kflag == 0 mean unsigned, with > > kflag == 1 being signed. It will make most existing enum definitions > > not change but also make it easy for libbpf's btf_dumper to use kflag > > if it's set, but otherwise have the same unsigned printing whether > > enum is really unsigned or Clang is too old to emit the kflag for > > enum. WDYT? > > Right, libbpf assumption is unsigned enum and the kernel prints as > signed. I agree that default unsigned should cover more cases. > Will change that in the next revision. > > > > >> + btf_show_type_value(show, "%u", v); > >> + else > >> + btf_show_type_value(show, "%d", v); > > > > you didn't got with ternary operator for fmt string selector like in > > previous case? I have mild preference for keeping it consistent (and > > keeping btf_type_kflag(t) ? "fmt1" : "fmt2" inline) > > The reason I didn't do it is the line is a little long. > But I can do it. > > > > >> btf_show_end_type(show); > >> } > >> > >> @@ -3770,6 +3779,109 @@ static struct btf_kind_operations enum_ops = { > >> .show = btf_enum_show, > >> }; > >> > >> +static s32 btf_enum64_check_meta(struct btf_verifier_env *env, > >> + const struct btf_type *t, > >> + u32 meta_left) > >> +{ > >> + const struct btf_enum64 *enums = btf_type_enum64(t); > >> + struct btf *btf = env->btf; > >> + const char *fmt_str; > >> + u16 i, nr_enums; > >> + u32 meta_needed; > >> + > >> + nr_enums = btf_type_vlen(t); > >> + meta_needed = nr_enums * sizeof(*enums); > >> + > >> + if (meta_left < meta_needed) { > >> + btf_verifier_log_basic(env, t, > >> + "meta_left:%u meta_needed:%u", > >> + meta_left, meta_needed); > >> + return -EINVAL; > >> + } > >> + > >> + if (t->size != 8) { > > > > technically there is nothing wrong with using enum64 for smaller > > sizes, right? Any particular reason to prevent this? We can just > > define that 64-bit value is sign-extended if enum is signed and has > > size < 8? > > My original idea is to support 64-bit enum only for ENUM64 kind. > But it is certainly possible to encode 32-bit enums as well for > ENUM64. So I will remove this restriction. > > The dwarf only generates sizes 4 (for up-to 32 bit values) > and 8 (for 64 bit values). But BTF_KIND_ENUM supports 1/2/4/8 > sizes, so BTF_KIND_ENUM64 will also support 1/2/4/8 sizes. Little known fact, but it's not true: $ bpftool btf dump file /sys/kernel/btf/vmlinux| rg 'ENUM.*size=1' -A8 [83476] ENUM 'hub_led_mode' size=1 vlen=8 'INDICATOR_AUTO' val=0 'INDICATOR_CYCLE' val=1 'INDICATOR_GREEN_BLINK' val=2 'INDICATOR_GREEN_BLINK_OFF' val=3 'INDICATOR_AMBER_BLINK' val=4 'INDICATOR_AMBER_BLINK_OFF' val=5 'INDICATOR_ALT_BLINK' val=6 'INDICATOR_ALT_BLINK_OFF' val=7 Defined as packed enum: enum hub_led_mode { INDICATOR_AUTO = 0, INDICATOR_CYCLE, /* software blinks for attention: software, hardware, reserved */ INDICATOR_GREEN_BLINK, INDICATOR_GREEN_BLINK_OFF, INDICATOR_AMBER_BLINK, INDICATOR_AMBER_BLINK_OFF, INDICATOR_ALT_BLINK, INDICATOR_ALT_BLINK_OFF } __attribute__ ((packed)); > > > > >> + btf_verifier_log_type(env, t, "Unexpected size"); > >> + return -EINVAL; > >> + } > >> + > >> + /* enum type either no name or a valid one */ > >> + if (t->name_off && > >> + !btf_name_valid_identifier(env->btf, t->name_off)) { > >> + btf_verifier_log_type(env, t, "Invalid name"); > >> + return -EINVAL; > >> + } > >> + > > > > [...]
On 5/10/22 4:18 PM, Andrii Nakryiko wrote: > On Tue, May 10, 2022 at 3:06 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 5/9/22 3:29 PM, Andrii Nakryiko wrote: >>> On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: >>>> >>>> Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM. >>>> But in kernel, some enum indeed has 64bit values, e.g., >>>> in uapi bpf.h, we have >>>> enum { >>>> BPF_F_INDEX_MASK = 0xffffffffULL, >>>> BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK, >>>> BPF_F_CTXLEN_MASK = (0xfffffULL << 32), >>>> }; >>>> In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK >>>> as 0, which certainly is incorrect. >>>> >>>> This patch added a new btf kind, BTF_KIND_ENUM64, which permits >>>> 64bit value to cover the above use case. The BTF_KIND_ENUM64 has >>>> the following three bytes followed by the common type: >>> >>> you probably meant three fields, not bytes >> >> correct. >> >>> >>>> struct bpf_enum64 { >>>> __u32 nume_off; >>>> __u32 hi32; >>>> __u32 lo32; >>> >>> I'd like to nitpick on name here, as hi/lo of what? Maybe val_hi32 and >>> val_lo32? Can we also reverse the order here? For x86 you'll be able >>> to use &lo32 to get value directly if you really want, without a local >>> copy. It also just logically seems better to have something low first, >>> then high next. >> >> I can go with val_hi32, val_lo32 and put val_lo32 before val_hi32. >> I don't have any preference for the ordering of these two fields. >> >>> >>> >>>> }; >>>> Currently, btf type section has an alignment of 4 as all element types >>>> are u32. Representing the value with __u64 will introduce a pad >>>> for bpf_enum64 and may also introduce misalignment for the 64bit value. >>>> Hence, two members of hi32 and lo32 are chosen to avoid these issues. >>>> >>>> The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64 >>>> to indicate whether the value is signed or unsigned. The kflag intends >>>> to provide consistent output of BTF C fortmat with the original >>>> source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff. >>>> The format C has two choices, print out 0xffffffff or -1 and current libbpf >>>> prints out as unsigned value. But if the signedness is preserved in btf, >>>> the value can be printed the same as the original source code. >>>> >>>> The new BTF_KIND_ENUM64 is intended to support the enum value represented as >>>> 64bit value. But it can represent all BTF_KIND_ENUM values as well. >>>> The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent. >>>> The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has >>>> to be represented with 64 bits. >>>> >>>> [1] https://reviews.llvm.org/D124641 >>>> >>>> Signed-off-by: Yonghong Song <yhs@fb.com> [...] >>> >>>> btf_show_end_type(show); >>>> } >>>> >>>> @@ -3770,6 +3779,109 @@ static struct btf_kind_operations enum_ops = { >>>> .show = btf_enum_show, >>>> }; >>>> >>>> +static s32 btf_enum64_check_meta(struct btf_verifier_env *env, >>>> + const struct btf_type *t, >>>> + u32 meta_left) >>>> +{ >>>> + const struct btf_enum64 *enums = btf_type_enum64(t); >>>> + struct btf *btf = env->btf; >>>> + const char *fmt_str; >>>> + u16 i, nr_enums; >>>> + u32 meta_needed; >>>> + >>>> + nr_enums = btf_type_vlen(t); >>>> + meta_needed = nr_enums * sizeof(*enums); >>>> + >>>> + if (meta_left < meta_needed) { >>>> + btf_verifier_log_basic(env, t, >>>> + "meta_left:%u meta_needed:%u", >>>> + meta_left, meta_needed); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (t->size != 8) { >>> >>> technically there is nothing wrong with using enum64 for smaller >>> sizes, right? Any particular reason to prevent this? We can just >>> define that 64-bit value is sign-extended if enum is signed and has >>> size < 8? >> >> My original idea is to support 64-bit enum only for ENUM64 kind. >> But it is certainly possible to encode 32-bit enums as well for >> ENUM64. So I will remove this restriction. >> >> The dwarf only generates sizes 4 (for up-to 32 bit values) >> and 8 (for 64 bit values). But BTF_KIND_ENUM supports 1/2/4/8 >> sizes, so BTF_KIND_ENUM64 will also support 1/2/4/8 sizes. > > Little known fact, but it's not true: > > $ bpftool btf dump file /sys/kernel/btf/vmlinux| rg 'ENUM.*size=1' -A8 > [83476] ENUM 'hub_led_mode' size=1 vlen=8 > 'INDICATOR_AUTO' val=0 > 'INDICATOR_CYCLE' val=1 > 'INDICATOR_GREEN_BLINK' val=2 > 'INDICATOR_GREEN_BLINK_OFF' val=3 > 'INDICATOR_AMBER_BLINK' val=4 > 'INDICATOR_AMBER_BLINK_OFF' val=5 > 'INDICATOR_ALT_BLINK' val=6 > 'INDICATOR_ALT_BLINK_OFF' val=7 > > Defined as packed enum: > > enum hub_led_mode { > INDICATOR_AUTO = 0, > INDICATOR_CYCLE, > /* software blinks for attention: software, hardware, reserved */ > INDICATOR_GREEN_BLINK, INDICATOR_GREEN_BLINK_OFF, > INDICATOR_AMBER_BLINK, INDICATOR_AMBER_BLINK_OFF, > INDICATOR_ALT_BLINK, INDICATOR_ALT_BLINK_OFF > } __attribute__ ((packed)); I am not aware of this.... Good to know. > > >> >>> >>>> + btf_verifier_log_type(env, t, "Unexpected size"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* enum type either no name or a valid one */ >>>> + if (t->name_off && >>>> + !btf_name_valid_identifier(env->btf, t->name_off)) { >>>> + btf_verifier_log_type(env, t, "Invalid name"); >>>> + return -EINVAL; >>>> + } >>>> + >>> >>> [...]
diff --git a/include/linux/btf.h b/include/linux/btf.h index 2611cea2c2b6..280c33c9414a 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -174,7 +174,8 @@ static inline bool btf_type_is_small_int(const struct btf_type *t) static inline bool btf_type_is_enum(const struct btf_type *t) { - return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM; + return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM || + BTF_INFO_KIND(t->info) == BTF_KIND_ENUM64; } static inline bool str_is_empty(const char *s) @@ -192,6 +193,16 @@ static inline bool btf_is_enum(const struct btf_type *t) return btf_kind(t) == BTF_KIND_ENUM; } +static inline bool btf_is_enum64(const struct btf_type *t) +{ + return btf_kind(t) == BTF_KIND_ENUM64; +} + +static inline u64 btf_enum64_value(const struct btf_enum64 *e) +{ + return (u64)e->hi32 << 32 | e->lo32; +} + static inline bool btf_is_composite(const struct btf_type *t) { u16 kind = btf_kind(t); @@ -332,6 +343,11 @@ static inline struct btf_enum *btf_enum(const struct btf_type *t) return (struct btf_enum *)(t + 1); } +static inline struct btf_enum64 *btf_enum64(const struct btf_type *t) +{ + return (struct btf_enum64 *)(t + 1); +} + static inline const struct btf_var_secinfo *btf_type_var_secinfo( const struct btf_type *t) { diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h index a9162a6c0284..2aac226a27b2 100644 --- a/include/uapi/linux/btf.h +++ b/include/uapi/linux/btf.h @@ -36,10 +36,10 @@ struct btf_type { * bits 24-28: kind (e.g. int, ptr, array...etc) * bits 29-30: unused * bit 31: kind_flag, currently used by - * struct, union and fwd + * struct, union, enum, fwd and enum64 */ __u32 info; - /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC. + /* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64. * "size" tells the size of the type it is describing. * * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, @@ -63,7 +63,7 @@ enum { BTF_KIND_ARRAY = 3, /* Array */ BTF_KIND_STRUCT = 4, /* Struct */ BTF_KIND_UNION = 5, /* Union */ - BTF_KIND_ENUM = 6, /* Enumeration */ + BTF_KIND_ENUM = 6, /* Enumeration for int/unsigned int values */ BTF_KIND_FWD = 7, /* Forward */ BTF_KIND_TYPEDEF = 8, /* Typedef */ BTF_KIND_VOLATILE = 9, /* Volatile */ @@ -76,6 +76,7 @@ enum { BTF_KIND_FLOAT = 16, /* Floating point */ BTF_KIND_DECL_TAG = 17, /* Decl Tag */ BTF_KIND_TYPE_TAG = 18, /* Type Tag */ + BTF_KIND_ENUM64 = 19, /* Enumeration for long/unsigned long values */ NR_BTF_KINDS, BTF_KIND_MAX = NR_BTF_KINDS - 1, @@ -186,4 +187,14 @@ struct btf_decl_tag { __s32 component_idx; }; +/* BTF_KIND_ENUM64 is followed by multiple "struct btf_enum64". + * The exact number of btf_enum64 is stored in the vlen (of the + * info in "struct btf_type"). + */ +struct btf_enum64 { + __u32 name_off; + __u32 hi32; + __u32 lo32; +}; + #endif /* _UAPI__LINUX_BTF_H__ */ diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 2f0b0440131c..17e24b362d3d 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -307,6 +307,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = { [BTF_KIND_FLOAT] = "FLOAT", [BTF_KIND_DECL_TAG] = "DECL_TAG", [BTF_KIND_TYPE_TAG] = "TYPE_TAG", + [BTF_KIND_ENUM64] = "ENUM64", }; const char *btf_type_str(const struct btf_type *t) @@ -664,6 +665,7 @@ static bool btf_type_has_size(const struct btf_type *t) case BTF_KIND_ENUM: case BTF_KIND_DATASEC: case BTF_KIND_FLOAT: + case BTF_KIND_ENUM64: return true; } @@ -709,6 +711,11 @@ static const struct btf_decl_tag *btf_type_decl_tag(const struct btf_type *t) return (const struct btf_decl_tag *)(t + 1); } +static const struct btf_enum64 *btf_type_enum64(const struct btf_type *t) +{ + return (const struct btf_enum64 *)(t + 1); +} + static const struct btf_kind_operations *btf_type_ops(const struct btf_type *t) { return kind_ops[BTF_INFO_KIND(t->info)]; @@ -1017,6 +1024,7 @@ static const char *btf_show_name(struct btf_show *show) parens = "{"; break; case BTF_KIND_ENUM: + case BTF_KIND_ENUM64: prefix = "enum"; break; default: @@ -1832,6 +1840,7 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type, case BTF_KIND_UNION: case BTF_KIND_ENUM: case BTF_KIND_FLOAT: + case BTF_KIND_ENUM64: size = type->size; goto resolved; @@ -3668,6 +3677,7 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env, { const struct btf_enum *enums = btf_type_enum(t); struct btf *btf = env->btf; + const char *fmt_str; u16 i, nr_enums; u32 meta_needed; @@ -3681,11 +3691,6 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env, return -EINVAL; } - if (btf_type_kflag(t)) { - btf_verifier_log_type(env, t, "Invalid btf_info kind_flag"); - return -EINVAL; - } - if (t->size > 8 || !is_power_of_2(t->size)) { btf_verifier_log_type(env, t, "Unexpected size"); return -EINVAL; @@ -3716,7 +3721,8 @@ static s32 btf_enum_check_meta(struct btf_verifier_env *env, if (env->log.level == BPF_LOG_KERNEL) continue; - btf_verifier_log(env, "\t%s val=%d\n", + fmt_str = btf_type_kflag(t) ? "\t%s val=%u\n" : "\t%s val=%d\n"; + btf_verifier_log(env, fmt_str, __btf_name_by_offset(btf, enums[i].name_off), enums[i].val); } @@ -3757,7 +3763,10 @@ static void btf_enum_show(const struct btf *btf, const struct btf_type *t, return; } - btf_show_type_value(show, "%d", v); + if (btf_type_kflag(t)) + btf_show_type_value(show, "%u", v); + else + btf_show_type_value(show, "%d", v); btf_show_end_type(show); } @@ -3770,6 +3779,109 @@ static struct btf_kind_operations enum_ops = { .show = btf_enum_show, }; +static s32 btf_enum64_check_meta(struct btf_verifier_env *env, + const struct btf_type *t, + u32 meta_left) +{ + const struct btf_enum64 *enums = btf_type_enum64(t); + struct btf *btf = env->btf; + const char *fmt_str; + u16 i, nr_enums; + u32 meta_needed; + + nr_enums = btf_type_vlen(t); + meta_needed = nr_enums * sizeof(*enums); + + if (meta_left < meta_needed) { + btf_verifier_log_basic(env, t, + "meta_left:%u meta_needed:%u", + meta_left, meta_needed); + return -EINVAL; + } + + if (t->size != 8) { + btf_verifier_log_type(env, t, "Unexpected size"); + return -EINVAL; + } + + /* enum type either no name or a valid one */ + if (t->name_off && + !btf_name_valid_identifier(env->btf, t->name_off)) { + btf_verifier_log_type(env, t, "Invalid name"); + return -EINVAL; + } + + btf_verifier_log_type(env, t, NULL); + + for (i = 0; i < nr_enums; i++) { + if (!btf_name_offset_valid(btf, enums[i].name_off)) { + btf_verifier_log(env, "\tInvalid name_offset:%u", + enums[i].name_off); + return -EINVAL; + } + + /* enum member must have a valid name */ + if (!enums[i].name_off || + !btf_name_valid_identifier(btf, enums[i].name_off)) { + btf_verifier_log_type(env, t, "Invalid name"); + return -EINVAL; + } + + if (env->log.level == BPF_LOG_KERNEL) + continue; + + fmt_str = btf_type_kflag(t) ? "\t%s val=%llu\n" : "\t%s val=%lld\n"; + btf_verifier_log(env, fmt_str, + __btf_name_by_offset(btf, enums[i].name_off), + btf_enum64_value(enums + i)); + } + + return meta_needed; +} + +static void btf_enum64_show(const struct btf *btf, const struct btf_type *t, + u32 type_id, void *data, u8 bits_offset, + struct btf_show *show) +{ + const struct btf_enum64 *enums = btf_type_enum64(t); + u32 i, nr_enums = btf_type_vlen(t); + void *safe_data; + s64 v; + + safe_data = btf_show_start_type(show, t, type_id, data); + if (!safe_data) + return; + + v = *(u64 *)safe_data; + + for (i = 0; i < nr_enums; i++) { + if (v != btf_enum64_value(enums + i)) + continue; + + btf_show_type_value(show, "%s", + __btf_name_by_offset(btf, + enums[i].name_off)); + + btf_show_end_type(show); + return; + } + + if (btf_type_kflag(t)) + btf_show_type_value(show, "%llu", v); + else + btf_show_type_value(show, "%lld", v); + btf_show_end_type(show); +} + +static struct btf_kind_operations enum64_ops = { + .check_meta = btf_enum64_check_meta, + .resolve = btf_df_resolve, + .check_member = btf_enum_check_member, + .check_kflag_member = btf_enum_check_kflag_member, + .log_details = btf_enum_log, + .show = btf_enum64_show, +}; + static s32 btf_func_proto_check_meta(struct btf_verifier_env *env, const struct btf_type *t, u32 meta_left) @@ -4436,6 +4548,7 @@ static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = { [BTF_KIND_FLOAT] = &float_ops, [BTF_KIND_DECL_TAG] = &decl_tag_ops, [BTF_KIND_TYPE_TAG] = &modifier_ops, + [BTF_KIND_ENUM64] = &enum64_ops, }; static s32 btf_check_meta(struct btf_verifier_env *env, @@ -7329,6 +7442,7 @@ int __bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id, case BTF_KIND_UNION: case BTF_KIND_ENUM: case BTF_KIND_FWD: + case BTF_KIND_ENUM64: return 1; case BTF_KIND_INT: /* just reject deprecated bitfield-like integers; all other @@ -7381,10 +7495,10 @@ int __bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id, * field-based relocations. This function assumes that root types were already * checked for name match. Beyond that initial root-level name check, names * are completely ignored. Compatibility rules are as follows: - * - any two STRUCTs/UNIONs/FWDs/ENUMs/INTs are considered compatible, but + * - any two STRUCTs/UNIONs/FWDs/ENUMs/INTs/ENUM64s are considered compatible, but * kind should match for local and target types (i.e., STRUCT is not * compatible with UNION); - * - for ENUMs, the size is ignored; + * - for ENUMs/ENUM64s, the size is ignored; * - for INT, size and signedness are ignored; * - for ARRAY, dimensionality is ignored, element types are checked for * compatibility recursively; diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h index a9162a6c0284..2aac226a27b2 100644 --- a/tools/include/uapi/linux/btf.h +++ b/tools/include/uapi/linux/btf.h @@ -36,10 +36,10 @@ struct btf_type { * bits 24-28: kind (e.g. int, ptr, array...etc) * bits 29-30: unused * bit 31: kind_flag, currently used by - * struct, union and fwd + * struct, union, enum, fwd and enum64 */ __u32 info; - /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC. + /* "size" is used by INT, ENUM, STRUCT, UNION, DATASEC and ENUM64. * "size" tells the size of the type it is describing. * * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, @@ -63,7 +63,7 @@ enum { BTF_KIND_ARRAY = 3, /* Array */ BTF_KIND_STRUCT = 4, /* Struct */ BTF_KIND_UNION = 5, /* Union */ - BTF_KIND_ENUM = 6, /* Enumeration */ + BTF_KIND_ENUM = 6, /* Enumeration for int/unsigned int values */ BTF_KIND_FWD = 7, /* Forward */ BTF_KIND_TYPEDEF = 8, /* Typedef */ BTF_KIND_VOLATILE = 9, /* Volatile */ @@ -76,6 +76,7 @@ enum { BTF_KIND_FLOAT = 16, /* Floating point */ BTF_KIND_DECL_TAG = 17, /* Decl Tag */ BTF_KIND_TYPE_TAG = 18, /* Type Tag */ + BTF_KIND_ENUM64 = 19, /* Enumeration for long/unsigned long values */ NR_BTF_KINDS, BTF_KIND_MAX = NR_BTF_KINDS - 1, @@ -186,4 +187,14 @@ struct btf_decl_tag { __s32 component_idx; }; +/* BTF_KIND_ENUM64 is followed by multiple "struct btf_enum64". + * The exact number of btf_enum64 is stored in the vlen (of the + * info in "struct btf_type"). + */ +struct btf_enum64 { + __u32 name_off; + __u32 hi32; + __u32 lo32; +}; + #endif /* _UAPI__LINUX_BTF_H__ */
Currently, BTF only supports upto 32bit enum value with BTF_KIND_ENUM. But in kernel, some enum indeed has 64bit values, e.g., in uapi bpf.h, we have enum { BPF_F_INDEX_MASK = 0xffffffffULL, BPF_F_CURRENT_CPU = BPF_F_INDEX_MASK, BPF_F_CTXLEN_MASK = (0xfffffULL << 32), }; In this case, BTF_KIND_ENUM will encode the value of BPF_F_CTXLEN_MASK as 0, which certainly is incorrect. This patch added a new btf kind, BTF_KIND_ENUM64, which permits 64bit value to cover the above use case. The BTF_KIND_ENUM64 has the following three bytes followed by the common type: struct bpf_enum64 { __u32 nume_off; __u32 hi32; __u32 lo32; }; Currently, btf type section has an alignment of 4 as all element types are u32. Representing the value with __u64 will introduce a pad for bpf_enum64 and may also introduce misalignment for the 64bit value. Hence, two members of hi32 and lo32 are chosen to avoid these issues. The kflag is also introduced for BTF_KIND_ENUM and BTF_KIND_ENUM64 to indicate whether the value is signed or unsigned. The kflag intends to provide consistent output of BTF C fortmat with the original source code. For example, the original BTF_KIND_ENUM bit value is 0xffffffff. The format C has two choices, print out 0xffffffff or -1 and current libbpf prints out as unsigned value. But if the signedness is preserved in btf, the value can be printed the same as the original source code. The new BTF_KIND_ENUM64 is intended to support the enum value represented as 64bit value. But it can represent all BTF_KIND_ENUM values as well. The value size of BTF_KIND_ENUM64 is encoded to 8 to represent its intent. The compiler ([1]) and pahole will generate BTF_KIND_ENUM64 only if the value has to be represented with 64 bits. [1] https://reviews.llvm.org/D124641 Signed-off-by: Yonghong Song <yhs@fb.com> --- include/linux/btf.h | 18 ++++- include/uapi/linux/btf.h | 17 ++++- kernel/bpf/btf.c | 132 ++++++++++++++++++++++++++++++--- tools/include/uapi/linux/btf.h | 17 ++++- 4 files changed, 168 insertions(+), 16 deletions(-)