On 29/05/2025 17:30, Alexei Starovoitov wrote:
> 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".
Right. The challenge here is that there are two problems we might want
to solve here. One is the easier one; separating parsing BTF from being
able to use it. The basic kind_layout info solves this because it
associates the singular and vlen-specified lengths with each kind.
The second, harder problem - which I had hoped we might be able to crack
- is how to make BTF _useful_ to older tools when it contains kinds they
do not support. We've often seen the case where distro kernels are
built with a newer pahole so have newer kind info and older tools run on
those kernels then fall over trying to read their vmlinux BTF. My
original hope was that we could provide enough context in the kind
layout to sanitize BTF with new kinds, but the challenge is to ensure
that they do not participate in the type graph (and then whether they
can be safely skipped) we need to know where type references live in
each kind representation.
Another more dynamic sort of approach to this problem would be a kind of
auto-negotiation with the kernel. Since the kernel is the source of the
newer BTF info, we could imagine asking for a mostly-equivalent BTF
representation that supports the more limited subset of features.
Because it knows what the kinds are, it is in a position to generate
such a subset. This would be in-kernel BTF sanitization similar to what
is done in libbpf for BPF program BTF. We also sometimes have the case
that BTF generated during compilation is too new for the kernel, but
libbpf's BTF sanitization helps there.
I guess what I'm getting at here is we can potentially separate these
problems and focus on making BTF parseable for now.
>
>> 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.
Makes sense.
> 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.
Sure, will do. Thanks!
Alan
