Re: [PATCH v6 bpf-next 14/17] libbpf: support llvm-generated indirect jumps

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

On 25/10/24 12:52PM, Anton Protopopov wrote:
> On 25/10/21 03:18PM, Eduard Zingerman wrote:
> > On Sun, 2025-10-19 at 20:21 +0000, Anton Protopopov wrote:
> > 
> > [...]
> > 
> > > ---
> > >  tools/lib/bpf/libbpf.c        | 240 +++++++++++++++++++++++++++++++++-
> > >  tools/lib/bpf/libbpf_probes.c |   4 +
> > >  tools/lib/bpf/linker.c        |  10 +-
> > >  3 files changed, 251 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index b90574f39d1c..ee44bc49a3ba 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > 
> > [...]
> > 
> > > +/*
> > > + * 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->subprogs[i].sub_insn_off)
> > 
> > Sorry, I'm still confused about what happens here.
> > The `insn_idx` is comes from relocation, meaning that it is a value
> > recorded relative to section start, right?  On the other hand,
> > `.sub_insn_off` is an offset of a subprogram within a concatenated
> > program, about to be loaded.  These values should not be compared
> > directly.
> > 
> > I think, that my suggestion from v5 [1] should be easier to understand:
> 
> Well, if you insist :) (I saw the next e-mail as well, thanks.)
> 
> >    > Or rename this thing to find_subprog_idx(), pass relo object into
> >    > create_jt_map(), call find_subprog_idx() there, and do the following:
> >    >
> >    >   xlated_off = jt[i] / sizeof(struct bpf_insn);
> >    >   /* make xlated_off relative to subprogram start */
> >    >   xlated_off -= prog->subprogs[subprog_idx].sec_insn_off;
> >    >   /* make xlated_off relative to main subprogram start */
> >    >   xlated_off += prog->subprogs[subprog_idx].sub_insn_off;
> > 
> > [1] https://lore.kernel.org/bpf/b5fd31c3e703c8c84c6710f5536510fbce04b36f.camel@gmail.com/
> > 
> > > +			return prog->subprogs[i].sub_insn_off - prog->subprogs[i].sec_insn_off;
> > > +	}
> > > +
> > > +	return -prog->sec_insn_off;
> > > +}
> > > +
> > > +
> > >  /* Relocate data references within program code:
> > >   *  - map references;
> > >   *  - global variable references;
> > > @@ -6235,6 +6422,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, prog, relo->sym_off, relo->sym_size,
> > > +					       jt_adjust_off(prog, relo->insn_idx));
> > > +			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);
> > 
> > [...]
> > 
> > > @@ -9228,6 +9457,15 @@ void bpf_object__close(struct bpf_object *obj)
> > >  
> > >  	zfree(&obj->arena_data);
> > >  
> > > +	zfree(&obj->jumptables_data);
> > > +	obj->jumptables_data_sz = 0;
> > > +
> > > +	if (obj->jumptable_maps && obj->jumptable_map_cnt) {
> > 
> > Nit: outer 'if' seems unnecessary.
> 
> I suspect this was a check for if obj->jumptable_maps is null or not.
> I think this should never happen that jumptable_map_cnt && !jumptable_map,
> so I will remove the if.
> 
> > > +		for (i = 0; i < obj->jumptable_map_cnt; i++)
> > > +			close(obj->jumptable_maps[i].fd);
> > > +	}
> > > +	zfree(&obj->jumptable_maps);
> > > +
> > >  	free(obj);
> > >  }
> > 
> > [...]
> > 
> > > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > > index 56ae77047bc3..3defd4bc9154 100644
> > > --- a/tools/lib/bpf/linker.c
> > > +++ b/tools/lib/bpf/linker.c
> > > @@ -27,6 +27,8 @@
> > >  #include "strset.h"
> > >  
> > >  #define BTF_EXTERN_SEC ".extern"
> > > +#define JUMPTABLES_SEC ".jumptables"
> > > +#define JUMPTABLES_REL_SEC ".rel.jumptables"
> > 
> > Nit: maybe avoid duplicating JUMPTABLES_SEC by moving all *_SEC macro
> >      to libbpf_internal.h?
> 
> Yes, ok.
> 
> > >  
> > >  struct src_sec {
> > >  	const char *sec_name;
> > > @@ -2025,6 +2027,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)
> > > @@ -2271,8 +2276,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) != 0) {
> > > +					pr_warn("relocation against STT_SECTION in section %s is not supported!\n",
> > > +						src_sec->sec_name);
> > 
> > Sorry, I missed this on a previous iteration.
> > LLVM generates section relative offsets for jump table contents, so it
> > seems that relocations inside jump table section should not occur.
> > Is this a leftover, or am I confused?
> 
> I think this is a leftover in LLVM, so I have to keep it here.
> I will check again with the latest LLVM.

Yes, the latest LLVM I've built (llvmorg-22-init-12400-g09eea2256e53)
does generate it:

    $ llvm-readelf -S bpf_gotox.bpf.o | grep jumptables
      [ 8] .jumptables       PROGBITS        ...
      [ 9] .rel.jumptables   REL             ...

> > >  					return -EINVAL;
> > >  				}
> > >  			}