diff mbox series

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

Message ID 20210224234535.106970-7-iii@linux.ibm.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Add BTF_KIND_FLOAT support | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 4 maintainers not CCed: netdev@vger.kernel.org kafai@fb.com kpsingh@kernel.org songliubraving@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 121 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Ilya Leoshkevich Feb. 24, 2021, 11:45 p.m. UTC
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(-)

Comments

Yonghong Song Feb. 25, 2021, 7:10 a.m. UTC | #1
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
> @@ -173,7 +173,7 @@
>   #define BITS_ROUNDUP_BYTES(bits) \
>   	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
>   
> -#define BTF_INFO_MASK 0x8f00ffff
> +#define BTF_INFO_MASK 0x9f00ffff
>   #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)
> @@ -280,6 +280,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
>   	[BTF_KIND_FUNC_PROTO]	= "FUNC_PROTO",
>   	[BTF_KIND_VAR]		= "VAR",
>   	[BTF_KIND_DATASEC]	= "DATASEC",
> +	[BTF_KIND_FLOAT]	= "FLOAT",
>   };
>   
>   static const char *btf_type_str(const struct btf_type *t)
> @@ -574,6 +575,7 @@ static bool btf_type_has_size(const struct btf_type *t)
>   	case BTF_KIND_UNION:
>   	case BTF_KIND_ENUM:
>   	case BTF_KIND_DATASEC:
> +	case BTF_KIND_FLOAT:
>   		return true;
>   	}
>   
> @@ -1704,6 +1706,7 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>   		case BTF_KIND_STRUCT:
>   		case BTF_KIND_UNION:
>   		case BTF_KIND_ENUM:
> +		case BTF_KIND_FLOAT:
>   			size = type->size;
>   			goto resolved;
>   
> @@ -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.

