diff mbox series

[bpf-next,v2,4/8] libbpf: Support BTF.ext loading and output in either endianness

Message ID bf9275e2af357f287bc4d37dd9b43a1f8be33ef7.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 82 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 32 this patch: 32
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-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
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-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-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on 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-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier 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>

Support for handling BTF data of either endianness was added in [1], but
did not include BTF.ext data for lack of use cases. Later, support for
static linking [2] provided a use case, but this feature and later ones
were restricted to native-endian usage.

Add support for BTF.ext handling in either endianness. Convert BTF.ext data
to native endianness when read into memory for further processing, and
support raw data access that restores the original byte-order for output.

Add new API functions btf_ext__endianness() and btf_ext__set_endianness()
for query and setting byte-order, as already exist for BTF data.

[1]:commit 3289959b97ca ("libbpf: Support BTF loading and raw data output in both endianness")
[2]:commit 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")

Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/lib/bpf/btf.c             | 163 ++++++++++++++++++++++++++++++--
 tools/lib/bpf/btf.h             |   3 +
 tools/lib/bpf/libbpf.map        |   2 +
 tools/lib/bpf/libbpf_internal.h |   2 +
 4 files changed, 161 insertions(+), 9 deletions(-)

Comments

Andrii Nakryiko Aug. 22, 2024, 11:36 p.m. UTC | #1
I ran out of time looking through this, I'll try to get back to this
patch set later today or tomorrow. So please don't repost, but see my
comments below.

On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@gmail.com>
>
> Support for handling BTF data of either endianness was added in [1], but
> did not include BTF.ext data for lack of use cases. Later, support for
> static linking [2] provided a use case, but this feature and later ones
> were restricted to native-endian usage.
>
> Add support for BTF.ext handling in either endianness. Convert BTF.ext data
> to native endianness when read into memory for further processing, and
> support raw data access that restores the original byte-order for output.
>
> Add new API functions btf_ext__endianness() and btf_ext__set_endianness()
> for query and setting byte-order, as already exist for BTF data.
>
> [1]:commit 3289959b97ca ("libbpf: Support BTF loading and raw data output in both endianness")
> [2]:commit 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")

Is this some standard syntax to refer to commit? If not, maybe drop
":commit" parts?

>
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  tools/lib/bpf/btf.c             | 163 ++++++++++++++++++++++++++++++--
>  tools/lib/bpf/btf.h             |   3 +
>  tools/lib/bpf/libbpf.map        |   2 +
>  tools/lib/bpf/libbpf_internal.h |   2 +
>  4 files changed, 161 insertions(+), 9 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index cf4f7bd7ff5c..cb4ee4ed4b5c 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -2884,6 +2884,52 @@ int btf__add_decl_tag(struct btf *btf, const char *value, int ref_type_id,
>         return btf_commit_type(btf, sz);
>  }
>
> +/*
> + * Swap endianness of the info segment in a BTF.ext data section:
> + *   - requires BTF.ext header data in native byte order
> + *   - expects info record fields are 32-bit
> + */
> +static int btf_ext_bswap_info(void *data, const __u32 data_size)

pass btf_ext_header (in native endianness) as input argument

> +{
> +       const struct btf_ext_header *hdr = data;
> +       __u32 info_size, sum_len, i, *p;
> +
> +       if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
> +               pr_warn("BTF.ext initial header not found\n");
> +               return -EINVAL;
> +       }
> +
> +       if (data_size < hdr->hdr_len) {
> +               pr_warn("BTF.ext header not found\n");
> +               return -EINVAL;
> +       }

and we already checked this, no?

> +
> +       if (hdr->hdr_len < offsetofend(struct btf_ext_header, line_info_len)) {
> +               pr_warn("BTF.ext header missing func_info, line_info\n");
> +               return -EINVAL;
> +       }

we have this check in btf_ext__new() already, do the swap after we
validated this and keep this function simpler and more focused. You
are mixing sanity checks with byte swapping, it's confusing.

> +
> +       sum_len = hdr->func_info_len + hdr->line_info_len;
> +       if (hdr->hdr_len >= offsetofend(struct btf_ext_header, core_relo_len))
> +               sum_len += hdr->core_relo_len;
> +
> +       info_size = data_size - hdr->hdr_len;
> +       if (info_size != sum_len) {
> +               pr_warn("BTF.ext info size mismatch with header data\n");
> +               return -EINVAL;
> +       }
> +
> +       if (info_size && info_size % sizeof(__u32)) {
> +               pr_warn("BTF.ext info size not 32-bit multiple\n");
> +               return -EINVAL;
> +       }

