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@xxxxxxxxxxxxx>
> >>> ---
> >>> 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!
