diff mbox series

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

Message ID 0511638ed6191f4434be23ed7fb56001ccb5b687.1724313164.git.tony.ambardar@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series libbpf, selftests/bpf: Support cross-endian usage | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 7 this patch: 7
netdev/build_tools success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Tony Ambardar Aug. 22, 2024, 9:24 a.m. UTC 
From: Tony Ambardar <tony.ambardar@gmail.com>

Allow static linking object files of either endianness, checking that input
files have consistent byte-order, and setting output endianness from input.

Linking requires in-memory processing of programs, relocations, sections,
etc. in native endianness, and output conversion to target byte-order. This
is enabled by built-in ELF translation and recent BTF/BTF.ext endianness
functions. Further add local functions for swapping byte-order of sections
containing BPF insns.

Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/lib/bpf/linker.c | 106 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 90 insertions(+), 16 deletions(-)

Comments

Andrii Nakryiko Aug. 23, 2024, 7:47 p.m. UTC | #1 
On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@gmail.com>
>
> Allow static linking object files of either endianness, checking that input
> files have consistent byte-order, and setting output endianness from input.
>
> Linking requires in-memory processing of programs, relocations, sections,
> etc. in native endianness, and output conversion to target byte-order. This
> is enabled by built-in ELF translation and recent BTF/BTF.ext endianness
> functions. Further add local functions for swapping byte-order of sections
> containing BPF insns.
>
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  tools/lib/bpf/linker.c | 106 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 90 insertions(+), 16 deletions(-)
>

Mostly just stylistic and code organization nits, the change overall looks good.

> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 7489306cd6f7..9bf218db443e 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -135,6 +135,7 @@ struct bpf_linker {
>         int fd;
>         Elf *elf;
>         Elf64_Ehdr *elf_hdr;
> +       bool swapped_endian;
>
>         /* Output sections metadata */
>         struct dst_sec *secs;
> @@ -324,13 +325,8 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
>
>         linker->elf_hdr->e_machine = EM_BPF;
>         linker->elf_hdr->e_type = ET_REL;
> -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> -       linker->elf_hdr->e_ident[EI_DATA] = ELFDATA2LSB;
> -#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> -       linker->elf_hdr->e_ident[EI_DATA] = ELFDATA2MSB;
> -#else
> -#error "Unknown __BYTE_ORDER__"
> -#endif
> +       /* Set unknown ELF endianness, assign later from input files */
> +       linker->elf_hdr->e_ident[EI_DATA] = ELFDATANONE;
>
>         /* STRTAB */
>         /* initialize strset with an empty string to conform to ELF */
> @@ -541,19 +537,21 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                                 const struct bpf_linker_file_opts *opts,
>                                 struct src_obj *obj)
>  {
> -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> -       const int host_endianness = ELFDATA2LSB;
> -#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> -       const int host_endianness = ELFDATA2MSB;
> -#else
> -#error "Unknown __BYTE_ORDER__"
> -#endif
>         int err = 0;
>         Elf_Scn *scn;
>         Elf_Data *data;
>         Elf64_Ehdr *ehdr;
>         Elf64_Shdr *shdr;
>         struct src_sec *sec;
> +       unsigned char obj_byteorder;
> +       unsigned char *link_byteorder = &linker->elf_hdr->e_ident[EI_DATA];

nit: not a fan of pointer into e_ident, just read local value of byte
order, and then assign it directly below (it's only in one of the
branches, no duplication, really)

> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +       const unsigned char host_byteorder = ELFDATA2LSB;
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +       const unsigned char host_byteorder = ELFDATA2MSB;
> +#else
> +#error "Unknown __BYTE_ORDER__"
> +#endif
>
>         pr_debug("linker: adding object file '%s'...\n", filename);
>
> @@ -579,11 +577,25 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>                 pr_warn_elf("failed to get ELF header for %s", filename);
>                 return err;
>         }
> -       if (ehdr->e_ident[EI_DATA] != host_endianness) {
> +
> +       /* Linker output endianness set by first input object */
> +       obj_byteorder = ehdr->e_ident[EI_DATA];
> +       if (obj_byteorder != ELFDATA2LSB && obj_byteorder != ELFDATA2MSB) {
>                 err = -EOPNOTSUPP;
> -               pr_warn_elf("unsupported byte order of ELF file %s", filename);
> +               pr_warn("linker: unknown byte order of ELF file %s\n", filename);
>                 return err;
>         }
> +       if (*link_byteorder == ELFDATANONE) {
> +               *link_byteorder = obj_byteorder;

see above, I'd prefer:

linker->elf_hdr->e_ident[EI_DATA] = obj_byteorder;

> +               linker->swapped_endian = obj_byteorder != host_byteorder;
> +               pr_debug("linker: set %s-endian output byte order\n",
> +                        obj_byteorder == ELFDATA2MSB ? "big" : "little");
> +       } else if (*link_byteorder != obj_byteorder) {
> +               err = -EOPNOTSUPP;
> +               pr_warn("linker: byte order mismatch with ELF file %s\n", filename);
> +               return err;
> +       }
> +
>         if (ehdr->e_type != ET_REL
>             || ehdr->e_machine != EM_BPF
>             || ehdr->e_ident[EI_CLASS] != ELFCLASS64) {
> @@ -1111,6 +1123,27 @@ static bool sec_content_is_same(struct dst_sec *dst_sec, struct src_sec *src_sec
>         return true;
>  }
>
> +static bool is_exec_sec(struct dst_sec *sec)
> +{
> +       if (!sec || sec->ephemeral)
> +               return false;
> +       return (sec->shdr->sh_type == SHT_PROGBITS) &&
> +              (sec->shdr->sh_flags & SHF_EXECINSTR);
> +}
> +
> +static int exec_sec_bswap(void *raw_data, int size)
> +{
> +       const int insn_cnt = size / sizeof(struct bpf_insn);
> +       struct bpf_insn *insn = raw_data;
> +       int i;
> +
> +       if (size % sizeof(struct bpf_insn))
> +               return -EINVAL;
> +       for (i = 0; i < insn_cnt; i++, insn++)
> +               bpf_insn_bswap(insn);
> +       return 0;
> +}
> +
>  static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src_sec *src)
>  {
>         void *tmp;
> @@ -1170,6 +1203,16 @@ static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
>                 memset(dst->raw_data + dst->sec_sz, 0, dst_align_sz - dst->sec_sz);
>                 /* now copy src data at a properly aligned offset */
>                 memcpy(dst->raw_data + dst_align_sz, src->data->d_buf, src->shdr->sh_size);
> +
> +               /* convert added bpf insns to native byte-order */
> +               if (linker->swapped_endian && is_exec_sec(dst)) {
> +                       err = exec_sec_bswap(dst->raw_data + dst_align_sz,
> +                                            src->shdr->sh_size);

nit: I think exec_sec_bswap() shouldn't ever fail, so given we have
is_exec_sec() now, let's do the size alignment check early on (and
regardless of swapped_endian), and then just proceed with byte swap
that can't fail

> +                       if (err) {
> +                               pr_warn("%s: error changing insns endianness\n", __func__);
> +                               return err;
> +                       }
> +               }
>         }
>
>         dst->sec_sz = dst_final_sz;
> @@ -2630,6 +2673,14 @@ int bpf_linker__finalize(struct bpf_linker *linker)
>                 if (!sec->scn)
>                         continue;
>
> +               /* restore sections with bpf insns to target byte-order */
> +               if (linker->swapped_endian && is_exec_sec(sec)) {
> +                       err = exec_sec_bswap(sec->raw_data, sec->sec_sz);

and here we'll know that size is validly aligned anyways, so no checks required

> +                       if (err) {
> +                               pr_warn("error finalizing insns endianness\n");
> +                               return libbpf_err(err);
> +                       }
> +               }
>                 sec->data->d_buf = sec->raw_data;
>         }
>
> @@ -2696,6 +2747,13 @@ static int emit_elf_data_sec(struct bpf_linker *linker, const char *sec_name,
>         return 0;
>  }
>
> +static enum btf_endianness
> +linker_btf_endianness(const struct bpf_linker *linker)
> +{
> +       unsigned char byteorder = linker->elf_hdr->e_ident[EI_DATA];

empty line between variable declaration and the rest of the code

> +       return byteorder == ELFDATA2MSB ? BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
> +}
> +

but actually, this whole helper function seems unnecessary, just do
everything inside finalize_btf, it's a pretty trivial piece of logic