same as above, all this has nothing to do with endianness, it's all
sanity checks. Let's extract them into a separate validation function
that can happen before byte swap.

> +
> +       p = data + hdr->hdr_len;
> +       for (i = 0; i < info_size / sizeof(__u32); i++, p++)
> +               *p = bswap_32(*p);

this is very suspicious, it might work right now because all fields
are u32, but I'm uncomfortable making these decision.

I think for byte swapping case we need to make sure that BTF version
is exactly known to us (i.e., all record length is knowable for us),
and then do more verbose, but correct byte swap based on
bpf_func_info/bpf_line_info/etc records. Just treating all this as u32
is not great.

> +       return 0;
> +}
> +
>  struct btf_ext_sec_setup_param {
>         __u32 off;
>         __u32 len;
> @@ -3023,24 +3069,56 @@ static int btf_ext_setup_core_relos(struct btf_ext *btf_ext)
>         return btf_ext_setup_info(btf_ext, &param);
>  }
>
> -static int btf_ext_parse_hdr(__u8 *data, __u32 data_size)
> +/* Swap byte-order of BTF.ext header */
> +static void btf_ext_bswap_hdr(struct btf_ext_header *h)
>  {
> -       const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
> +       __u32 hdr_len = h->hdr_len; /* need native byte-order */
>
> -       if (data_size < offsetofend(struct btf_ext_header, hdr_len) ||
> -           data_size < hdr->hdr_len) {
> -               pr_debug("BTF.ext header not found\n");
> +       if (h->magic == bswap_16(BTF_MAGIC))
> +               hdr_len = bswap_32(hdr_len);

why so complicated? just pass hdr_len as input argument into
btf_ext_bswap_hdr(), and then uniformly swap everything, no
double-checking of decisions we already made

> +
> +       h->magic = bswap_16(h->magic);
> +       h->hdr_len = bswap_32(h->hdr_len);
> +       h->func_info_off = bswap_32(h->func_info_off);
> +       h->func_info_len = bswap_32(h->func_info_len);
> +       h->line_info_off = bswap_32(h->line_info_off);
> +       h->line_info_len = bswap_32(h->line_info_len);
> +
> +       if (hdr_len < offsetofend(struct btf_ext_header, core_relo_len))
> +               return;
> +
> +       h->core_relo_off = bswap_32(h->core_relo_off);
> +       h->core_relo_len = bswap_32(h->core_relo_len);
> +}
> +
> +static int btf_ext_parse_hdr(struct btf_ext *btf_ext)
> +{
> +       struct btf_ext_header *hdr = btf_ext->hdr;
> +       __u32 hdr_len, data_size = btf_ext->data_size;
> +
> +       if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
> +               pr_debug("BTF.ext initial header not found\n");

"BTF.ext header too short" ?

>                 return -EINVAL;
>         }
>
> +       hdr_len = hdr->hdr_len;
>         if (hdr->magic == bswap_16(BTF_MAGIC)) {
> -               pr_warn("BTF.ext in non-native endianness is not supported\n");
> -               return -ENOTSUP;
> +               btf_ext->swapped_endian = true;
> +               hdr_len = bswap_32(hdr_len);
>         } else if (hdr->magic != BTF_MAGIC) {
>                 pr_debug("Invalid BTF.ext magic:%x\n", hdr->magic);
>                 return -EINVAL;
>         }
>
> +       if (data_size < hdr_len) {
> +               pr_debug("BTF.ext header not found\n");
> +               return -EINVAL;
> +       }
> +
> +       /* Maintain native byte-order in memory for introspection */
> +       if (btf_ext->swapped_endian)
> +               btf_ext_bswap_hdr(hdr);
> +
>         if (hdr->version != BTF_VERSION) {
>                 pr_debug("Unsupported BTF.ext version:%u\n", hdr->version);
>                 return -ENOTSUP;
> @@ -3067,6 +3145,7 @@ void btf_ext__free(struct btf_ext *btf_ext)
>         free(btf_ext->line_info.sec_idxs);
>         free(btf_ext->core_relo_info.sec_idxs);
>         free(btf_ext->data);
> +       free(btf_ext->data_swapped);
>         free(btf_ext);
>  }
>
> @@ -3087,7 +3166,12 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
>         }
>         memcpy(btf_ext->data, data, size);
>
> -       err = btf_ext_parse_hdr(btf_ext->data, size);
> +       err = btf_ext_parse_hdr(btf_ext);
> +       if (err)
> +               goto done;
> +
> +       if (btf_ext->swapped_endian)
> +               err = btf_ext_bswap_info(btf_ext->data, btf_ext->data_size);
>         if (err)
>                 goto done;

