On Thu, May 29, 2025 at 5:53 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 29/05/2025 06:35, Alexei Starovoitov wrote:
> > On Wed, May 28, 2025 at 2:58 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> >>
> >> This allows BTF parsing to proceed even if we do not know the
> >> kind.
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> >> ---
> >> tools/lib/bpf/btf.c | 35 ++++++++++++++++++++++++++++-------
> >> 1 file changed, 28 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >> index 43d1fce8977c..7a197dbfc689 100644
> >> --- a/tools/lib/bpf/btf.c
> >> +++ b/tools/lib/bpf/btf.c
> >> @@ -355,7 +355,29 @@ static int btf_parse_kind_layout_sec(struct btf *btf)
> >> return 0;
> >> }
> >>
> >> -static int btf_type_size(const struct btf_type *t)
> >> +/* for unknown kinds, consult kind layout. */
> >> +static int btf_type_size_unknown(const struct btf *btf, const struct btf_type *t)
> >> +{
> >> + int size = sizeof(struct btf_type);
> >> + struct btf_kind_layout *k = NULL;
> >> + __u16 vlen = btf_vlen(t);
> >> + __u8 kind = btf_kind(t);
> >> +
> >> + if (btf->kind_layout)
> >> + k = &((struct btf_kind_layout *)btf->kind_layout)[kind];
> >> +
> >> + if (!k || (void *)k > ((void *)btf->kind_layout + btf->hdr->kind_layout_len)) {
> >> + pr_debug("Unsupported BTF_KIND: %u\n", btf_kind(t));
> >> + return -EINVAL;
> >
> > I'm missing the point around kind_layout->flags.
> > I was expecting that this helper and others at least
> > would check that flags == 0, but none of it is happening.
> > The patches say that flags is unused and do nothing.
> > Why add flags field at all?
> >
>
> The intent of the flags field is to provide space to add additional
> information about BTF kind encoding that may prove useful. E.g. at time
> of encoding for this kind, was the kind flag supported? Perhaps if the
> size/type field specifies a type or a size might be another useful flag
> setting. But basically the idea is to provide space for additional
> information around kind encoding for future use.
I feel there is a desire to add "flags" as an escape hatch,
but even for this example of "is kflag supported by this kind"
I suspect it would be incompatible to add it later.
Currently kflag is used by some, but not all kinds.
Say, we decide to make kflag meaningful for kind_int.
Currently kernel BTF validator will reject such BTF.
If we add a new flag to kind_laoyout->flags to indicate
that "at the time of encoding this kflag is supported"
the older kernel will ignore kind_layout->flags
and will error on future BTF with int's kflag.
So kind_layout->flags is really only meaningful for user tooling
that may or may not act on it.
Long ago there was an idea to use flags for "is it ok to skip
this unknown kind". imo it's the same issue. If we don't define
this flag now we won't be able to add it later and give it that meaning.
The "ok to ignore" flag will appear, but different versions
of libbpf/bpftool will either ignore it or will act on it.
That breaks the semantics of "ok to ignore".
> So in that context, should we check that flags are 0 now? I'm not sure,
> because in some cases we'd like to have older libbpf be able to handle
> newer kind layouts which might make use of flags.
yeah, if we make libbpf act as not-an-error on flags != 0 and
future version will make use of it, we're introducing an unusual
concept of flags that was never used in the kernel and libbpf.
Maybe it's ok, but I cannot wrap my head around it, since
prior knowledge and concepts don't apply to this scheme.
imo if we want extensibility for kind_layout we better use
known techniques like adding a size_of_kind_layout field to
struct btf_header, so we can add new fields to kind_layout and
do a similar check to bpf_check_uarg_tail_zero().
Then we don't have to add 'flags' there today.
We can add them later and bump the size.
I would also make
__u8 info_sz;
__u8 elem_sz;
to be __u32, since size is already __u32 in btf_type.
There is no need to save these few bytes.
With these two fields libbpf/bpftool can skip unknown kinds.
I would also add name to kind_layout, since extra hundred
bytes of strings is cheap, but warning about unknown kind
will be readable.