>  static int finalize_btf(struct bpf_linker *linker)
>  {
>         LIBBPF_OPTS(btf_dedup_opts, opts);
> @@ -2742,6 +2800,22 @@ static int finalize_btf(struct bpf_linker *linker)
>                 return err;
>         }
>
> +       /* Set .BTF and .BTF.ext output byte order */
> +       err = btf__set_endianness(linker->btf,
> +                                 linker_btf_endianness(linker));
> +       if (err) {
> +               pr_warn("failed to set .BTF output endianness: %d\n", err);

nit: you used "linker: " prefix for messages like this, stay consistent?


> +               return err;
> +       }
> +       if (linker->btf_ext) {
> +               err = btf_ext__set_endianness(linker->btf_ext,
> +                                             linker_btf_endianness(linker));
> +               if (err) {
> +                       pr_warn("failed to set .BTF.ext output endianness: %d\n", err);
> +                       return err;
> +               }
> +       }
> +
>         /* Emit .BTF section */
>         raw_data = btf__raw_data(linker->btf, &raw_sz);
>         if (!raw_data)
> --
> 2.34.1
>
Tony Ambardar Aug. 26, 2024, 10:56 a.m. UTC | #2 
On Fri, Aug 23, 2024 at 12:47:52PM -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 static linking object files of either endianness, checking that input
> > files have consistent byte-order, and setting output endianness from input.
> >
> > Linking requires in-memory processing of programs, relocations, sections,
> > etc. in native endianness, and output conversion to target byte-order. This
> > is enabled by built-in ELF translation and recent BTF/BTF.ext endianness
> > functions. Further add local functions for swapping byte-order of sections
> > containing BPF insns.
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/lib/bpf/linker.c | 106 ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 90 insertions(+), 16 deletions(-)
> >
> 
> Mostly just stylistic and code organization nits, the change overall looks good.
> 
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index 7489306cd6f7..9bf218db443e 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -135,6 +135,7 @@ struct bpf_linker {
> >         int fd;
> >         Elf *elf;
> >         Elf64_Ehdr *elf_hdr;
> > +       bool swapped_endian;
> >
> >         /* Output sections metadata */
> >         struct dst_sec *secs;
> > @@ -324,13 +325,8 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
> >
> >         linker->elf_hdr->e_machine = EM_BPF;
> >         linker->elf_hdr->e_type = ET_REL;
> > -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > -       linker->elf_hdr->e_ident[EI_DATA] = ELFDATA2LSB;
> > -#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > -       linker->elf_hdr->e_ident[EI_DATA] = ELFDATA2MSB;
> > -#else
> > -#error "Unknown __BYTE_ORDER__"
> > -#endif
> > +       /* Set unknown ELF endianness, assign later from input files */
> > +       linker->elf_hdr->e_ident[EI_DATA] = ELFDATANONE;
> >
> >         /* STRTAB */
> >         /* initialize strset with an empty string to conform to ELF */
> > @@ -541,19 +537,21 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> >                                 const struct bpf_linker_file_opts *opts,
> >                                 struct src_obj *obj)
> >  {
> > -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > -       const int host_endianness = ELFDATA2LSB;
> > -#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > -       const int host_endianness = ELFDATA2MSB;
> > -#else
> > -#error "Unknown __BYTE_ORDER__"
> > -#endif
> >         int err = 0;
> >         Elf_Scn *scn;
> >         Elf_Data *data;
> >         Elf64_Ehdr *ehdr;
> >         Elf64_Shdr *shdr;
> >         struct src_sec *sec;
> > +       unsigned char obj_byteorder;
> > +       unsigned char *link_byteorder = &linker->elf_hdr->e_ident[EI_DATA];
> 
> nit: not a fan of pointer into e_ident, just read local value of byte
> order, and then assign it directly below (it's only in one of the
> branches, no duplication, really)

Yes, it's awkward. Fixed.

> 
> > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > +       const unsigned char host_byteorder = ELFDATA2LSB;
> > +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > +       const unsigned char host_byteorder = ELFDATA2MSB;
> > +#else
> > +#error "Unknown __BYTE_ORDER__"
> > +#endif
> >
> >         pr_debug("linker: adding object file '%s'...\n", filename);
> >
> > @@ -579,11 +577,25 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> >                 pr_warn_elf("failed to get ELF header for %s", filename);
> >                 return err;
> >         }
> > -       if (ehdr->e_ident[EI_DATA] != host_endianness) {
> > +
> > +       /* Linker output endianness set by first input object */
> > +       obj_byteorder = ehdr->e_ident[EI_DATA];
> > +       if (obj_byteorder != ELFDATA2LSB && obj_byteorder != ELFDATA2MSB) {
> >                 err = -EOPNOTSUPP;
> > -               pr_warn_elf("unsupported byte order of ELF file %s", filename);
> > +               pr_warn("linker: unknown byte order of ELF file %s\n", filename);
> >                 return err;
> >         }
> > +       if (*link_byteorder == ELFDATANONE) {
> > +               *link_byteorder = obj_byteorder;
> 
> see above, I'd prefer:
> 
> linker->elf_hdr->e_ident[EI_DATA] = obj_byteorder;
> 

Done.

> > +               linker->swapped_endian = obj_byteorder != host_byteorder;
> > +               pr_debug("linker: set %s-endian output byte order\n",
> > +                        obj_byteorder == ELFDATA2MSB ? "big" : "little");
> > +       } else if (*link_byteorder != obj_byteorder) {
> > +               err = -EOPNOTSUPP;
> > +               pr_warn("linker: byte order mismatch with ELF file %s\n", filename);
> > +               return err;
> > +       }
> > +
> >         if (ehdr->e_type != ET_REL
> >             || ehdr->e_machine != EM_BPF
> >             || ehdr->e_ident[EI_CLASS] != ELFCLASS64) {
> > @@ -1111,6 +1123,27 @@ static bool sec_content_is_same(struct dst_sec *dst_sec, struct src_sec *src_sec
> >         return true;
> >  }
> >
> > +static bool is_exec_sec(struct dst_sec *sec)
> > +{
> > +       if (!sec || sec->ephemeral)
> > +               return false;
> > +       return (sec->shdr->sh_type == SHT_PROGBITS) &&
> > +              (sec->shdr->sh_flags & SHF_EXECINSTR);
> > +}
> > +
> > +static int exec_sec_bswap(void *raw_data, int size)
> > +{
> > +       const int insn_cnt = size / sizeof(struct bpf_insn);
> > +       struct bpf_insn *insn = raw_data;
> > +       int i;
> > +
> > +       if (size % sizeof(struct bpf_insn))
> > +               return -EINVAL;
> > +       for (i = 0; i < insn_cnt; i++, insn++)
> > +               bpf_insn_bswap(insn);
> > +       return 0;
> > +}
> > +
> >  static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src_sec *src)
> >  {
> >         void *tmp;
> > @@ -1170,6 +1203,16 @@ static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
> >                 memset(dst->raw_data + dst->sec_sz, 0, dst_align_sz - dst->sec_sz);
> >                 /* now copy src data at a properly aligned offset */
> >                 memcpy(dst->raw_data + dst_align_sz, src->data->d_buf, src->shdr->sh_size);
> > +
> > +               /* convert added bpf insns to native byte-order */
> > +               if (linker->swapped_endian && is_exec_sec(dst)) {
> > +                       err = exec_sec_bswap(dst->raw_data + dst_align_sz,
> > +                                            src->shdr->sh_size);
> 
> nit: I think exec_sec_bswap() shouldn't ever fail, so given we have
> is_exec_sec() now, let's do the size alignment check early on (and
> regardless of swapped_endian), and then just proceed with byte swap
> that can't fail

Looking more closely, I see we already have this size check from:
32fa058398 ("libbpf: Add pr_warn() for EINVAL cases in linker_sanity_check_elf")

So can just drop the error-handling.
> 
> > +                       if (err) {
> > +                               pr_warn("%s: error changing insns endianness\n", __func__);
> > +                               return err;
> > +                       }
> > +               }
> >         }
> >
> >         dst->sec_sz = dst_final_sz;
> > @@ -2630,6 +2673,14 @@ int bpf_linker__finalize(struct bpf_linker *linker)
> >                 if (!sec->scn)
> >                         continue;
> >
> > +               /* restore sections with bpf insns to target byte-order */
> > +               if (linker->swapped_endian && is_exec_sec(sec)) {
> > +                       err = exec_sec_bswap(sec->raw_data, sec->sec_sz);
> 
> and here we'll know that size is validly aligned anyways, so no checks required
> 
> > +                       if (err) {
> > +                               pr_warn("error finalizing insns endianness\n");
> > +                               return libbpf_err(err);
> > +                       }
> > +               }
> >                 sec->data->d_buf = sec->raw_data;
> >         }
> >
> > @@ -2696,6 +2747,13 @@ static int emit_elf_data_sec(struct bpf_linker *linker, const char *sec_name,
> >         return 0;
> >  }
> >
> > +static enum btf_endianness
> > +linker_btf_endianness(const struct bpf_linker *linker)
> > +{
> > +       unsigned char byteorder = linker->elf_hdr->e_ident[EI_DATA];
> 
> empty line between variable declaration and the rest of the code
> 
> > +       return byteorder == ELFDATA2MSB ? BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
> > +}
> > +
> 
> but actually, this whole helper function seems unnecessary, just do
> everything inside finalize_btf, it's a pretty trivial piece of logic

