On 12/15/2018 12:34 AM, 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.
Looks good in general, just small nit below.
> 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
[...]
> +static int btf_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)
> +{
> + u32 struct_bits_off, nr_bits, nr_int_data_bits, bytes_offset;
> + u32 int_data = btf_type_int(member_type);
> + u32 struct_size = struct_type->size;
> + u32 nr_copy_bits;
> +
> + /* a regular int type is required for the kflag int member */
> + if (!btf_type_int_is_regular(member_type)) {
> + btf_verifier_log_member(env, struct_type, member,
> + "Invalid member base type");
> + return -EINVAL;
> + }
> +
> + /* check sanity of bitfield size */
> + nr_bits = BTF_MEMBER_BITFIELD_SIZE(member->offset);
> + struct_bits_off = BTF_MEMBER_BIT_OFFSET(member->offset);
> + nr_int_data_bits = BTF_INT_BITS(int_data);
> + if (!nr_bits) {
> + /* Not a bitfield member, member offset must be at byte
> + * boundary.
> + */
> + if (BITS_PER_BYTE_MASKED(struct_bits_off)) {
> + btf_verifier_log_member(env, struct_type, member,
> + "Invalid member offset");
> + return -EINVAL;
> + }
> +
> + nr_bits = nr_int_data_bits;
> + } else if (nr_bits > nr_int_data_bits) {
Should the test here not include the bit offset as well aka nr_copy_bits?
Thus test would be e.g. (nr_copy_bits > nr_int_data_bits || nr_copy_bits >
BITS_PER_U64) ...
Wrt to future 256 bit support, doesn't UAPI on BTF_INT_BITS() only support
up to max 255 bits?
> + btf_verifier_log_member(env, struct_type, member,
> + "Invalid member bitfield_size");
> + return -EINVAL;
> + }
> +
> + bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off);
> + nr_copy_bits = nr_bits + BITS_PER_BYTE_MASKED(struct_bits_off);
> + if (nr_copy_bits > BITS_PER_U64) {
> + btf_verifier_log_member(env, struct_type, member,
> + "nr_copy_bits exceeds 64");
> + return -EINVAL;
> + }
> +
> + if (struct_size < bytes_offset ||
> + struct_size - bytes_offset < BITS_ROUNDUP_BYTES(nr_copy_bits)) {
> + btf_verifier_log_member(env, struct_type, member,
> + "Member exceeds struct_size");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
