Re: [PATCH v3 bpf-next 11/13] libbpf: support llvm-generated indirect jumps

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/09/19 04:18PM, Andrii Nakryiko wrote:
> On Thu, Sep 18, 2025 at 2:32 AM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > For v5 instruction set LLVM is allowed to generate indirect jumps for
> > switch statements and for 'goto *rX' assembly. Every such a jump will
> > be accompanied by necessary metadata, e.g. (`llvm-objdump -Sr ...`):
> >
> >        0:       r2 = 0x0 ll
> >                 0000000000000030:  R_BPF_64_64  BPF.JT.0.0
> >
> > Here BPF.JT.1.0 is a symbol residing in the .jumptables section:
> >
> >     Symbol table:
> >        4: 0000000000000000   240 OBJECT  GLOBAL DEFAULT     4 BPF.JT.0.0
> >
> > The -bpf-min-jump-table-entries llvm option may be used to control the
> > minimal size of a switch which will be converted to an indirect jumps.
> >
> > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > ---
> >  tools/lib/bpf/libbpf.c        | 150 +++++++++++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf_probes.c |   4 +
> >  tools/lib/bpf/linker.c        |  10 ++-
> >  3 files changed, 161 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 2c1f48f77680..57cac0810d2e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -191,6 +191,7 @@ static const char * const map_type_name[] = {
> >         [BPF_MAP_TYPE_USER_RINGBUF]             = "user_ringbuf",
> >         [BPF_MAP_TYPE_CGRP_STORAGE]             = "cgrp_storage",
> >         [BPF_MAP_TYPE_ARENA]                    = "arena",
> > +       [BPF_MAP_TYPE_INSN_ARRAY]               = "insn_array",
> >  };
> >
> >  static const char * const prog_type_name[] = {
> > @@ -372,6 +373,7 @@ enum reloc_type {
> >         RELO_EXTERN_CALL,
> >         RELO_SUBPROG_ADDR,
> >         RELO_CORE,
> > +       RELO_INSN_ARRAY,
> >  };
> >
> >  struct reloc_desc {
> > @@ -382,7 +384,10 @@ struct reloc_desc {
> >                 struct {
> >                         int map_idx;
> >                         int sym_off;
> > -                       int ext_idx;
> > +                       union {
> > +                               int ext_idx;
> > +                               int sym_size;
> > +                       };
> >                 };
> >         };
> >  };
> > @@ -424,6 +429,11 @@ struct bpf_sec_def {
> >         libbpf_prog_attach_fn_t prog_attach_fn;
> >  };
> >
> > +struct bpf_light_subprog {
> > +       __u32 sec_insn_off;
> > +       __u32 sub_insn_off;
> > +};
> > +
> >  /*
> >   * bpf_prog should be a better name but it has been used in
> >   * linux/filter.h.
> > @@ -496,6 +506,9 @@ struct bpf_program {
> >         __u32 line_info_rec_size;
> >         __u32 line_info_cnt;
> >         __u32 prog_flags;
> > +
> > +       struct bpf_light_subprog *subprog;
> 
> nit: subprogs (but still subprog_cnt, yep)

done

> 
> > +       __u32 subprog_cnt;
> >  };
> >
> >  struct bpf_struct_ops {
> > @@ -525,6 +538,7 @@ struct bpf_struct_ops {
> >  #define STRUCT_OPS_SEC ".struct_ops"
> >  #define STRUCT_OPS_LINK_SEC ".struct_ops.link"
> >  #define ARENA_SEC ".addr_space.1"
> > +#define JUMPTABLES_SEC ".jumptables"
> >
> >  enum libbpf_map_type {
> >         LIBBPF_MAP_UNSPEC,
> > @@ -668,6 +682,7 @@ struct elf_state {
> >         int symbols_shndx;
> >         bool has_st_ops;
> >         int arena_data_shndx;
> > +       int jumptables_data_shndx;
> >  };
> >
> >  struct usdt_manager;
> > @@ -739,6 +754,9 @@ struct bpf_object {
> >         void *arena_data;
> >         size_t arena_data_sz;
> >
> > +       void *jumptables_data;
> > +       size_t jumptables_data_sz;
> > +
> >         struct kern_feature_cache *feat_cache;
> >         char *token_path;
> >         int token_fd;
> > @@ -765,6 +783,7 @@ void bpf_program__unload(struct bpf_program *prog)
> >
> >         zfree(&prog->func_info);
> >         zfree(&prog->line_info);
> > +       zfree(&prog->subprog);
> >  }
> >
> >  static void bpf_program__exit(struct bpf_program *prog)
> > @@ -3945,6 +3964,13 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> >                         } else if (strcmp(name, ARENA_SEC) == 0) {
> >                                 obj->efile.arena_data = data;
> >                                 obj->efile.arena_data_shndx = idx;
> > +                       } else if (strcmp(name, JUMPTABLES_SEC) == 0) {
> > +                               obj->jumptables_data = malloc(data->d_size);
> > +                               if (!obj->jumptables_data)
> > +                                       return -ENOMEM;
> > +                               memcpy(obj->jumptables_data, data->d_buf, data->d_size);
> > +                               obj->jumptables_data_sz = data->d_size;
> > +                               obj->efile.jumptables_data_shndx = idx;
> >                         } else {
> >                                 pr_info("elf: skipping unrecognized data section(%d) %s\n",
> >                                         idx, name);
> > @@ -4599,6 +4625,16 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
> >                 return 0;
> >         }
> >
> > +       /* jump table data relocation */
> > +       if (shdr_idx == obj->efile.jumptables_data_shndx) {
> > +               reloc_desc->type = RELO_INSN_ARRAY;
> > +               reloc_desc->insn_idx = insn_idx;
> > +               reloc_desc->map_idx = -1;
> > +               reloc_desc->sym_off = sym->st_value;
> > +               reloc_desc->sym_size = sym->st_size;
> > +               return 0;
> > +       }
> > +
> >         /* generic map reference relocation */
> >         if (type == LIBBPF_MAP_UNSPEC) {
> >                 if (!bpf_object__shndx_is_maps(obj, shdr_idx)) {
> > @@ -6101,6 +6137,74 @@ static void poison_kfunc_call(struct bpf_program *prog, int relo_idx,
> >         insn->imm = POISON_CALL_KFUNC_BASE + ext_idx;
> >  }
> >
> > +static int create_jt_map(struct bpf_object *obj, int off, int size, int adjust_off)
> > +{
> > +       const __u32 value_size = sizeof(struct bpf_insn_array_value);
> > +       const __u32 max_entries = size / value_size;
> > +       struct bpf_insn_array_value val = {};
> > +       int map_fd, err;
> > +       __u64 xlated_off;
> > +       __u64 *jt;
> > +       __u32 i;
> > +
> > +       map_fd = bpf_map_create(BPF_MAP_TYPE_INSN_ARRAY, "jt",
> 
> let's call it ".jumptables" just like special global data maps?

done

> > +                               4, value_size, max_entries, NULL);
> > +       if (map_fd < 0)
> > +               return map_fd;
> > +
> > +       if (!obj->jumptables_data) {
> > +               pr_warn("object contains no jumptables_data\n");
> 
> for map-related errors we follow (pretty consistently) error format:
> 
> map '%s': whatever bad happened
> 
> let's stick to that here? "map '.jumptables': ELF file is missing jump
> table data" or something along those lines?

sure, thanks

> > +               return -EINVAL;
> > +       }
> > +       if ((off + size) > obj->jumptables_data_sz) {
> 
> nit: unnecessary ()

Thanks, removed

> > +               pr_warn("jumptables_data size is %zd, trying to access %d\n",
> > +                       obj->jumptables_data_sz, off + size);
> > +               return -EINVAL;
> > +       }
> > +
> > +       jt = (__u64 *)(obj->jumptables_data + off);
> > +       for (i = 0; i < max_entries; i++) {
> > +               /*
> > +                * LLVM-generated jump tables contain u64 records, however
> > +                * should contain values that fit in u32.
> > +                * The adjust_off provided by the caller adjusts the offset to
> > +                * be relative to the beginning of the main function
> > +                */
> > +               xlated_off = jt[i]/sizeof(struct bpf_insn) + adjust_off;
> > +               if (xlated_off > UINT32_MAX) {
> > +                       pr_warn("invalid jump table value %llx at offset %d (adjust_off %d)\n",
> > +                               jt[i], off + i, adjust_off);
> 
> no close(map_fd)? same in a bunch of places above? I'd actually move
> map create to right before this loop and simplify error handling

oops, thanks...

> pw-bot: cr
> 
> > +                       return -EINVAL;
> > +               }
> > +
> > +               val.xlated_off = xlated_off;
> > +               err = bpf_map_update_elem(map_fd, &i, &val, 0);
> > +               if (err) {
> > +                       close(map_fd);
> > +                       return err;
> > +               }
> > +       }
> > +       return map_fd;
> > +}
> > +
> > +/*
> > + * In LLVM the .jumptables section contains jump tables entries relative to the
> > + * section start. The BPF kernel-side code expects jump table offsets relative
> > + * to the beginning of the program (passed in bpf(BPF_PROG_LOAD)). This helper
> > + * computes a delta to be added when creating a map.
> > + */
> > +static int jt_adjust_off(struct bpf_program *prog, int insn_idx)
> > +{
> > +       int i;
> > +
> > +       for (i = prog->subprog_cnt - 1; i >= 0; i--)
> > +               if (insn_idx >= prog->subprog[i].sub_insn_off)
> > +                       return prog->subprog[i].sub_insn_off - prog->subprog[i].sec_insn_off;
> 
> nit: please add {} around multi-line for loop body (even if it's a
> single statement)

Sure, done.

> > +
> > +       return -prog->sec_insn_off;
> > +}
> > +
> > +
> >  /* Relocate data references within program code:
> >   *  - map references;
> >   *  - global variable references;
> > @@ -6192,6 +6296,21 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
> >                 case RELO_CORE:
> >                         /* will be handled by bpf_program_record_relos() */
> >                         break;
> > +               case RELO_INSN_ARRAY: {
> > +                       int map_fd;
> > +
> > +                       map_fd = create_jt_map(obj, relo->sym_off, relo->sym_size,
> > +                                              jt_adjust_off(prog, relo->insn_idx));
> 
> Who's closing all these fds? (I feel like we'd want to have all those
> maps in a list of bpf_object's maps, just like .rodata and others)

Ok, thanks, I've overlooked this.

> Also, how many of those will we have? Each individual relocation gets
> its own map, right?..

Yes. I think I didn't have a case where we have two loads fo the same
table. I will take a look at if this makes sense to add such a use
case, and then I will change this code to create only one map.

> 
> > +                       if (map_fd < 0) {
> > +                               pr_warn("prog '%s': relo #%d: can't create jump table: sym_off %u\n",
> > +                                               prog->name, i, relo->sym_off);
> > +                               return map_fd;
> > +                       }
> > +                       insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
> > +                       insn->imm = map_fd;
> > +                       insn->off = 0;
> > +               }
> > +                       break;
> >                 default:
> >                         pr_warn("prog '%s': relo #%d: bad relo type %d\n",
> >                                 prog->name, i, relo->type);
> > @@ -6389,6 +6508,24 @@ static int append_subprog_relos(struct bpf_program *main_prog, struct bpf_progra
> >         return 0;
> >  }
> >
> > +static int save_subprog_offsets(struct bpf_program *main_prog, struct bpf_program *subprog)
> > +{
> > +       size_t size = sizeof(main_prog->subprog[0]);
> > +       int new_cnt = main_prog->subprog_cnt + 1;
> > +       void *tmp;
> > +
> > +       tmp = libbpf_reallocarray(main_prog->subprog, new_cnt, size);
> > +       if (!tmp)
> > +               return -ENOMEM;
> > +
> > +       main_prog->subprog = tmp;
> > +       main_prog->subprog[new_cnt - 1].sec_insn_off = subprog->sec_insn_off;
> > +       main_prog->subprog[new_cnt - 1].sub_insn_off = subprog->sub_insn_off;
> > +       main_prog->subprog_cnt = new_cnt;
> > +
> > +       return 0;
> > +}
> > +
> >  static int
> >  bpf_object__append_subprog_code(struct bpf_object *obj, struct bpf_program *main_prog,
> >                                 struct bpf_program *subprog)
> > @@ -6418,6 +6555,14 @@ bpf_object__append_subprog_code(struct bpf_object *obj, struct bpf_program *main
> >         err = append_subprog_relos(main_prog, subprog);
> >         if (err)
> >                 return err;
> > +
> > +       /* Save subprogram offsets */
> > +       err = save_subprog_offsets(main_prog, subprog);
> > +       if (err) {
> > +               pr_warn("prog '%s': failed to add subprog offsets\n", main_prog->name);
> 
> emit error itself as well, use errstr()

ok, done

> > +               return err;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -9185,6 +9330,9 @@ void bpf_object__close(struct bpf_object *obj)
> >
> >         zfree(&obj->arena_data);
> >
> > +       zfree(&obj->jumptables_data);
> > +       obj->jumptables_data_sz = 0;
> > +
> >         free(obj);
> >  }
> >
> > diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> > index 9dfbe7750f56..bccf4bb747e1 100644
> > --- a/tools/lib/bpf/libbpf_probes.c
> > +++ b/tools/lib/bpf/libbpf_probes.c
> > @@ -364,6 +364,10 @@ static int probe_map_create(enum bpf_map_type map_type)
> >         case BPF_MAP_TYPE_SOCKHASH:
> >         case BPF_MAP_TYPE_REUSEPORT_SOCKARRAY:
> >                 break;
> > +       case BPF_MAP_TYPE_INSN_ARRAY:
> > +               key_size        = sizeof(__u32);
> > +               value_size      = sizeof(struct bpf_insn_array_value);
> > +               break;
> >         case BPF_MAP_TYPE_UNSPEC:
> >         default:
> >                 return -EOPNOTSUPP;
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index a469e5d4fee7..d1585baa9f14 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -28,6 +28,8 @@
> >  #include "str_error.h"
> >
> >  #define BTF_EXTERN_SEC ".extern"
> > +#define JUMPTABLES_SEC ".jumptables"
> > +#define JUMPTABLES_REL_SEC ".rel.jumptables"
> >
> >  struct src_sec {
> >         const char *sec_name;
> > @@ -2026,6 +2028,9 @@ static int linker_append_elf_sym(struct bpf_linker *linker, struct src_obj *obj,
> >                         obj->sym_map[src_sym_idx] = dst_sec->sec_sym_idx;
> >                         return 0;
> >                 }
> > +
> > +               if (strcmp(src_sec->sec_name, JUMPTABLES_SEC) == 0)
> > +                       goto add_sym;
> >         }
> >
> >         if (sym_bind == STB_LOCAL)
> > @@ -2272,8 +2277,9 @@ static int linker_append_elf_relos(struct bpf_linker *linker, struct src_obj *ob
> >                                                 insn->imm += sec->dst_off / sizeof(struct bpf_insn);
> >                                         else
> >                                                 insn->imm += sec->dst_off;
> > -                               } else {
> > -                                       pr_warn("relocation against STT_SECTION in non-exec section is not supported!\n");
> > +                               } else if (strcmp(src_sec->sec_name, JUMPTABLES_REL_SEC)) {
> 
> please add explicit `!= 0`, but also didn't we agree to have
> 
> if (strcmp(..., JUMPTABLES_REL_SEC) == 0) {
>     /* no need to adjust .jumptables */
> } else {
>     ... original default handling of errors ...
>
> 
> Also, how did you test that this actually works? Can you add a
> selftest demonstrating this?

I see that I've missed your comment about linking two objects.
I will add a selftest and patch the code above as you've suggested.

> }
> 
> > +                                       pr_warn("relocation against STT_SECTION in section %s is not supported!\n",
> > +                                               src_sec->sec_name);
> >                                         return -EINVAL;
> >                                 }
> >                         }
> > --
> > 2.34.1
> >