OK, merged as it's not complicated.

> 
> >  static int finalize_btf(struct bpf_linker *linker)
> >  {
> >         LIBBPF_OPTS(btf_dedup_opts, opts);
> > @@ -2742,6 +2800,22 @@ static int finalize_btf(struct bpf_linker *linker)
> >                 return err;
> >         }
> >
> > +       /* Set .BTF and .BTF.ext output byte order */
> > +       err = btf__set_endianness(linker->btf,
> > +                                 linker_btf_endianness(linker));
> > +       if (err) {
> > +               pr_warn("failed to set .BTF output endianness: %d\n", err);
> 
> nit: you used "linker: " prefix for messages like this, stay consistent?

Right, original code used extra "linker:" detail only for "debug" messages
and not "warn" level. Changed to do the same. Thanks!

> 
> 
> > +               return err;
> > +       }
> > +       if (linker->btf_ext) {
> > +               err = btf_ext__set_endianness(linker->btf_ext,
> > +                                             linker_btf_endianness(linker));
> > +               if (err) {
> > +                       pr_warn("failed to set .BTF.ext output endianness: %d\n", err);
> > +                       return err;
> > +               }
> > +       }
> > +
> >         /* Emit .BTF section */
> >         raw_data = btf__raw_data(linker->btf, &raw_sz);
> >         if (!raw_data)
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 7489306cd6f7..9bf218db443e 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -135,6 +135,7 @@  struct bpf_linker {
 	int fd;
 	Elf *elf;
 	Elf64_Ehdr *elf_hdr;
+	bool swapped_endian;
 
 	/* Output sections metadata */
 	struct dst_sec *secs;
@@ -324,13 +325,8 @@  static int init_output_elf(struct bpf_linker *linker, const char *file)
 
 	linker->elf_hdr->e_machine = EM_BPF;
 	linker->elf_hdr->e_type = ET_REL;
-#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-	linker->elf_hdr->e_ident[EI_DATA] = ELFDATA2LSB;
-#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-	linker->elf_hdr->e_ident[EI_DATA] = ELFDATA2MSB;
-#else
-#error "Unknown __BYTE_ORDER__"
-#endif
+	/* Set unknown ELF endianness, assign later from input files */
+	linker->elf_hdr->e_ident[EI_DATA] = ELFDATANONE;
 
 	/* STRTAB */
 	/* initialize strset with an empty string to conform to ELF */
@@ -541,19 +537,21 @@  static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 				const struct bpf_linker_file_opts *opts,
 				struct src_obj *obj)
 {
-#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-	const int host_endianness = ELFDATA2LSB;
-#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-	const int host_endianness = ELFDATA2MSB;
-#else
-#error "Unknown __BYTE_ORDER__"
-#endif
 	int err = 0;
 	Elf_Scn *scn;
 	Elf_Data *data;
 	Elf64_Ehdr *ehdr;
 	Elf64_Shdr *shdr;
 	struct src_sec *sec;
+	unsigned char obj_byteorder;
+	unsigned char *link_byteorder = &linker->elf_hdr->e_ident[EI_DATA];
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	const unsigned char host_byteorder = ELFDATA2LSB;
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	const unsigned char host_byteorder = ELFDATA2MSB;
+#else
+#error "Unknown __BYTE_ORDER__"
+#endif
 
 	pr_debug("linker: adding object file '%s'...\n", filename);
 
@@ -579,11 +577,25 @@  static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 		pr_warn_elf("failed to get ELF header for %s", filename);
 		return err;
 	}
