Message-ID: <20181215163648.5lnhguk7b43hzy6w@kafai-mbp>
Date:   Sat, 15 Dec 2018 16:37:06 +0000
From:   Martin Lau <kafai@...com>
To:     Yonghong Song <yhs@...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 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.

> +#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).

> +
>  /* 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).

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