Re: [PATCH v2 bpf-next 1/9] btf: add kind layout encoding, crcs to UAPI

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Jul 3, 2023 at 6:42 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 22/06/2023 23:02, Andrii Nakryiko wrote:
> > On Fri, Jun 16, 2023 at 10:17 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>
> >> BTF kind layouts provide information to parse BTF kinds.
> >> By separating parsing BTF from using all the information
> >> it provides, we allow BTF to encode new features even if
> >> they cannot be used.  This is helpful in particular for
> >> cases where newer tools for BTF generation run on an
> >> older kernel; BTF kinds may be present that the kernel
> >> cannot yet use, but at least it can parse the BTF
> >> provided.  Meanwhile userspace tools with newer libbpf
> >> may be able to use the newer information.
> >>
> >> The intent is to support encoding of kind layouts
> >> optionally so that tools like pahole can add this
> >> information.  So for each kind we record
> >>
> >> - kind-related flags
> >> - length of singular element following struct btf_type
> >> - length of each of the btf_vlen() elements following
> >>
> >> In addition we make space in the BTF header for
> >> CRC32s computed over the BTF along with a CRC for
> >> the base BTF; this allows split BTF to identify
> >> a mismatch explicitly.
> >>
> >> The ideas here were discussed at [1], [2]; hence
> >>
> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >>
> >> [1] https://lore.kernel.org/bpf/CAEf4BzYjWHRdNNw4B=eOXOs_ONrDwrgX4bn=Nuc1g8JPFC34MA@mail.gmail.com/
> >> [2] https://lore.kernel.org/bpf/20230531201936.1992188-1-alan.maguire@oracle.com/
> >> ---
> >>  include/uapi/linux/btf.h       | 24 ++++++++++++++++++++++++
> >>  tools/include/uapi/linux/btf.h | 24 ++++++++++++++++++++++++
> >>  2 files changed, 48 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> >> index ec1798b6d3ff..cea9125ed953 100644
> >> --- a/include/uapi/linux/btf.h
> >> +++ b/include/uapi/linux/btf.h
> >> @@ -8,6 +8,22 @@
> >>  #define BTF_MAGIC      0xeB9F
> >>  #define BTF_VERSION    1
> >>
> >> +/* is this information required? If so it cannot be sanitized safely. */
> >> +#define BTF_KIND_LAYOUT_OPTIONAL               (1 << 0)
> >
> > hm.. I thought we agreed to not have OPTIONAL flag last time, no? From
> > kernel's perspective nothing is optional. From libbpf perspective
> > everything should be optional, unless we get type_id reference to
> > something that we don't recognize. So why the flag and extra code to
> > handle it?
> >
> > We can always add it later, if necessary.
> >
>
> I totally agree we need to reject any BTF that contains references
> to unknown objects if these references are made via known ones;
> so for example an enum64 in a struct (in the case we didn't know
> about an enum64). However, it's possible a BTF kind could point
> _at_ other BTF kinds but not be pointed _to_ by any known kinds;
> in such a case wouldn't optional make sense even for the kernel
> to say "ignore any kinds that we don't know about that aren't
> participating in any known BTF relationships"? Default assumption
> without the optional flag would be to reject such BTF.

I think it's simpler (and would follow what we've been doing with
kernel-side strict validation of everything) to reject everything
unknown. "Being pointed to" isn't always contained within BTF itself.
E.g., for line and func info, type_id comes during BPF_PROG_LOAD. So
at the point of BTF validation you don't know that some FUNCs will be
pointed to (as an example). So I'd avoid making unnecessarily
dangerous assumptions that some pieces of information can be ignored.

And in general, kernel doesn't trust user-space data without
validation, so we'd have to double-check all this OPTIONAL flags
somehow anyways.