> +	align_bits = align_bytes * BITS_PER_BYTE;
> +	div64_u64_rem(member->offset, align_bits, &misalign_bits);
> +	if (misalign_bits) {
> +		btf_verifier_log_member(env, struct_type, member,
> +					"Member is not properly aligned");
> +		return -EINVAL;
> +	}
> +
> +	start_offset_bytes = member->offset / BITS_PER_BYTE;
> +	end_offset_bytes = start_offset_bytes + member_type->size;
> +	if (end_offset_bytes > struct_type->size) {
> +		btf_verifier_log_member(env, struct_type, member,
> +					"Member exceeds struct_size");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void btf_float_log(struct btf_verifier_env *env,
> +			  const struct btf_type *t)
> +{
> +	btf_verifier_log(env, "size=%u", t->size);
> +}
> +
> +static const struct btf_kind_operations float_ops = {
> +	.check_meta = btf_float_check_meta,
> +	.resolve = btf_df_resolve,
> +	.check_member = btf_float_check_member,
> +	.check_kflag_member = btf_generic_check_kflag_member,
> +	.log_details = btf_float_log,
> +	.show = btf_df_show,
> +};
> +
>   static int btf_func_proto_check(struct btf_verifier_env *env,
>   				const struct btf_type *t)
>   {
> @@ -3808,6 +3882,7 @@ static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = {
>   	[BTF_KIND_FUNC_PROTO] = &func_proto_ops,
>   	[BTF_KIND_VAR] = &var_ops,
>   	[BTF_KIND_DATASEC] = &datasec_ops,
> +	[BTF_KIND_FLOAT] = &float_ops,
>   };
>   
>   static s32 btf_check_meta(struct btf_verifier_env *env,
>
Ilya Leoshkevich Feb. 25, 2021, 10:40 a.m. UTC | #2
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?

[...]
>
Yonghong Song Feb. 25, 2021, 2:59 p.m. UTC | #3
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.

> 
> [...]
>>
>
John Fastabend Feb. 26, 2021, 1:53 a.m. UTC | #4
Yonghong Song wrote:
> 
> 
> 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.

I can't think of any specific issue here. From the program/BTF side
the actual offsets of any given field in a struct are going to vary
wildly depending on arch and configuration anyways.

I assume if a BPF program really needs the size then 
__builtin_preserve_field_info(var, BPF_FIELD_BYTE_SIZE) will do the
right thing?

Thanks!
Yonghong Song Feb. 26, 2021, 5:43 a.m. UTC | #5
On 2/25/21 5:53 PM, John Fastabend wrote:
> Yonghong Song wrote:
>>
>>
>> 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.
> 
> I can't think of any specific issue here. From the program/BTF side
> the actual offsets of any given field in a struct are going to vary
> wildly depending on arch and configuration anyways.

That is true.

> 
> I assume if a BPF program really needs the size then
> __builtin_preserve_field_info(var, BPF_FIELD_BYTE_SIZE) will do the
> right thing?

For __builtin_preserve_field_info(var, BPF_FIELD_BYTE_SIZE), libbpf
should be able to check vmlinux to find correct field size and use
it.

All my above opinion is for bpf program BTF. I agree that if libbpf
CO-RE kicks in, we should be able to get proper aligned size/offset.

for float, if I understand correctly, we don't print data yet, so
even print, maybe just bytes, so alignment requirement is not critical.

Ilya, maybe just add some comments like different arch has different
alignment requirements so here we just ensure minimum alignment 
requirement this way we ensure types after CO-RE can pass kernel
btf verifier?

> 
> Thanks!
>
diff mbox series

Patch

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
@@ -173,7 +173,7 @@ 
 #define BITS_ROUNDUP_BYTES(bits) \
 	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
 
-#define BTF_INFO_MASK 0x8f00ffff
+#define BTF_INFO_MASK 0x9f00ffff
 #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)
@@ -280,6 +280,7 @@  static const char * const btf_kind_str[NR_BTF_KINDS] = {
 	[BTF_KIND_FUNC_PROTO]	= "FUNC_PROTO",
 	[BTF_KIND_VAR]		= "VAR",
 	[BTF_KIND_DATASEC]	= "DATASEC",
+	[BTF_KIND_FLOAT]	= "FLOAT",
 };
 
 static const char *btf_type_str(const struct btf_type *t)
@@ -574,6 +575,7 @@  static bool btf_type_has_size(const struct btf_type *t)
 	case BTF_KIND_UNION:
 	case BTF_KIND_ENUM:
 	case BTF_KIND_DATASEC:
+	case BTF_KIND_FLOAT:
 		return true;
 	}
 
@@ -1704,6 +1706,7 @@  __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		case BTF_KIND_STRUCT:
 		case BTF_KIND_UNION:
 		case BTF_KIND_ENUM:
+		case BTF_KIND_FLOAT:
 			size = type->size;
 			goto resolved;
 
@@ -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);
+	align_bits = align_bytes * BITS_PER_BYTE;
+	div64_u64_rem(member->offset, align_bits, &misalign_bits);
+	if (misalign_bits) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Member is not properly aligned");
+		return -EINVAL;
+	}
+
+	start_offset_bytes = member->offset / BITS_PER_BYTE;
+	end_offset_bytes = start_offset_bytes + member_type->size;
+	if (end_offset_bytes > struct_type->size) {
+		btf_verifier_log_member(env, struct_type, member,
+					"Member exceeds struct_size");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void btf_float_log(struct btf_verifier_env *env,
+			  const struct btf_type *t)
+{
+	btf_verifier_log(env, "size=%u", t->size);
+}
+
+static const struct btf_kind_operations float_ops = {
+	.check_meta = btf_float_check_meta,
+	.resolve = btf_df_resolve,
+	.check_member = btf_float_check_member,
+	.check_kflag_member = btf_generic_check_kflag_member,
+	.log_details = btf_float_log,
+	.show = btf_df_show,
+};
+
 static int btf_func_proto_check(struct btf_verifier_env *env,
 				const struct btf_type *t)
 {
@@ -3808,6 +3882,7 @@  static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = {
 	[BTF_KIND_FUNC_PROTO] = &func_proto_ops,
 	[BTF_KIND_VAR] = &var_ops,
 	[BTF_KIND_DATASEC] = &datasec_ops,
+	[BTF_KIND_FLOAT] = &float_ops,
 };
 
 static s32 btf_check_meta(struct btf_verifier_env *env,