On Mon, Sep 16, 2024 at 1:38 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> wrote:
>
> 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 internal header functions for byte-swapping func, line, and core info
> records.
>
> 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] 3289959b97ca ("libbpf: Support BTF loading and raw data output in both endianness")
> [2] 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
>
> Signed-off-by: Tony Ambardar <tony.ambardar@xxxxxxxxx>
> ---
> tools/lib/bpf/btf.c | 280 +++++++++++++++++++++++++-------
> tools/lib/bpf/btf.h | 3 +
> tools/lib/bpf/libbpf.map | 2 +
> tools/lib/bpf/libbpf_internal.h | 30 ++++
> 4 files changed, 258 insertions(+), 57 deletions(-)
>
[...]
>
> - /* The record size needs to meet the minimum standard */
> - record_size = *(__u32 *)info;
> + /* The record size needs to meet either the minimum standard or, when
> + * handling non-native endianness data, the exact standard so as
> + * to allow safe byte-swapping.
> + */
> + record_size = is_native ? *(__u32 *)info : bswap_32(*(__u32 *)info);
> if (record_size < ext_sec->min_rec_size ||
> + (!is_native && record_size != ext_sec->rec_size) ||
ok, so the way you define min_rec_size and rec_size is actually
broken. bpf_func_info, bpf_line_info, and now also bpf_core_relo all
come from kernel UAPI header, and it could happen that libbpf is
compiled against newest definitions of them without actually
"knowing"/supporting those newer and bigger struct definitions. So
assuming that sizeof(struct bpf_line_info) is a correct expected
record size is wrong.
As it is right now, min_rec_size is *the* rec_size, so I removed that
part and updated this check to record_size != ext_sec->min_rec_size.
If we ever extend .BTF.ext records, we'll need to add a bit more code
to accomodate for that.
> record_size & 0x03) {
> pr_debug("%s section in .BTF.ext has invalid record size %u\n",
> ext_sec->desc, record_size);
> @@ -2956,7 +2968,7 @@ static int btf_ext_setup_info(struct btf_ext *btf_ext,
> return -EINVAL;
> }
>
> - num_records = sinfo->num_info;
> + num_records = is_native ? sinfo->num_info : bswap_32(sinfo->num_info);
> if (num_records == 0) {
> pr_debug("%s section has incorrect num_records in .BTF.ext\n",
> ext_sec->desc);
> @@ -2984,64 +2996,160 @@ static int btf_ext_setup_info(struct btf_ext *btf_ext,
> return 0;
> }
>
> -static int btf_ext_setup_func_info(struct btf_ext *btf_ext)
> +/* Parse all info secs in the BTF.ext info data */
> +static int btf_ext_parse_info(struct btf_ext *btf_ext, bool is_native)
> {
> - struct btf_ext_sec_setup_param param = {
> + struct btf_ext_sec_info_param func_info = {
> .off = btf_ext->hdr->func_info_off,
> .len = btf_ext->hdr->func_info_len,
> .min_rec_size = sizeof(struct bpf_func_info_min),
> + .rec_size = sizeof(struct bpf_func_info),
> .ext_info = &btf_ext->func_info,
> .desc = "func_info"
> };
> -
> - return btf_ext_setup_info(btf_ext, ¶m);
> -}
> -
> -static int btf_ext_setup_line_info(struct btf_ext *btf_ext)
> -{
> - struct btf_ext_sec_setup_param param = {
> + struct btf_ext_sec_info_param line_info = {
> .off = btf_ext->hdr->line_info_off,
> .len = btf_ext->hdr->line_info_len,
> .min_rec_size = sizeof(struct bpf_line_info_min),
> + .rec_size = sizeof(struct bpf_line_info),
> .ext_info = &btf_ext->line_info,
> .desc = "line_info",
> };
> -
> - return btf_ext_setup_info(btf_ext, ¶m);
> -}
> -
> -static int btf_ext_setup_core_relos(struct btf_ext *btf_ext)
> -{
> - struct btf_ext_sec_setup_param param = {
> + struct btf_ext_sec_info_param core_relo = {
> .off = btf_ext->hdr->core_relo_off,
> .len = btf_ext->hdr->core_relo_len,
> .min_rec_size = sizeof(struct bpf_core_relo),
> + .rec_size = sizeof(struct bpf_core_relo),
> .ext_info = &btf_ext->core_relo_info,
> .desc = "core_relo",
> };
[...]
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4e349ad79ee6..47ee8f6ac489 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 0096e483f7eb..f40ccc2946e7 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_object__token_fd;
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 8cda511a1982..1307753b49b3 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
missing #include <byteswap.h> in libbpf_internal.h, you added too late
(in the next patch). I moved that include into this patch/commit to
not break bisectability, please be careful about things like that
> @@ -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;
> @@ -511,6 +513,34 @@ struct bpf_line_info_min {
> __u32 line_col;
> };
>
> +/* Functions to byte-swap info records */
> +
> +typedef void (*info_rec_bswap_fn)(void *);
> +
> +static inline void bpf_func_info_bswap(struct bpf_func_info *i)
> +{
> + i->insn_off = bswap_32(i->insn_off);
> + i->type_id = bswap_32(i->type_id);
> +}
> +
> +static inline void bpf_line_info_bswap(struct bpf_line_info *i)
> +{
> + i->insn_off = bswap_32(i->insn_off);
> + i->file_name_off = bswap_32(i->file_name_off);
> + i->line_off = bswap_32(i->line_off);
> + i->line_col = bswap_32(i->line_col);
> +}
> +
> +static inline void bpf_core_relo_bswap(struct bpf_core_relo *i)
> +{
> + _Static_assert(sizeof(i->kind) == sizeof(__u32),
> + "enum bpf_core_relo_kind is not 32-bit\n");
Do we need the endline in _Static_assert() messages? And generally I
think this is a bit too paranoid to check that that enum is backed by
u32, I just dropped the assertion. This is part of kernel UAPI, it
shouldn't change.
> + i->insn_off = bswap_32(i->insn_off);
> + i->type_id = bswap_32(i->type_id);
> + i->access_str_off = bswap_32(i->access_str_off);
> + i->kind = bswap_32(i->kind);
> +}
> +
> enum btf_field_iter_kind {
> BTF_FIELD_ITER_IDS,
> BTF_FIELD_ITER_STRS,
> --
> 2.34.1
>
