Re: [PATCH bpf-next v2 7/8] libbpf: Support creating light skeleton of either endianness

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 22, 2024 at 2:25 AM Tony Ambardar <tony.ambardar@xxxxxxxxx> wrote:
>
> From: Tony Ambardar <tony.ambardar@xxxxxxxxx>
>
> Track target endianness in 'struct bpf_gen' and process in-memory data in
> native byte-order, but on finalization convert the embedded loader BPF
> insns to target endianness.
>
> The light skeleton also includes a target-accessed data blob which is
> heterogeneous and thus difficult to convert to target byte-order on
> finalization. Add support functions to convert data to target endianness
> as it is added to the blob.
>
> Also add additional debug logging for data blob structure details and
> skeleton loading.
>
> Signed-off-by: Tony Ambardar <tony.ambardar@xxxxxxxxx>
> ---
>  tools/lib/bpf/bpf_gen_internal.h |   1 +
>  tools/lib/bpf/gen_loader.c       | 187 +++++++++++++++++++++++--------
>  tools/lib/bpf/libbpf.c           |   1 +
>  tools/lib/bpf/skel_internal.h    |   3 +-
>  4 files changed, 147 insertions(+), 45 deletions(-)
>

[...]

> +/*
> + * Fields of bpf_attr are set to values in native byte-order before being
> + * written to the target-bound data blob, and may need endian conversion.
> + * This macro allows setting the correct value in situ and is simpler than
> + * writing a separate converter for *all fields* of *all records* included
> + * in union bpf_attr.
> + */
> +#define move_tgt_endian(lval, rval) {                          \
> +       if (!gen->swapped_endian)                               \
> +               lval = (rval);                                  \

add {}  here and for else branch, please

