Re: [PATCH bpf-next v2 5/8] libbpf: Support opening bpf objects of either endianness

[Tony Ambardar <tony.ambardar@gmail.com>]

On Fri, Aug 23, 2024 at 12:47:47PM -0700, Andrii Nakryiko wrote:
> On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@gmail.com>
> >
> > Allow bpf_object__open() to access files of either endianness, and convert
> > included BPF programs to native byte-order in-memory for introspection.
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/lib/bpf/libbpf.c          | 21 +++++++++++++++++++--
> >  tools/lib/bpf/libbpf_internal.h | 11 +++++++++++
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> >
> 
> Instructions are not the only data that would need swapping. We have
> user's data sections and stuff like that, which, generally speaking,
> isn't that safe to just byteswap.
> 
> I do understand the appeal of being endianness-agnostic, but doesn't
> extend all the way to actually loading BPF programs. At least I
> wouldn't start there.

Yes, absolutely. I first planned to move the endianness check from "open"
to "load" functions but got waylaid tracing skeleton code into the latter
and left it to continue progress. Let me figure out the best place to put
a check without breaking things.

> 
> We need to make open phase endianness agnostic, load should just fail
> for swapped endianness case. So let's record the fact that we are not
> in native endianness, and fail early in load step.
> 
> This will still allow us to generate skeletons and stuff like that, right?
> 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 8a0a0c1e37e1..a542031f4f73 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -940,6 +940,21 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
> >         return 0;
> >  }
> >
> > +static void bpf_object_bswap_progs(struct bpf_object *obj)
> > +{
> > +       struct bpf_program *prog = obj->programs;
> > +       struct bpf_insn *insn;
> > +       int p, i;
> > +
> > +       for (p = 0; p < obj->nr_programs; p++, prog++) {
> > +               insn = prog->insns;
> > +               for (i = 0; i < prog->insns_cnt; i++, insn++)
> > +                       bpf_insn_bswap(insn);
> > +               pr_debug("prog '%s': converted %zu insns to native byteorder\n",
> 
> "byte order"?
> 

Good catch. Fixed.

> > +                        prog->name, prog->insns_cnt);
> > +       }
> > +}
> > +
> >  static const struct btf_member *
> >  find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
> >  {
> > @@ -1610,7 +1625,6 @@ static int bpf_object__check_endianness(struct bpf_object *obj)
> >  #else
> >  # error "Unrecognized __BYTE_ORDER__"
> >  #endif
> > -       pr_warn("elf: endianness mismatch in %s.\n", obj->path);
> >         return -LIBBPF_ERRNO__ENDIAN;
> >  }
> >
> > @@ -3953,6 +3967,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> >                 return -LIBBPF_ERRNO__FORMAT;
> >         }
> >
> > +       /* change BPF program insns to native endianness for introspection */
> > +       if (bpf_object__check_endianness(obj))
> 
> let's rename this to "is_native_endianness()" and return true/false.
> "check" makes sense as something that errors out, but now it's purely
> a query, so "check" naming is confusing.
> 

Right, I mistook this as exported before and left it.

> 
> BTW, so libelf will transparently byte-swap relocations and stuff like
> that to native endianness, is that right?

Correct. Sections with types like ELF_T_REL (.rel) and ELF_T_SYM (.symtab)
get translated automagically. See patch #3 for example.

> 
> > +               bpf_object_bswap_progs(obj);
> > +
> >         /* sort BPF programs by section name and in-section instruction offset
> >          * for faster search
> >          */
> > @@ -7993,7 +8011,6 @@ static struct bpf_object *bpf_object_open(const char *path, const void *obj_buf,
> >         }
> >
> >         err = bpf_object__elf_init(obj);
> > -       err = err ? : bpf_object__check_endianness(obj);
> >         err = err ? : bpf_object__elf_collect(obj);
> >         err = err ? : bpf_object__collect_externs(obj);
> >         err = err ? : bpf_object_fixup_btf(obj);
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 6b0270c83537..f53daa601c6f 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -11,6 +11,7 @@
> >
> >  #include <stdlib.h>
> >  #include <limits.h>
> > +#include <byteswap.h>
> >  #include <errno.h>
> >  #include <linux/err.h>
> >  #include <fcntl.h>
> > @@ -590,6 +591,16 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
> >         return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> >  }
> >
> > +static inline void bpf_insn_bswap(struct bpf_insn *insn)
> > +{
> > +       /* dst_reg & src_reg nibbles */
> > +       __u8 *regs = (__u8 *)insn + offsetofend(struct bpf_insn, code);
> > +
> > +       *regs = (*regs >> 4) | (*regs << 4);
> 
> hm... we have fields, just do a brain-dead swap instead of all this
> mucking with offsetofend(
> 
> __u8 tmp_reg = insn->dst_reg;
> 
> insn->dst_reg = insn->src_reg;
> insn->src_reg = tmp_reg;
> 
> ?

Main reason for this is most compilers recognize the shift/or statement
pattern and emit a rotate op as I recall. And the offsetofend() seemed
clearest at documenting "the byte after opcode" while not obscuring these
are nibble fields. So would prefer to leave it unless you have strong
objections or I'm off the mark somehow. Let me know either way? Thanks!

> 
> 
> > +       insn->off = bswap_16(insn->off);
> > +       insn->imm = bswap_32(insn->imm);
> > +}
> > +
> >  /* Unconditionally dup FD, ensuring it doesn't use [0, 2] range.
> >   * Original FD is not closed or altered in any other way.
> >   * Preserves original FD value, if it's invalid (negative).
> > --
> > 2.34.1
> >