nit:

if (btf_ext->swapped_endian) {
    err = ...;
    if (err)
        goto done;
}

i.e., keep error handling close and at the same nesting level

>
> @@ -3120,15 +3204,76 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
>         return btf_ext;
>  }
>
> +static void *btf_ext_raw_data(const struct btf_ext *btf_ext_ro, __u32 *size,
> +                             bool swap_endian)
> +{
> +       struct btf_ext *btf_ext = (struct btf_ext *)btf_ext_ro;
> +       const __u32 data_sz = btf_ext->data_size;
> +       void *data;
> +       int err;
> +
> +       data = swap_endian ? btf_ext->data_swapped : btf_ext->data;
> +       if (data) {
> +               *size = data_sz;
> +               return data;
> +       }
> +
> +       data = calloc(1, data_sz);
> +       if (!data)
> +               return NULL;
> +       memcpy(data, btf_ext->data, data_sz);
> +
> +       if (swap_endian) {
> +               err = btf_ext_bswap_info(data, data_sz);
> +               if (err) {
> +                       free(data);
> +                       return NULL;
> +               }
> +               btf_ext_bswap_hdr(data);
> +               btf_ext->data_swapped = data;
> +       }
> +
> +       *size = data_sz;
> +       return data;
> +}
> +
>  const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size)
>  {
> +       __u32 data_sz;
> +       void *data;
> +
> +       data = btf_ext_raw_data(btf_ext, &data_sz, btf_ext->swapped_endian);
> +       if (!data)
> +               return errno = ENOMEM, NULL;
> +
>         *size = btf_ext->data_size;
> -       return btf_ext->data;
> +       return data;
>  }
>
>  __attribute__((alias("btf_ext__raw_data")))
>  const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext, __u32 *size);
>
> +enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext)
> +{
> +       return (is_host_big_endian() != btf_ext->swapped_endian) ?
> +               BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
> +}
> +
> +int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
> +{
> +       bool is_tgt_big_endian = (endian == BTF_BIG_ENDIAN);
> +
> +       if (endian != BTF_LITTLE_ENDIAN && endian != BTF_BIG_ENDIAN)
> +               return libbpf_err(-EINVAL);
> +
> +       btf_ext->swapped_endian = (is_host_big_endian() != is_tgt_big_endian);
> +
> +       if (!btf_ext->swapped_endian) {
> +               free(btf_ext->data_swapped);
> +               btf_ext->data_swapped = NULL;
> +       }
> +       return 0;
> +}
>
>  struct btf_dedup;
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index b68d216837a9..e3cf91687c78 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -167,6 +167,9 @@ LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
>  LIBBPF_API struct btf_ext *btf_ext__new(const __u8 *data, __u32 size);
>  LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
>  LIBBPF_API const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size);
> +LIBBPF_API enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext);
> +LIBBPF_API int btf_ext__set_endianness(struct btf_ext *btf_ext,
> +                                      enum btf_endianness endian);
>
>  LIBBPF_API int btf__find_str(struct btf *btf, const char *s);
>  LIBBPF_API int btf__add_str(struct btf *btf, const char *s);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 8f0d9ea3b1b4..5c17632807b6 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -421,6 +421,8 @@ LIBBPF_1.5.0 {
>         global:
>                 btf__distill_base;
>                 btf__relocate;
> +               btf_ext__endianness;
> +               btf_ext__set_endianness;
>                 bpf_map__autoattach;
>                 bpf_map__set_autoattach;
>                 bpf_program__attach_sockmap;
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 8cda511a1982..6b0270c83537 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -484,6 +484,8 @@ struct btf_ext {
>                 struct btf_ext_header *hdr;
>                 void *data;
>         };
> +       void *data_swapped;
> +       bool swapped_endian;
>         struct btf_ext_info func_info;
>         struct btf_ext_info line_info;
>         struct btf_ext_info core_relo_info;
> --
> 2.34.1
>
Andrii Nakryiko Aug. 23, 2024, 7:47 p.m. UTC | #2
On Thu, Aug 22, 2024 at 4:36 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> I ran out of time looking through this, I'll try to get back to this
> patch set later today or tomorrow. So please don't repost, but see my
> comments below.
>
> On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@gmail.com>
> >
> > Support for handling BTF data of either endianness was added in [1], but
> > did not include BTF.ext data for lack of use cases. Later, support for
> > static linking [2] provided a use case, but this feature and later ones
> > were restricted to native-endian usage.
> >
> > Add support for BTF.ext handling in either endianness. Convert BTF.ext data
> > to native endianness when read into memory for further processing, and
> > support raw data access that restores the original byte-order for output.
> >
> > Add new API functions btf_ext__endianness() and btf_ext__set_endianness()
> > for query and setting byte-order, as already exist for BTF data.
> >
> > [1]:commit 3289959b97ca ("libbpf: Support BTF loading and raw data output in both endianness")
> > [2]:commit 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
>
> Is this some standard syntax to refer to commit? If not, maybe drop
> ":commit" parts?
>
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/lib/bpf/btf.c             | 163 ++++++++++++++++++++++++++++++--
> >  tools/lib/bpf/btf.h             |   3 +
> >  tools/lib/bpf/libbpf.map        |   2 +
> >  tools/lib/bpf/libbpf_internal.h |   2 +
> >  4 files changed, 161 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index cf4f7bd7ff5c..cb4ee4ed4b5c 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -2884,6 +2884,52 @@ int btf__add_decl_tag(struct btf *btf, const char *value, int ref_type_id,
> >         return btf_commit_type(btf, sz);
> >  }
> >
> > +/*
> > + * Swap endianness of the info segment in a BTF.ext data section:
> > + *   - requires BTF.ext header data in native byte order
> > + *   - expects info record fields are 32-bit
> > + */
> > +static int btf_ext_bswap_info(void *data, const __u32 data_size)
>
> pass btf_ext_header (in native endianness) as input argument
>
> > +{
> > +       const struct btf_ext_header *hdr = data;
> > +       __u32 info_size, sum_len, i, *p;
> > +
> > +       if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
> > +               pr_warn("BTF.ext initial header not found\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (data_size < hdr->hdr_len) {
> > +               pr_warn("BTF.ext header not found\n");
> > +               return -EINVAL;
> > +       }
>
> and we already checked this, no?
>
> > +
> > +       if (hdr->hdr_len < offsetofend(struct btf_ext_header, line_info_len)) {
> > +               pr_warn("BTF.ext header missing func_info, line_info\n");
> > +               return -EINVAL;
> > +       }
>
> we have this check in btf_ext__new() already, do the swap after we
> validated this and keep this function simpler and more focused. You
> are mixing sanity checks with byte swapping, it's confusing.
>
> > +
> > +       sum_len = hdr->func_info_len + hdr->line_info_len;
> > +       if (hdr->hdr_len >= offsetofend(struct btf_ext_header, core_relo_len))
> > +               sum_len += hdr->core_relo_len;
> > +
> > +       info_size = data_size - hdr->hdr_len;
> > +       if (info_size != sum_len) {
> > +               pr_warn("BTF.ext info size mismatch with header data\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (info_size && info_size % sizeof(__u32)) {
> > +               pr_warn("BTF.ext info size not 32-bit multiple\n");
> > +               return -EINVAL;
> > +       }
>
> same as above, all this has nothing to do with endianness, it's all
> sanity checks. Let's extract them into a separate validation function
> that can happen before byte swap.
>
> > +
> > +       p = data + hdr->hdr_len;
> > +       for (i = 0; i < info_size / sizeof(__u32); i++, p++)
> > +               *p = bswap_32(*p);
>
> this is very suspicious, it might work right now because all fields
> are u32, but I'm uncomfortable making these decision.
>
> I think for byte swapping case we need to make sure that BTF version
> is exactly known to us (i.e., all record length is knowable for us),
> and then do more verbose, but correct byte swap based on
> bpf_func_info/bpf_line_info/etc records. Just treating all this as u32
> is not great.
>
> > +       return 0;
> > +}
> > +
> >  struct btf_ext_sec_setup_param {
> >         __u32 off;
> >         __u32 len;
> > @@ -3023,24 +3069,56 @@ static int btf_ext_setup_core_relos(struct btf_ext *btf_ext)
> >         return btf_ext_setup_info(btf_ext, &param);
> >  }
> >
> > -static int btf_ext_parse_hdr(__u8 *data, __u32 data_size)
> > +/* Swap byte-order of BTF.ext header */
> > +static void btf_ext_bswap_hdr(struct btf_ext_header *h)
> >  {
> > -       const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
> > +       __u32 hdr_len = h->hdr_len; /* need native byte-order */
> >
> > -       if (data_size < offsetofend(struct btf_ext_header, hdr_len) ||
> > -           data_size < hdr->hdr_len) {
> > -               pr_debug("BTF.ext header not found\n");
> > +       if (h->magic == bswap_16(BTF_MAGIC))
> > +               hdr_len = bswap_32(hdr_len);
>
> why so complicated? just pass hdr_len as input argument into
> btf_ext_bswap_hdr(), and then uniformly swap everything, no
> double-checking of decisions we already made
>
> > +
> > +       h->magic = bswap_16(h->magic);
> > +       h->hdr_len = bswap_32(h->hdr_len);
> > +       h->func_info_off = bswap_32(h->func_info_off);
> > +       h->func_info_len = bswap_32(h->func_info_len);
> > +       h->line_info_off = bswap_32(h->line_info_off);
> > +       h->line_info_len = bswap_32(h->line_info_len);
> > +
> > +       if (hdr_len < offsetofend(struct btf_ext_header, core_relo_len))
> > +               return;
> > +
> > +       h->core_relo_off = bswap_32(h->core_relo_off);
> > +       h->core_relo_len = bswap_32(h->core_relo_len);
> > +}
> > +
> > +static int btf_ext_parse_hdr(struct btf_ext *btf_ext)
> > +{
> > +       struct btf_ext_header *hdr = btf_ext->hdr;
> > +       __u32 hdr_len, data_size = btf_ext->data_size;
> > +
> > +       if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
> > +               pr_debug("BTF.ext initial header not found\n");
>
> "BTF.ext header too short" ?
>
> >                 return -EINVAL;
> >         }
> >
> > +       hdr_len = hdr->hdr_len;
> >         if (hdr->magic == bswap_16(BTF_MAGIC)) {
> > -               pr_warn("BTF.ext in non-native endianness is not supported\n");
> > -               return -ENOTSUP;
> > +               btf_ext->swapped_endian = true;
> > +               hdr_len = bswap_32(hdr_len);
> >         } else if (hdr->magic != BTF_MAGIC) {
> >                 pr_debug("Invalid BTF.ext magic:%x\n", hdr->magic);
> >                 return -EINVAL;
> >         }
> >
> > +       if (data_size < hdr_len) {
> > +               pr_debug("BTF.ext header not found\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* Maintain native byte-order in memory for introspection */
> > +       if (btf_ext->swapped_endian)
> > +               btf_ext_bswap_hdr(hdr);
> > +
> >         if (hdr->version != BTF_VERSION) {
> >                 pr_debug("Unsupported BTF.ext version:%u\n", hdr->version);
> >                 return -ENOTSUP;
> > @@ -3067,6 +3145,7 @@ void btf_ext__free(struct btf_ext *btf_ext)
> >         free(btf_ext->line_info.sec_idxs);
> >         free(btf_ext->core_relo_info.sec_idxs);
> >         free(btf_ext->data);
> > +       free(btf_ext->data_swapped);
> >         free(btf_ext);
> >  }
> >
> > @@ -3087,7 +3166,12 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
> >         }
> >         memcpy(btf_ext->data, data, size);
> >
> > -       err = btf_ext_parse_hdr(btf_ext->data, size);
> > +       err = btf_ext_parse_hdr(btf_ext);
> > +       if (err)
> > +               goto done;
> > +
> > +       if (btf_ext->swapped_endian)
> > +               err = btf_ext_bswap_info(btf_ext->data, btf_ext->data_size);
> >         if (err)
> >                 goto done;
>
> nit:
>
> if (btf_ext->swapped_endian) {
>     err = ...;
>     if (err)
>         goto done;
> }
>
> i.e., keep error handling close and at the same nesting level
>
> >
> > @@ -3120,15 +3204,76 @@ struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
> >         return btf_ext;
> >  }
> >
> > +static void *btf_ext_raw_data(const struct btf_ext *btf_ext_ro, __u32 *size,
> > +                             bool swap_endian)
> > +{
> > +       struct btf_ext *btf_ext = (struct btf_ext *)btf_ext_ro;
> > +       const __u32 data_sz = btf_ext->data_size;
> > +       void *data;
> > +       int err;
> > +
> > +       data = swap_endian ? btf_ext->data_swapped : btf_ext->data;
> > +       if (data) {
> > +               *size = data_sz;
> > +               return data;
> > +       }
> > +
> > +       data = calloc(1, data_sz);
> > +       if (!data)
> > +               return NULL;
> > +       memcpy(data, btf_ext->data, data_sz);
> > +
> > +       if (swap_endian) {
> > +               err = btf_ext_bswap_info(data, data_sz);
> > +               if (err) {
> > +                       free(data);
> > +                       return NULL;
> > +               }
> > +               btf_ext_bswap_hdr(data);
> > +               btf_ext->data_swapped = data;
> > +       }
> > +
> > +       *size = data_sz;
> > +       return data;
> > +}
> > +
> >  const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size)
> >  {
> > +       __u32 data_sz;
> > +       void *data;
> > +
> > +       data = btf_ext_raw_data(btf_ext, &data_sz, btf_ext->swapped_endian);
> > +       if (!data)
> > +               return errno = ENOMEM, NULL;
> > +
> >         *size = btf_ext->data_size;
> > -       return btf_ext->data;
> > +       return data;
> >  }
> >
> >  __attribute__((alias("btf_ext__raw_data")))
> >  const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext, __u32 *size);
> >
> > +enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext)
> > +{
> > +       return (is_host_big_endian() != btf_ext->swapped_endian) ?
> > +               BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
> > +}