> +       else                                                    \
> +               switch (sizeof(lval)) {                         \
> +               case 2:                                         \
> +                       lval = bswap_16(rval);                  \
> +                       break;                                  \
> +               case 4:                                         \
> +                       lval = bswap_32(rval);                  \
> +                       break;                                  \
> +               case 8:                                         \
> +                       lval = bswap_64(rval);                  \
> +                       break;                                  \

I'd also go for more compact:


case 2: lval = bswap_16(rval); break;
case 4: lval = bswap_32(rval); break;

> +               default:                                        \
> +                       lval = (rval);                          \
> +                       pr_warn("unsupported bswap size!\n");   \

case 1: is unsupported? It just doesn't need a byte swap, so please
add it explicitly and avoid unnecessary warnings

> +               }                                               \
> +       }
> +
>  void bpf_gen__load_btf(struct bpf_gen *gen, const void *btf_raw_data,
>                        __u32 btf_raw_size)
>  {

[...]

>         emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
>         emit(gen, BPF_ALU32_IMM(BPF_AND, BPF_REG_9, reg_mask));
>         emit(gen, BPF_STX_MEM(BPF_B, BPF_REG_8, BPF_REG_9, offsetofend(struct bpf_insn, code)));
> @@ -931,11 +971,34 @@ static void cleanup_relos(struct bpf_gen *gen, int insns)
>         cleanup_core_relo(gen);
>  }
>
> +/* Covert func, line, and core relo info records to target endianness,

typo: convert

> + * checking the blob size is consistent with 32-bit fields.
> + */
> +static void info_blob_bswap(struct bpf_gen *gen, int info_off, __u32 size)
> +{
> +       __u32 *field = gen->data_start + info_off;
> +       int i, cnt = size / sizeof(__u32);
> +
> +       if (size && size % sizeof(__u32)) {

nit: () around mod operation

> +               pr_warn("info records not using 32-bit fields!\n");
> +               return;
> +       }
> +       if (gen->swapped_endian)
> +               for (i = 0; i < cnt; i++, field++)
> +                       *field = bswap_32(*field);
> +}
> +
>  void bpf_gen__prog_load(struct bpf_gen *gen,
>                         enum bpf_prog_type prog_type, const char *prog_name,
>                         const char *license, struct bpf_insn *insns, size_t insn_cnt,
>                         struct bpf_prog_load_opts *load_attr, int prog_idx)
>  {
> +       int func_info_tot_sz = load_attr->func_info_cnt *
> +                              load_attr->func_info_rec_size;
> +       int line_info_tot_sz = load_attr->line_info_cnt *
> +                              load_attr->line_info_rec_size;
> +       int core_relo_tot_sz = gen->core_relo_cnt *
> +                              sizeof(struct bpf_core_relo);
>         int prog_load_attr, license_off, insns_off, func_info, line_info, core_relos;
>         int attr_size = offsetofend(union bpf_attr, core_relo_rec_size);
>         union bpf_attr attr;
> @@ -947,32 +1010,60 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
>         license_off = add_data(gen, license, strlen(license) + 1);
>         /* add insns to blob of bytes */
>         insns_off = add_data(gen, insns, insn_cnt * sizeof(struct bpf_insn));
> +       pr_debug("gen: prog_load: license off %d insn off %d\n",
> +                license_off, insns_off);
>
> -       attr.prog_type = prog_type;
> -       attr.expected_attach_type = load_attr->expected_attach_type;
> -       attr.attach_btf_id = load_attr->attach_btf_id;
> -       attr.prog_ifindex = load_attr->prog_ifindex;
> -       attr.kern_version = 0;
> -       attr.insn_cnt = (__u32)insn_cnt;
> -       attr.prog_flags = load_attr->prog_flags;
> -
> -       attr.func_info_rec_size = load_attr->func_info_rec_size;
> -       attr.func_info_cnt = load_attr->func_info_cnt;
> -       func_info = add_data(gen, load_attr->func_info,
> -                            attr.func_info_cnt * attr.func_info_rec_size);
> +       /* convert blob insns to target endianness */
> +       if (gen->swapped_endian) {
> +               struct bpf_insn *insn = gen->data_start + insns_off;
> +               int i;
>
> -       attr.line_info_rec_size = load_attr->line_info_rec_size;
> -       attr.line_info_cnt = load_attr->line_info_cnt;
> -       line_info = add_data(gen, load_attr->line_info,
> -                            attr.line_info_cnt * attr.line_info_rec_size);
> +               for (i = 0; i < insn_cnt; i++, insn++)
> +                       bpf_insn_bswap(insn);
> +       }
>
> -       attr.core_relo_rec_size = sizeof(struct bpf_core_relo);
> -       attr.core_relo_cnt = gen->core_relo_cnt;
> -       core_relos = add_data(gen, gen->core_relos,
> -                            attr.core_relo_cnt * attr.core_relo_rec_size);
> +       move_tgt_endian(attr.prog_type, prog_type);
> +       move_tgt_endian(attr.expected_attach_type, load_attr->expected_attach_type);
> +       move_tgt_endian(attr.attach_btf_id, load_attr->attach_btf_id);
> +       move_tgt_endian(attr.prog_ifindex, load_attr->prog_ifindex);
> +       attr.kern_version = 0;
> +       move_tgt_endian(attr.insn_cnt, (__u32)insn_cnt);
> +       move_tgt_endian(attr.prog_flags, load_attr->prog_flags);
> +
> +       move_tgt_endian(attr.func_info_rec_size, load_attr->func_info_rec_size);
> +       move_tgt_endian(attr.func_info_cnt, load_attr->func_info_cnt);

this is quite intrusive, maybe instead of imperative move_tgt_endian()
macro, develop the one that could be used as

attr.func_info_cnt = tgt_endian(load_attr->func_info_cnt);

? I.e., working as an expression, taking into account the need to swap
and byte size of the argument. Should be doable.

> +       func_info = add_data(gen, load_attr->func_info, func_info_tot_sz);
> +       pr_debug("gen: prog_load: func_info: off %d cnt %d rec size %d\n",
> +                func_info, load_attr->func_info_cnt,
> +                load_attr->func_info_rec_size);

[...]





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux