On 25/08/21 11:14AM, Andrii Nakryiko wrote:
> On Thu, Aug 21, 2025 at 6:00 AM Anton Protopopov
> <a.s.protopopov@xxxxxxxxx> wrote:
> >
> > On 25/08/20 05:20PM, Andrii Nakryiko wrote:
> > > On Sat, Aug 16, 2025 at 11:02 AM Anton Protopopov
> > > <a.s.protopopov@xxxxxxxxx> wrote:
> > > >
> > > > For v5 instruction set, LLVM now 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.
> > > >
> > > > The code generated by LLVM for a switch will look, approximately,
> > > > like this:
> > > >
> > > > 0: rX <- jump_table_x[i]
> > > > 2: rX <<= 3
> > > > 3: gotox *rX
> > > >
> > > > Right now there is no robust way to associate the jump with the
> > > > corresponding map, so libbpf doesn't insert map file descriptor
> > > > inside the gotox instruction.
> > >
> > > Just from the commit description it's not clear whether that's
> > > something that needs fixing or is OK? If it's OK, why call it out?..
> >
> > Right, will rephrase.
> >
> > The idea here is that if you have, say, a switch, then, most
> > probably, it is compiled into 1 jump table and 1 gotox. And, if
> > compiler can provide enough metadata, then this makes sense for
> > libbpf to also associate JT with gotox by inserting the same map
> > descriptor inside both instructions. However now this doesn't
> > work, and also there are cases when one gotox can be associated with
> > multiple JTs.
>
> Ok, and right now we'll basically generate two identical BPF maps? If
> we wanted to optimize this, wouldn't it be sufficient to just reuse
> maps if relocation points to the same symbol?
No, right now the gotox doesn't contain a map, only ldimm64. In
check_cfg when the verifier encounters a gotox instruction it finds
all the potential jump tables for that subprog. In the later stage
for a `gotox Rx` the verifier knows the exact map from which Rx was
loaded, and can verify precisely.
> > > > Signed-off-by: Anton Protopopov <a.s.protopopov@xxxxxxxxx>
> > > > ---
> > > > .../bpf/bpftool/Documentation/bpftool-map.rst | 2 +-
> > > > tools/bpf/bpftool/map.c | 2 +-
> > > > tools/lib/bpf/libbpf.c | 159 +++++++++++++++---
> > > > tools/lib/bpf/libbpf_probes.c | 4 +
> > > > tools/lib/bpf/linker.c | 12 +-
> > > > 5 files changed, 153 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> > > > index 252e4c538edb..3377d4a01c62 100644
> > > > --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
> > > > +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> > > > @@ -55,7 +55,7 @@ MAP COMMANDS
> > > > | | **devmap** | **devmap_hash** | **sockmap** | **cpumap** | **xskmap** | **sockhash**
> > > > | | **cgroup_storage** | **reuseport_sockarray** | **percpu_cgroup_storage**
> > > > | | **queue** | **stack** | **sk_storage** | **struct_ops** | **ringbuf** | **inode_storage**
> > > > -| | **task_storage** | **bloom_filter** | **user_ringbuf** | **cgrp_storage** | **arena** }
> > > > +| | **task_storage** | **bloom_filter** | **user_ringbuf** | **cgrp_storage** | **arena** | **insn_array** }
> > > >
> > > > DESCRIPTION
> > > > ===========
> > > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > > index c9de44a45778..79b90f274bef 100644
> > > > --- a/tools/bpf/bpftool/map.c
> > > > +++ b/tools/bpf/bpftool/map.c
> > > > @@ -1477,7 +1477,7 @@ static int do_help(int argc, char **argv)
> > > > " devmap | devmap_hash | sockmap | cpumap | xskmap | sockhash |\n"
> > > > " cgroup_storage | reuseport_sockarray | percpu_cgroup_storage |\n"
> > > > " queue | stack | sk_storage | struct_ops | ringbuf | inode_storage |\n"
> > > > - " task_storage | bloom_filter | user_ringbuf | cgrp_storage | arena }\n"
> > > > + " task_storage | bloom_filter | user_ringbuf | cgrp_storage | arena | insn_array }\n"
> > > > " " HELP_SPEC_OPTIONS " |\n"
> > > > " {-f|--bpffs} | {-n|--nomount} }\n"
> > > > "",
> > >
> > > bpftool changes sifted through into libbpf patch?
> >
> > Yes thanks. I think I've sqhashed the fix here, becase it broke
> > the `test_progs -a libbpf_str` test.
> >
>
> libbpf_str test doesn't rely on bpftool, so fixing up selftest in the
> same patch makes sense (to not break bisection), but bpftool changes
> still make no change and should be done separately
Yes, seems that you're right. I think I also was fixing the
./test_bpftool.py and squashed similar changes into the libbpf
commit. I will check and split before resending.
> [...]
>
> > >
> > > > +
> > > > + return -prog->sec_insn_off;
> > >
> > > why this return value?... can you elaborate?
> >
> > Jump tables generated by LLVM contain offsets relative to the
> > beginning of a section. The offsets inside a BPF_INSN_ARRAY
> > are absolute (for a "load unit", i.e., insns in bpf_prog_load).
> > So if, say, a section A contains two progs, f1 and f2, then,
> > f1 starts at 0 and f2 at F2_START. So when the f2 is loaded
> > jump tables needs to be adjusted by -F2_START such that offsets
> > are correct.
>
> the thing I missed is that this isn't some sort of error condition,
> it's just when offset falls into main program function
>
> naming is also a bit misleading, IMO because it doesn't just return
> instruction offset, but rather an *adjustment* to an offset in jump
> table
Yeah, and I think it is even named appropriately in the call site.
I will check how to make this more transparent for the reader.
> [...]
>
> > > where does .rel.rodata come from?
> > >
> > > and we don't need to adjust the contents of any of those sections, right?...
> > >
> > > can you please add some tests validating that two object files with
> > > jumptables can be linked together and end up with proper combined
> > > .jumptables section?
> > >
> > >
> > > and in terms of code, can we do
> > >
> > > } else if (strcmp(..., JUMPTABLES_REL_SEC) == 0) {
> > > /* nothing to do for .rel.jumptables */
> > > } else {
> > > pr_warn(...);
> > > }
> > >
> > > It makes it more apparent what is supported and what's not.
> >
> > Yes, sure. The rodata might be obsolete, I will check, and
> > .rel.jumptables is actually not used. This should be cleaned up
> > once LLVM patch stabilizes. Thanks for noticing this,
> > this way it is for sure added to my checklist :-)
> >
>
> ok, thanks
>
> > >
> > > > + pr_warn("relocation against STT_SECTION in section %s is not supported!\n",
> > > > + src_sec->sec_name);
> > > > return -EINVAL;
> > > > }
> > > > }
> > > > --
> > > > 2.34.1
> > > >