nit: this looks correct, but I'd prefer consistency with
btf__endianness() in terms of implementation, if you don't mind

> > +
> > +int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
> > +{
> > +       bool is_tgt_big_endian = (endian == BTF_BIG_ENDIAN);
> > +
> > +       if (endian != BTF_LITTLE_ENDIAN && endian != BTF_BIG_ENDIAN)
> > +               return libbpf_err(-EINVAL);
> > +
> > +       btf_ext->swapped_endian = (is_host_big_endian() != is_tgt_big_endian);

nit: any reason you didn't just stick to the same style as in
btf__set_endianness()? you clearly took that as an inspiration. Were
you worried about crossing the 80 characters boundary? Don't be, the
limit is 100 characters. Anyways, unnecessary () and I'd inline
is_tgt_big_endian given it's single use.


> > +
> > +       if (!btf_ext->swapped_endian) {
> > +               free(btf_ext->data_swapped);
> > +               btf_ext->data_swapped = NULL;
> > +       }
> > +       return 0;
> > +}
> >
> >  struct btf_dedup;
> >
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index b68d216837a9..e3cf91687c78 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -167,6 +167,9 @@ LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
> >  LIBBPF_API struct btf_ext *btf_ext__new(const __u8 *data, __u32 size);
> >  LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
> >  LIBBPF_API const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size);
> > +LIBBPF_API enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext);
> > +LIBBPF_API int btf_ext__set_endianness(struct btf_ext *btf_ext,
> > +                                      enum btf_endianness endian);
> >
> >  LIBBPF_API int btf__find_str(struct btf *btf, const char *s);
> >  LIBBPF_API int btf__add_str(struct btf *btf, const char *s);
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 8f0d9ea3b1b4..5c17632807b6 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -421,6 +421,8 @@ LIBBPF_1.5.0 {
> >         global:
> >                 btf__distill_base;
> >                 btf__relocate;
> > +               btf_ext__endianness;
> > +               btf_ext__set_endianness;
> >                 bpf_map__autoattach;
> >                 bpf_map__set_autoattach;
> >                 bpf_program__attach_sockmap;
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 8cda511a1982..6b0270c83537 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -484,6 +484,8 @@ struct btf_ext {
> >                 struct btf_ext_header *hdr;
> >                 void *data;
> >         };
> > +       void *data_swapped;
> > +       bool swapped_endian;
> >         struct btf_ext_info func_info;
> >         struct btf_ext_info line_info;
> >         struct btf_ext_info core_relo_info;
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index cf4f7bd7ff5c..cb4ee4ed4b5c 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -2884,6 +2884,52 @@  int btf__add_decl_tag(struct btf *btf, const char *value, int ref_type_id,
 	return btf_commit_type(btf, sz);
 }
 
+/*
+ * Swap endianness of the info segment in a BTF.ext data section:
+ *   - requires BTF.ext header data in native byte order
+ *   - expects info record fields are 32-bit
+ */
+static int btf_ext_bswap_info(void *data, const __u32 data_size)
+{
+	const struct btf_ext_header *hdr = data;
+	__u32 info_size, sum_len, i, *p;
+
+	if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
+		pr_warn("BTF.ext initial header not found\n");
+		return -EINVAL;
+	}
+
+	if (data_size < hdr->hdr_len) {
+		pr_warn("BTF.ext header not found\n");
+		return -EINVAL;
+	}
+
+	if (hdr->hdr_len < offsetofend(struct btf_ext_header, line_info_len)) {
+		pr_warn("BTF.ext header missing func_info, line_info\n");
+		return -EINVAL;
+	}
+
+	sum_len = hdr->func_info_len + hdr->line_info_len;
+	if (hdr->hdr_len >= offsetofend(struct btf_ext_header, core_relo_len))
+		sum_len += hdr->core_relo_len;
+
+	info_size = data_size - hdr->hdr_len;
+	if (info_size != sum_len) {
+		pr_warn("BTF.ext info size mismatch with header data\n");
+		return -EINVAL;
+	}
+
+	if (info_size && info_size % sizeof(__u32)) {
+		pr_warn("BTF.ext info size not 32-bit multiple\n");
+		return -EINVAL;
+	}
+
+	p = data + hdr->hdr_len;
+	for (i = 0; i < info_size / sizeof(__u32); i++, p++)
+		*p = bswap_32(*p);
+	return 0;
+}
+
 struct btf_ext_sec_setup_param {
 	__u32 off;
 	__u32 len;
@@ -3023,24 +3069,56 @@  static int btf_ext_setup_core_relos(struct btf_ext *btf_ext)
 	return btf_ext_setup_info(btf_ext, &param);
 }
 
