Re: [PATCH v6 bpf-next 6/9] bpf: Add BTF_KIND_FLOAT support

[Yonghong Song <yhs@fb.com>]

On 2/25/21 2:40 AM, Ilya Leoshkevich wrote:
> On Wed, 2021-02-24 at 23:10 -0800, Yonghong Song wrote:
>> On 2/24/21 3:45 PM, Ilya Leoshkevich wrote:
>>> On the kernel side, introduce a new btf_kind_operations. It is
>>> similar to that of BTF_KIND_INT, however, it does not need to
>>> handle encodings and bit offsets. Do not implement printing, since
>>> the kernel does not know how to format floating-point values.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    kernel/bpf/btf.c | 79
>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>    1 file changed, 77 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index 2efeb5f4b343..c405edc8e615 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
> 
> [...]
> 
>>> @@ -1849,7 +1852,7 @@ static int btf_df_check_kflag_member(struct
>>> btf_verifier_env *env,
>>>          return -EINVAL;
>>>    }
>>>    
>>> -/* Used for ptr, array and struct/union type members.
>>> +/* Used for ptr, array struct/union and float type members.
>>>     * int, enum and modifier types have their specific callback
>>> functions.
>>>     */
>>>    static int btf_generic_check_kflag_member(struct btf_verifier_env
>>> *env,
>>> @@ -3675,6 +3678,77 @@ static const struct btf_kind_operations
>>> datasec_ops = {
>>>          .show                   = btf_datasec_show,
>>>    };
>>>    
>>> +static s32 btf_float_check_meta(struct btf_verifier_env *env,
>>> +                               const struct btf_type *t,
>>> +                               u32 meta_left)
>>> +{
>>> +       if (btf_type_vlen(t)) {
>>> +               btf_verifier_log_type(env, t, "vlen != 0");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if (btf_type_kflag(t)) {
>>> +               btf_verifier_log_type(env, t, "Invalid btf_info
>>> kind_flag");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if (t->size != 2 && t->size != 4 && t->size != 8 && t->size
>>> != 12 &&
>>> +           t->size != 16) {
>>> +               btf_verifier_log_type(env, t, "Invalid type_size");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       btf_verifier_log_type(env, t, NULL);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int btf_float_check_member(struct btf_verifier_env *env,
>>> +                                 const struct btf_type
>>> *struct_type,
>>> +                                 const struct btf_member *member,
>>> +                                 const struct btf_type
>>> *member_type)
>>> +{
>>> +       u64 start_offset_bytes;
>>> +       u64 end_offset_bytes;
>>> +       u64 misalign_bits;
>>> +       u64 align_bytes;
>>> +       u64 align_bits;
>>> +
>>> +       align_bytes = min_t(u64, sizeof(void *), member_type-
>>>> size);
>>
>> I listed the following possible (size, align) pairs:
>>       size     x86_32 align_bytes   x86_64 align bytes
>>        2        2                    2
>>        4        4                    4
>>        8        4                    8
>>        12       4                    8
>>        16       4                    8
>>
>> A few observations.
>>     1. I don't know, just want to confirm, for double, the alignment
>> could be 4 (for a member) on 32bit system, is that right?
>>     2. for size 12, alignment will be 8 for x86_64 system, this is
>> strange, not sure whether it is true or not. Or size 12 cannot be
>> on x86_64 and we should error out if sizeof(void *) is 8.
> 
> 1 - Yes.
> 
> 2 - On x86_64 long double is 16 bytes and the required alignment is 16
> bytes too. However, on other architectures all this might be different.
> For example, for us long double is 16 bytes too, but the alignment can
> be 8. So can we be somewhat lax here and just allow smaller alignments,
> instead of trying to figure out what exactly each supported
> architecture does?

Maybe this is fine. I think, "float" is also the first BTF type whose
validation may have a dependence on underlying architecture. For 
example, member offset 4, type size 8, will be okay on x86_32,
but not okay on x84_64. That means BTF cannot be independently
validated without considering underlying architecture.

I am not against this patch. Maybe float is special. Maybe it is
okay since float is rarely used. I would like to see other people's
opinion.

> 
> [...]
>>
>