Date:   Sat, 15 Dec 2018 23:12:19 +0000
From:   Yonghong Song <yhs@...com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Alexei Starovoitov <ast@...com>, Kernel Team <Kernel-team@...com>,
        "Martin Lau" <kafai@...com>
Subject: Re: [PATCH bpf-next v2 2/8] bpf: btf: fix struct/union/fwd types with
 kind_flag



On 12/15/18 2:37 PM, Daniel Borkmann wrote:
> 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) ...

The test here is strictly checking whether the bitfield size is covered 
by underlying int type.
    struct t {
       int a:1;
       char b:8;
       ...
    }
In this case, for member "b", bitsize = 8, nr_copy_bits = 9, 
nr_int_data_bits = 8. nr_copy_bits > nr_int_data_bits, but it is legal.


> Wrt to future 256 bit support, doesn't UAPI on BTF_INT_BITS() only support
> up to max 255 bits?

This is a good question. BTF_INT_BITS() can be extended to maximum 
0xffff. There are bits available there.

When the kind_flag is set, the bitfield size can range from 1 to 255,
the same as today BTF_INT_BITS range. If user writes
struct t {
    ...
    int256 m:256;
    ...
};
The bitfield size information will not get encoded and the member "m" 
will just refer to a base type int256, the same as
struct t {
    ...
    int256 m;
    ...
};

Did you any problem with this?

> 
>> +		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;
>> +}

Powered by blists - more mailing lists