-static int btf_ext_parse_hdr(__u8 *data, __u32 data_size)
+/* Swap byte-order of BTF.ext header */
+static void btf_ext_bswap_hdr(struct btf_ext_header *h)
 {
-	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
+	__u32 hdr_len = h->hdr_len; /* need native byte-order */
 
-	if (data_size < offsetofend(struct btf_ext_header, hdr_len) ||
-	    data_size < hdr->hdr_len) {
-		pr_debug("BTF.ext header not found\n");
+	if (h->magic == bswap_16(BTF_MAGIC))
+		hdr_len = bswap_32(hdr_len);
+
+	h->magic = bswap_16(h->magic);
+	h->hdr_len = bswap_32(h->hdr_len);
+	h->func_info_off = bswap_32(h->func_info_off);
+	h->func_info_len = bswap_32(h->func_info_len);
+	h->line_info_off = bswap_32(h->line_info_off);
+	h->line_info_len = bswap_32(h->line_info_len);
+
+	if (hdr_len < offsetofend(struct btf_ext_header, core_relo_len))
+		return;
+
+	h->core_relo_off = bswap_32(h->core_relo_off);
+	h->core_relo_len = bswap_32(h->core_relo_len);
+}
+
+static int btf_ext_parse_hdr(struct btf_ext *btf_ext)
+{
+	struct btf_ext_header *hdr = btf_ext->hdr;
+	__u32 hdr_len, data_size = btf_ext->data_size;
+
+	if (data_size < offsetofend(struct btf_ext_header, hdr_len)) {
+		pr_debug("BTF.ext initial header not found\n");
 		return -EINVAL;
 	}
 
+	hdr_len = hdr->hdr_len;
 	if (hdr->magic == bswap_16(BTF_MAGIC)) {
-		pr_warn("BTF.ext in non-native endianness is not supported\n");
-		return -ENOTSUP;
+		btf_ext->swapped_endian = true;
+		hdr_len = bswap_32(hdr_len);
 	} else if (hdr->magic != BTF_MAGIC) {
 		pr_debug("Invalid BTF.ext magic:%x\n", hdr->magic);
 		return -EINVAL;
 	}
 
+	if (data_size < hdr_len) {
+		pr_debug("BTF.ext header not found\n");
+		return -EINVAL;
+	}
+
+	/* Maintain native byte-order in memory for introspection */
+	if (btf_ext->swapped_endian)
+		btf_ext_bswap_hdr(hdr);
+
 	if (hdr->version != BTF_VERSION) {
 		pr_debug("Unsupported BTF.ext version:%u\n", hdr->version);
 		return -ENOTSUP;
@@ -3067,6 +3145,7 @@  void btf_ext__free(struct btf_ext *btf_ext)
 	free(btf_ext->line_info.sec_idxs);
 	free(btf_ext->core_relo_info.sec_idxs);
 	free(btf_ext->data);
+	free(btf_ext->data_swapped);
 	free(btf_ext);
 }
 
@@ -3087,7 +3166,12 @@  struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
 	}
 	memcpy(btf_ext->data, data, size);
 
-	err = btf_ext_parse_hdr(btf_ext->data, size);
+	err = btf_ext_parse_hdr(btf_ext);
+	if (err)
+		goto done;
+
+	if (btf_ext->swapped_endian)
+		err = btf_ext_bswap_info(btf_ext->data, btf_ext->data_size);
 	if (err)
 		goto done;
 
@@ -3120,15 +3204,76 @@  struct btf_ext *btf_ext__new(const __u8 *data, __u32 size)
 	return btf_ext;
 }
 