>
> >> +
> >> +/* kind layout section consists of a struct btf_kind_layout for each known
> >> + * kind at BTF encoding time.
> >> + */
> >> +struct btf_kind_layout {
> >> +       __u16 flags;            /* see BTF_KIND_LAYOUT_* values above */
> >> +       __u8 info_sz;           /* size of singular element after btf_type */
> >> +       __u8 elem_sz;           /* size of each of btf_vlen(t) elements */
> >> +};
> >> +
> >> +/* for CRCs for BTF, base BTF to be considered usable, flags must be set. */
> >> +#define BTF_FLAG_CRC_SET               (1 << 0)
> >> +#define BTF_FLAG_BASE_CRC_SET          (1 << 1)
> >> +
> >>  struct btf_header {
> >>         __u16   magic;
> >>         __u8    version;
> >> @@ -19,8 +35,16 @@ struct btf_header {
> >>         __u32   type_len;       /* length of type section       */
> >>         __u32   str_off;        /* offset of string section     */
> >>         __u32   str_len;        /* length of string section     */
> >> +       __u32   kind_layout_off;/* offset of kind layout section */
> >> +       __u32   kind_layout_len;/* length of kind layout section */
> >> +
> >> +       __u32   crc;            /* crc of BTF; used if flags set BTF_FLAG_CRC_VALID */
> >> +       __u32   base_crc;       /* crc of base BTF; used if flags set BTF_FLAG_BASE_CRC_VALID */
> >>  };
> >>
> >> +/* required minimum BTF header length */
> >> +#define BTF_HEADER_MIN_LEN     (sizeof(struct btf_header) - 16)
> >
> > offsetof(struct btf_header, kind_layout_off) ?
> >
> > but actually why this needs to be a part of UAPI?
> >
>
> no not really. I was trying to come up with a more elegant
> way of differentiating between the old and new header formats
> on the basis of size and eventually just gave up and added
> a #define. It can absolutely be removed.

right, so that's why just checking if field is present based on
btf_header.len and field offset is a good approach? Let's drop
unnecessary constants from UAPI header

>
> >> +
> >>  /* Max # of type identifier */
> >>  #define BTF_MAX_TYPE   0x000fffff
> >>  /* Max offset into the string section */
> >> diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
> >> index ec1798b6d3ff..cea9125ed953 100644
> >> --- a/tools/include/uapi/linux/btf.h
> >> +++ b/tools/include/uapi/linux/btf.h
> >> @@ -8,6 +8,22 @@
> >>  #define BTF_MAGIC      0xeB9F
> >>  #define BTF_VERSION    1
> >>
> >> +/* is this information required? If so it cannot be sanitized safely. */
> >> +#define BTF_KIND_LAYOUT_OPTIONAL               (1 << 0)
> >> +
> >> +/* kind layout section consists of a struct btf_kind_layout for each known
> >> + * kind at BTF encoding time.
> >> + */
> >> +struct btf_kind_layout {
> >> +       __u16 flags;            /* see BTF_KIND_LAYOUT_* values above */
> >> +       __u8 info_sz;           /* size of singular element after btf_type */
> >> +       __u8 elem_sz;           /* size of each of btf_vlen(t) elements */
> >> +};
> >> +
> >> +/* for CRCs for BTF, base BTF to be considered usable, flags must be set. */
> >> +#define BTF_FLAG_CRC_SET               (1 << 0)
> >> +#define BTF_FLAG_BASE_CRC_SET          (1 << 1)
> >> +
> >>  struct btf_header {
> >>         __u16   magic;
> >>         __u8    version;
> >> @@ -19,8 +35,16 @@ struct btf_header {
> >>         __u32   type_len;       /* length of type section       */
> >>         __u32   str_off;        /* offset of string section     */
> >>         __u32   str_len;        /* length of string section     */
> >> +       __u32   kind_layout_off;/* offset of kind layout section */
> >> +       __u32   kind_layout_len;/* length of kind layout section */
> >> +
> >> +       __u32   crc;            /* crc of BTF; used if flags set BTF_FLAG_CRC_VALID */
> >
> > why are we making crc optional? shouldn't we just say that crc is
> > always filled out?
> >
>
> The approach I took was to have libbpf/pahole be flexible about
> specification of crcs and kind layout; neither, one of these or both
> are supported. When neither are specified we'll still generate
> a larger header, but it will be zeros for the new fields so parseable
> by older libbpf/kernel. I think we probably need to make it optional
> for a while to support new pahole on older kernels.

I'm not sure how this "optional for a while" will turn to
"non-optional", tbh, and who and when will decide that. I think the
"new pahole on old kernel" problem is solvable easily by making all
this new stuff opt-in. New kernel Makefiles will request pahole to
emit them, if pahole is new enough. Old kernels won't know about this
feature and even new pahole won't emit it.

I don't feel too strongly about it, just generally feeling like
minimizing all the different supportable variations.

>
>
> >> +       __u32   base_crc;       /* crc of base BTF; used if flags set BTF_FLAG_BASE_CRC_VALID */
> >
> > here it would be nice if we could just rely on zero meaning not set,
> > but I suspect not everyone will be happy about this, as technically
> > crc 0 is a valid crc :(
> >
>
> Right, I think we're stuck with the flags unfortunately.

yep. one extra reason why I like the idea of this being string offset,
but whatever, small thing


> Thanks for the review (and apologies for the belated response!)
>
> Alan
>
> >
> >>  };
> >>
> >> +/* required minimum BTF header length */
> >> +#define BTF_HEADER_MIN_LEN     (sizeof(struct btf_header) - 16)
> >> +
> >>  /* Max # of type identifier */
> >>  #define BTF_MAX_TYPE   0x000fffff
> >>  /* Max offset into the string section */
> >> --
> >> 2.39.3
> >>