Re: [RFC dwarves 1/2] dwarves: auto-detect maximum kind supported by vmlinux

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Jul 26, 2023 at 8:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 26/07/2023 11:39, Jiri Olsa wrote:
> > On Thu, Jul 20, 2023 at 09:14:42PM +0100, Alan Maguire wrote:
> >> When a newer pahole is run on an older kernel, it often knows about BTF
> >> kinds that the kernel does not support.  This is a problem because the BTF
> >> generated is then embedded in the kernel image and read, and if unknown
> >> kinds are found, BTF handling fails and core BPF functionality is
> >> unavailable.
> >>
> >> The scripts/pahole-flags.sh script enumerates the various pahole options
> >> available associated with various versions of pahole, but the problem is
> >> what matters in the case of an older kernel is the set of kinds the kernel
> >> understands.  Because recent features such as BTF_KIND_ENUM64 are added
> >> by default (and only skipped if --skip_encoding_btf_* is set), BTF will
> >> be created with these newer kinds that the older kernel cannot read.
> >> This can be fixed by stable-backporting --skip options, but this is
> >> cumbersome and would have to be done every time a new BTF kind is
> >> introduced.
> >>
> >> Here instead we pre-process the DWARF information associated with the
> >> target for BTF generation; if we find an enum with a BTF_KIND_MAX
> >> value in the DWARF associated with the object, we use that to
> >> determine the maximum BTF kind supported.  Note that the enum
> >> representation of BTF kinds starts for the 5.16 kernel; prior to this
> >> The benefit of auto-detection is that no work is required for older
> >> kernels when new kinds are added, and --skip_encoding options are
> >> less needed.
> >>
> >> [1] https://github.com/oracle-samples/bpftune/issues/35
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >> ---
> >>  btf_encoder.c  | 12 ++++++++++++
> >>  dwarf_loader.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  dwarves.h      |  2 ++
> >>  3 files changed, 66 insertions(+)
> >>
> >> diff --git a/btf_encoder.c b/btf_encoder.c
> >> index 65f6e71..98c7529 100644
> >> --- a/btf_encoder.c
> >> +++ b/btf_encoder.c
> >> @@ -1889,3 +1889,15 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder)
> >>  {
> >>      return encoder->btf;
> >>  }
> >> +
> >> +void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
> >> +{
> >> +    if (btf_kind_max < 0 || btf_kind_max >= BTF_KIND_MAX)
> >> +            return;
> >> +    if (btf_kind_max < BTF_KIND_DECL_TAG)
> >> +            conf_load->skip_encoding_btf_decl_tag = true;
> >> +    if (btf_kind_max < BTF_KIND_TYPE_TAG)
> >> +            conf_load->skip_encoding_btf_type_tag = true;
> >> +    if (btf_kind_max < BTF_KIND_ENUM64)
> >> +            conf_load->skip_encoding_btf_enum64 = true;
> >> +}
> >
> > hi,
> > so there are some older kernels other than stable that would use this feature
> > right? because stable already have proper setup for pahole options
> >
> > or it's just there to be complete and we'd eventually add new rules in here?
> > wouldn't that be covered by the BTF kind layout stuff you work on? is there
> > some overlap?
> >
>
> Yeah, the idea is to minimize the complexity when adding new kinds. The
> approach explored here does this because when adding a new kind we have
> to either
>
> - make it a pahole opt-in parameter, which means more --btf_encode_*
> parameters for dwarves; or
> - make it an opt-out parameter, which means more stable backports to set
> the opt-out.
>
> My original hope was BTF kind layout encoding would solve the problem,
> but the problem is that new kinds are often entwined tightly in the
> representation of structures, functions etc. When that happens, even
> knowing the kind layout won't help an older kernel interpret what the
> BTF actually means when it was generated by a newer pahole. Kind layout
> still has value - it means BTF can always be dumped for example - but it
> can't really help the kernel to reliabily _use_ kernel/module BTF
> information. So the approach here is to try to streamline the process by
> not requiring new options when a new kind is added; we can simply detect
> if the kernel knows about the kind and skip it if not. Since this
> detection is all internal to pahole, nothing needs to be exposed to the
> user.
>
>
> >> diff --git a/dwarf_loader.c b/dwarf_loader.c
> >> index ccf3194..8984043 100644
> >> --- a/dwarf_loader.c
> >> +++ b/dwarf_loader.c
> >> @@ -3358,8 +3358,60 @@ static int __dwarf_cus__process_cus(struct dwarf_cus *dcus)
> >>      return 0;
> >>  }
> >>
> >> +/* Find enumeration value for BTF_KIND_MAX; replace conf_load->btf_kind_max with
> >> + * this value if found since it indicates that the target object does not know
> >> + * about kinds > its BTF_KIND_MAX value.  This is valuable for kernel/module
> >> + * BTF where a newer pahole/libbpf operate on an older kernel which cannot
> >> + * parse some of the newer kinds pahole can generate.
> >> + */
> >> +static void dwarf__find_btf_kind_max(struct dwarf_cus *dcus)
> >> +{
> >> +    struct conf_load *conf = dcus->conf;
> >> +    uint8_t pointer_size, offset_size;
> >> +    Dwarf_Off off = 0, noff;
> >> +    size_t cuhl;
> >> +
> >> +    while (dwarf_nextcu(dcus->dw, off, &noff, &cuhl, NULL, &pointer_size, &offset_size) == 0) {
> >> +            Dwarf_Die die_mem;
> >> +            Dwarf_Die *cu_die = dwarf_offdie(dcus->dw, off + cuhl, &die_mem);
> >> +            Dwarf_Die child;
> >> +
> >> +            if (cu_die == NULL)
> >> +                    break;
> >> +            if (dwarf_child(cu_die, &child) == 0) {
> >> +                    Dwarf_Die *die = &child;
> >> +
> >> +                    do {
> >> +                            Dwarf_Die echild, *edie;
> >> +
> >> +                            if (dwarf_tag(die) != DW_TAG_enumeration_type ||
> >> +                                !dwarf_haschildren(die) ||
> >> +                                dwarf_child(die, &echild) != 0)
> >> +                                    continue;
> >> +                            edie = &echild;
> >> +                            do {
> >> +                                    const char *ename;
> >> +                                    int btf_kind_max;
> >> +
> >> +                                    if (dwarf_tag(edie) != DW_TAG_enumerator)
> >> +                                            continue;
> >> +                                    ename = attr_string(edie, DW_AT_name, conf);
> >> +                                    if (!ename || strcmp(ename, "BTF_KIND_MAX") != 0)
> >> +                                            continue;
> >> +                                    btf_kind_max = attr_numeric(edie, DW_AT_const_value);
> >> +                                    dwarves__set_btf_kind_max(conf, btf_kind_max);
> >> +                                    return;
> >> +                            } while (dwarf_siblingof(edie, edie) == 0);
> >> +                    } while (dwarf_siblingof(die, die) == 0);
> >> +            }
> >> +            off = noff;
> >> +    }
> >> +}
> >> +
> >>  static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
> >>  {
> >> +    dwarf__find_btf_kind_max(dcus);
> >
> > first I though this should be enabled by some (detect) option.. but that
> > would probably beat the main purpose.. also I think we don't need kernel
> > with BTF that it can't process
> >
>
> Hmm, maybe it might be good to have it associated with --btf_gen_all in
> case we ever want to disable it for debugging purposes? What do you
> think? Thanks!

Overall approach makes sense to me and an extra flag like --btf_gen_all
to disable this smartness also makes sense.