-	if (ehdr->e_ident[EI_DATA] != host_endianness) {
+
+	/* Linker output endianness set by first input object */
+	obj_byteorder = ehdr->e_ident[EI_DATA];
+	if (obj_byteorder != ELFDATA2LSB && obj_byteorder != ELFDATA2MSB) {
 		err = -EOPNOTSUPP;
-		pr_warn_elf("unsupported byte order of ELF file %s", filename);
+		pr_warn("linker: unknown byte order of ELF file %s\n", filename);
 		return err;
 	}
+	if (*link_byteorder == ELFDATANONE) {
+		*link_byteorder = obj_byteorder;
+		linker->swapped_endian = obj_byteorder != host_byteorder;
+		pr_debug("linker: set %s-endian output byte order\n",
+			 obj_byteorder == ELFDATA2MSB ? "big" : "little");
+	} else if (*link_byteorder != obj_byteorder) {
+		err = -EOPNOTSUPP;
+		pr_warn("linker: byte order mismatch with ELF file %s\n", filename);
+		return err;
+	}
+
 	if (ehdr->e_type != ET_REL
 	    || ehdr->e_machine != EM_BPF
 	    || ehdr->e_ident[EI_CLASS] != ELFCLASS64) {
@@ -1111,6 +1123,27 @@  static bool sec_content_is_same(struct dst_sec *dst_sec, struct src_sec *src_sec
 	return true;
 }
 
+static bool is_exec_sec(struct dst_sec *sec)
+{
+	if (!sec || sec->ephemeral)
+		return false;
+	return (sec->shdr->sh_type == SHT_PROGBITS) &&
+	       (sec->shdr->sh_flags & SHF_EXECINSTR);
+}
+
+static int exec_sec_bswap(void *raw_data, int size)
+{
+	const int insn_cnt = size / sizeof(struct bpf_insn);
+	struct bpf_insn *insn = raw_data;
+	int i;
+
+	if (size % sizeof(struct bpf_insn))
+		return -EINVAL;
+	for (i = 0; i < insn_cnt; i++, insn++)
+		bpf_insn_bswap(insn);
+	return 0;
+}
+
 static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src_sec *src)
 {
 	void *tmp;
@@ -1170,6 +1203,16 @@  static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
 		memset(dst->raw_data + dst->sec_sz, 0, dst_align_sz - dst->sec_sz);
 		/* now copy src data at a properly aligned offset */
 		memcpy(dst->raw_data + dst_align_sz, src->data->d_buf, src->shdr->sh_size);
+
+		/* convert added bpf insns to native byte-order */
+		if (linker->swapped_endian && is_exec_sec(dst)) {
+			err = exec_sec_bswap(dst->raw_data + dst_align_sz,
+					     src->shdr->sh_size);
+			if (err) {
+				pr_warn("%s: error changing insns endianness\n", __func__);
+				return err;
+			}
+		}
 	}
 
 	dst->sec_sz = dst_final_sz;
@@ -2630,6 +2673,14 @@  int bpf_linker__finalize(struct bpf_linker *linker)
 		if (!sec->scn)
 			continue;
 
+		/* restore sections with bpf insns to target byte-order */
+		if (linker->swapped_endian && is_exec_sec(sec)) {
+			err = exec_sec_bswap(sec->raw_data, sec->sec_sz);
+			if (err) {
+				pr_warn("error finalizing insns endianness\n");
+				return libbpf_err(err);
+			}
+		}
 		sec->data->d_buf = sec->raw_data;
 	}
 
