On 12/15/18 8:37 AM, Martin Lau wrote:
> On Fri, Dec 14, 2018 at 03:34:27PM -0800, Yonghong Song wrote:
>> This patch fixed two issues with BTF. One is related to
>> struct/union bitfield encoding and the other is related to
>> forward type.
>>
>> Issue #1 and solution:
>> ======================
>>
>> Current btf encoding of bitfield follows what pahole generates.
>> For each bitfield, pahole will duplicate the type chain and
>> put the bitfield size at the final int or enum type.
>> Since the BTF enum type cannot encode bit size,
>> pahole workarounds the issue by generating
>> an int type whenever the enum bit size is not 32.
>>
>> For example,
>> -bash-4.4$ cat t.c
>> typedef int ___int;
>> enum A { A1, A2, A3 };
>> struct t {
>> int a[5];
>> ___int b:4;
>> volatile enum A c:4;
>> } g;
>> -bash-4.4$ gcc -c -O2 -g t.c
>> The current kernel supports the following BTF encoding:
>> $ pahole -JV t.o
>> [1] TYPEDEF ___int type_id=2
>> [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
>> [3] ENUM A size=4 vlen=3
>> A1 val=0
>> A2 val=1
>> A3 val=2
>> [4] STRUCT t size=24 vlen=3
>> a type_id=5 bits_offset=0
>> b type_id=9 bits_offset=160
>> c type_id=11 bits_offset=164
>> [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
>> [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
>> [7] VOLATILE (anon) type_id=3
>> [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
>> [9] TYPEDEF ___int type_id=8
>> [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
>> [11] VOLATILE (anon) type_id=10
>>
>> Two issues are in the above:
>> . by changing enum type to int, we lost the original
>> type information and this will not be ideal later
>> when we try to convert BTF to a header file.
>> . the type duplication for bitfields will cause
>> BTF bloat. Duplicated types cannot be deduplicated
>> later if the bitfield size is different.
>>
>> To fix this issue, this patch implemented a compatible
>> change for BTF struct type encoding:
>> . the bit 31 of struct_type->info, previously reserved,
>> now is used to indicate whether bitfield_size is
>> encoded in btf_member or not.
>> . if bit 31 of struct_type->info is set,
>> btf_member->offset will encode like:
>> bit 0 - 23: bit offset
>> bit 24 - 31: bitfield size
>> if bit 31 is not set, the old behavior is preserved:
>> bit 0 - 31: bit offset
>>
>> So if the struct contains a bit field, the maximum bit offset
>> will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum
>> bitfield size will be 256 which is enough for today as maximum
>> bitfield in compiler can be 128 where int128 type is supported.
>>
>> This kernel patch intends to support the new BTF encoding:
>> $ pahole -JV t.o
>> [1] TYPEDEF ___int type_id=2
>> [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
>> [3] ENUM A size=4 vlen=3
>> A1 val=0
>> A2 val=1
>> A3 val=2
>> [4] STRUCT t kind_flag=1 size=24 vlen=3
>> a type_id=5 bitfield_size=0 bits_offset=0
>> b type_id=1 bitfield_size=4 bits_offset=160
>> c type_id=7 bitfield_size=4 bits_offset=164
>> [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
>> [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
>> [7] VOLATILE (anon) type_id=3
>>
>> Issue #2 and solution:
>> ======================
>>
>> Current forward type in BTF does not specify whether the original
>> type is struct or union. This will not work for type pretty print
>> and BTF-to-header-file conversion as struct/union must be specified.
>> $ cat tt.c
>> struct t;
>> union u;
>> int foo(struct t *t, union u *u) { return 0; }
>> $ gcc -c -g -O2 tt.c
>> $ pahole -JV tt.o
>> [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
>> [2] FWD t type_id=0
>> [3] PTR (anon) type_id=2
>> [4] FWD u type_id=0
>> [5] PTR (anon) type_id=4
>>
>> To fix this issue, similar to issue #1, type->info bit 31
>> is used. If the bit is set, it is union type. Otherwise, it is
>> a struct type.
>>
>> $ pahole -JV tt.o
>> [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
>> [2] FWD t kind_flag=0 type_id=0
>> [3] PTR (anon) kind_flag=0 type_id=2
>> [4] FWD u kind_flag=1 type_id=0
>> [5] PTR (anon) kind_flag=0 type_id=4
>>
>> Pahole/LLVM change:
>> ===================
>>
>> The new kind_flag functionality has been implemented in pahole
>> and llvm:
>> https://github.com/yonghong-song/pahole/tree/bitfield
>> https://github.com/yonghong-song/llvm/tree/bitfield
>>
>> Note that pahole hasn't implemented func/func_proto kind
>> and .BTF.ext. So to print function signature with bpftool,
>> the llvm compiler should be used.
>>
>> Fixes: 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")
>> Signed-off-by: Martin KaFai Lau <ka...@fb.com>
>> Signed-off-by: Yonghong Song <y...@fb.com>
>> ---
>> include/uapi/linux/btf.h | 15 ++-
>> kernel/bpf/btf.c | 274 ++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 267 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
>> index 14f66948fc95..34aba40ed926 100644
>> --- a/include/uapi/linux/btf.h
>> +++ b/include/uapi/linux/btf.h
>> @@ -34,7 +34,9 @@ struct btf_type {
>> * bits 0-15: vlen (e.g. # of struct's members)
>> * bits 16-23: unused
>> * bits 24-27: kind (e.g. int, ptr, array...etc)
>> - * bits 28-31: unused
>> + * bits 28-30: unused
>> + * bit 31: kind_flag, currently used by
>> + * struct, union and fwd
>> */
>> __u32 info;
>> /* "size" is used by INT, ENUM, STRUCT and UNION.
>> @@ -52,6 +54,7 @@ struct btf_type {
>>
>> #define BTF_INFO_KIND(info) (((info) >> 24) & 0x0f)
>> #define BTF_INFO_VLEN(info) ((info) & 0xffff)
>> +#define BTF_INFO_KFLAG(info) ((info) >> 31)
>>
>> #define BTF_KIND_UNKN 0 /* Unknown */
>> #define BTF_KIND_INT 1 /* Integer */
>> @@ -110,9 +113,17 @@ struct btf_array {
>> struct btf_member {
>> __u32 name_off;
>> __u32 type;
>> - __u32 offset; /* offset in bits */
>> + __u32 offset; /* [bitfield_size and] offset in bits */
>> };
>>
>> +/* If the type info kind_flag set, the btf_member.offset
>> + * contains both member bit offset and bitfield size, and
>> + * bitfield size will set for struct/union bitfield members.
>> + * Otherwise, it contains only bit offset.
>> + */
> nit. It may be better to move this comment to the btf_member.offset
> above.
Make sense. Will have smaller comments as well before the below two
macros to indicate kind_flag needs to be set for the below two
macros make sense.
>
>> +#define BTF_MEMBER_BITFIELD_SIZE(val) ((val) >> 24)
>> +#define BTF_MEMBER_BIT_OFFSET(val) ((val) & 0xffffff)
> After re-thinking this setup again, I still think
> having these macros in btf.h to also do the kflag checking
> would be nice.
>
> Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't
> depend on other facts, the btf.h raw user must check kflag
> anyway before calling BTF_MEMBER_BIT*().
> Forcing a kflag check before the user can access these convenient
> 0xfffff and >>24 conversions may enforce this kflag check to
> some extend.
>
> Since it is in uapi, it will not be easy to change later.
> The above concern could be overkill ;), just want to ensure
> it has been thought through a bit more here.
>
> It could be as easy as moving the new btf_member_bit*() from
> btf.c to here and remove these two macros (or move them back to btf.c).
Not an expert in uapi design. I did not put static inline function
(check kflag and get bitfield_size/offset) there with following reasons.
(1). The user/kernel may want to check kflag either to simplify their
code or with certain guaranteed properties, e.g., extra
offset/regular_int checking. So putting macros in the header file will
be good and necessary as we do not want end user (kernel/usr) to have
"shift" or "and" in their codes.
(2). putting static inline functions there feels a little bit redundant
and could be misleading here if used blindly. First, we have macros here
which you can already get information. Second, if user just uses
btf_member_bitfield_size() and assumes that bitfield_size = 0, that will
be wrong since bitfield_size = 0 only if kflag is set, user needs to
trace to base int to find real bitfield size if kflag = 0. So user has
to be aware of kflag even if he calls btf_member_bitfield_size().
>
>> +
>> /* BTF_KIND_FUNC_PROTO is followed by multiple "struct btf_param".
>> * The exact number of btf_param is stored in the vlen (of the
>> * info in "struct btf_type").
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 72caa799e82f..ec3f80d3bef6 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -164,7 +164,7 @@
>> #define BITS_ROUNDUP_BYTES(bits) \
>> (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
>>
>> -#define BTF_INFO_MASK 0x0f00ffff
>> +#define BTF_INFO_MASK 0x8f00ffff
>> #define BTF_INT_MASK 0x0fffffff
>> #define BTF_TYPE_ID_VALID(type_id) ((type_id) <= BTF_MAX_TYPE)
>> #define BTF_STR_OFFSET_VALID(name_off) ((name_off) <= BTF_MAX_NAME_OFFSET)
>> @@ -274,6 +274,10 @@ struct btf_kind_operations {
>> const struct btf_type *struct_type,
>> const struct btf_member *member,
>> const struct btf_type *member_type);
>> + int (*check_kflag_member)(struct btf_verifier_env *env,
>> + const struct btf_type *struct_type,
>> + const struct btf_member *member,
>> + const struct btf_type *member_type);
>> void (*log_details)(struct btf_verifier_env *env,
>> const struct btf_type *t);
>> void (*seq_show)(const struct btf *btf, const struct btf_type *t,
>> @@ -419,6 +423,25 @@ static u16 btf_type_vlen(const struct btf_type *t)
>> return BTF_INFO_VLEN(t->info);
>> }
>>
>
> [ ... ]
>
>> +static int btf_enum_check_kflag_member(struct btf_verifier_env *env,
>> + const struct btf_type *struct_type,
>> + const struct btf_member *member,
>> + const struct btf_type *member_type)
>> +{
>> + u32 struct_bits_off, nr_bits, bytes_end, struct_size;
>> + u32 int_bitsize = sizeof(int) * BITS_PER_BYTE;
>> +
>> + struct_bits_off = BTF_MEMBER_BIT_OFFSET(member->offset);
>> + nr_bits = BTF_MEMBER_BITFIELD_SIZE(member->offset);
>> + if (!nr_bits) {
>> + nr_bits = int_bitsize;
> For non bitfield enum (i.e. !nr_bits), does it have to check for
> byte alignment also? i.e. BITS_PER_BYTE_MASKED(struct_bits_off).
Good catch. Will add this in next revision.
>
> Others look good.
>
>> + } else if (nr_bits > int_bitsize) {
>> + btf_verifier_log_member(env, struct_type, member,
>> + "Invalid member bitfield_size");
>> + return -EINVAL;
>> + }
>> +
>> + struct_size = struct_type->size;
>> + bytes_end = BITS_ROUNDUP_BYTES(struct_bits_off + nr_bits);
>> + if (struct_size < bytes_end) {
>> + btf_verifier_log_member(env, struct_type, member,
>> + "Member exceeds struct_size");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
