Message-ID: <2650e4ae-f013-df7b-4baf-579f15a2b19c@fb.com>
Date:   Sat, 15 Dec 2018 17:42:38 +0000
From:   Yonghong Song <yhs@...com>
To:     Martin Lau <kafai@...com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 2/8] bpf: btf: fix struct/union/fwd types with
 kind_flag



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 <kafai@...com>
>> Signed-off-by: Yonghong Song <yhs@...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;
>> +}
>> +

Powered by blists - more mailing lists