@@ -2696,6 +2747,13 @@  static int emit_elf_data_sec(struct bpf_linker *linker, const char *sec_name,
 	return 0;
 }
 
+static enum btf_endianness
+linker_btf_endianness(const struct bpf_linker *linker)
+{
+	unsigned char byteorder = linker->elf_hdr->e_ident[EI_DATA];
+	return byteorder == ELFDATA2MSB ? BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
+}
+
 static int finalize_btf(struct bpf_linker *linker)
 {
 	LIBBPF_OPTS(btf_dedup_opts, opts);
@@ -2742,6 +2800,22 @@  static int finalize_btf(struct bpf_linker *linker)
 		return err;
 	}
 
+	/* Set .BTF and .BTF.ext output byte order */
+	err = btf__set_endianness(linker->btf,
+				  linker_btf_endianness(linker));
+	if (err) {
+		pr_warn("failed to set .BTF output endianness: %d\n", err);
+		return err;
+	}
+	if (linker->btf_ext) {
+		err = btf_ext__set_endianness(linker->btf_ext,
+					      linker_btf_endianness(linker));
+		if (err) {
+			pr_warn("failed to set .BTF.ext output endianness: %d\n", err);
+			return err;
+		}
+	}
+
 	/* Emit .BTF section */
 	raw_data = btf__raw_data(linker->btf, &raw_sz);
 	if (!raw_data)