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

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(-)

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.

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"?

> +                        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.


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

> +               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;

?


> +       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
>

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
> >

On Mon, Aug 26, 2024 at 3:53 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> 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.
>

checking early during load should work just fine, I don't expect any problems

> >
> > 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?
> >

[...]

> > >
> > > +       /* 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.

yeah, that double underscore is very misleading and I'd like to get
rid of it, but my last attempt failed, so we are stuck with that for
now

>
> >
> > 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.
>

ok, thanks for confirming

[...]

> > >
> > > +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!
>

I do strongly prefer not having to use offsetofend() and pointer
manipulations. Whatever tiny performance difference is completely
irrelevant here. Let's go with a cleaner approach, please.


> >
> >
> > > +       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
> > >

On Mon, Aug 26, 2024 at 02:28:17PM -0700, Andrii Nakryiko wrote:
> On Mon, Aug 26, 2024 at 3:53 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > 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.
> >
> 
> checking early during load should work just fine, I don't expect any problems

Right, I believe I have this working now without impacting skeleton.

> 
> > >
> > > 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?
> > >
> 
> [...]
> 
> > > >
> > > > +       /* 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.
> 
> yeah, that double underscore is very misleading and I'd like to get
> rid of it, but my last attempt failed, so we are stuck with that for
> now
> 
> >
> > >
> > > 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.
> >
> 
> ok, thanks for confirming
> 
> [...]
> 
> > > >
> > > > +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!
> >
> 
> I do strongly prefer not having to use offsetofend() and pointer
> manipulations. Whatever tiny performance difference is completely
> irrelevant here. Let's go with a cleaner approach, please.

OK, will do for next revision.

> 
> 
> > >
> > >
> > > > +       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
> > > >