On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> wrote:
>
> From: Tony Ambardar <tony.ambardar@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
>
