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

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

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.

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