+static void *btf_ext_raw_data(const struct btf_ext *btf_ext_ro, __u32 *size,
+			      bool swap_endian)
+{
+	struct btf_ext *btf_ext = (struct btf_ext *)btf_ext_ro;
+	const __u32 data_sz = btf_ext->data_size;
+	void *data;
+	int err;
+
+	data = swap_endian ? btf_ext->data_swapped : btf_ext->data;
+	if (data) {
+		*size = data_sz;
+		return data;
+	}
+
+	data = calloc(1, data_sz);
+	if (!data)
+		return NULL;
+	memcpy(data, btf_ext->data, data_sz);
+
+	if (swap_endian) {
+		err = btf_ext_bswap_info(data, data_sz);
+		if (err) {
+			free(data);
+			return NULL;
+		}
+		btf_ext_bswap_hdr(data);
+		btf_ext->data_swapped = data;
+	}
+
+	*size = data_sz;
+	return data;
+}
+
 const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size)
 {
+	__u32 data_sz;
+	void *data;
+
+	data = btf_ext_raw_data(btf_ext, &data_sz, btf_ext->swapped_endian);
+	if (!data)
+		return errno = ENOMEM, NULL;
+
 	*size = btf_ext->data_size;
-	return btf_ext->data;
+	return data;
 }
 
 __attribute__((alias("btf_ext__raw_data")))
 const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext, __u32 *size);
 
+enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext)
+{
+	return (is_host_big_endian() != btf_ext->swapped_endian) ?
+		BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
+}
+
+int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
+{
+	bool is_tgt_big_endian = (endian == BTF_BIG_ENDIAN);
+
+	if (endian != BTF_LITTLE_ENDIAN && endian != BTF_BIG_ENDIAN)
+		return libbpf_err(-EINVAL);
+
+	btf_ext->swapped_endian = (is_host_big_endian() != is_tgt_big_endian);
+
+	if (!btf_ext->swapped_endian) {
+		free(btf_ext->data_swapped);
+		btf_ext->data_swapped = NULL;
+	}
+	return 0;
+}
 
 struct btf_dedup;
 
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index b68d216837a9..e3cf91687c78 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -167,6 +167,9 @@  LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API struct btf_ext *btf_ext__new(const __u8 *data, __u32 size);
 LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
 LIBBPF_API const void *btf_ext__raw_data(const struct btf_ext *btf_ext, __u32 *size);
+LIBBPF_API enum btf_endianness btf_ext__endianness(const struct btf_ext *btf_ext);
+LIBBPF_API int btf_ext__set_endianness(struct btf_ext *btf_ext,
+				       enum btf_endianness endian);
 
 LIBBPF_API int btf__find_str(struct btf *btf, const char *s);
 LIBBPF_API int btf__add_str(struct btf *btf, const char *s);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 8f0d9ea3b1b4..5c17632807b6 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -421,6 +421,8 @@  LIBBPF_1.5.0 {
 	global:
 		btf__distill_base;
 		btf__relocate;
+		btf_ext__endianness;
+		btf_ext__set_endianness;
 		bpf_map__autoattach;
 		bpf_map__set_autoattach;
 		bpf_program__attach_sockmap;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 8cda511a1982..6b0270c83537 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -484,6 +484,8 @@  struct btf_ext {
 		struct btf_ext_header *hdr;
 		void *data;
 	};
+	void *data_swapped;
+	bool swapped_endian;
 	struct btf_ext_info func_info;
 	struct btf_ext_info line_info;
 	struct btf_ext_info core_relo_info;