Page MenuHomePhabricator

BPF: Add more relocation kinds
ClosedPublic

Authored by yonghong-song on May 18 2021, 12:36 PM.

Details

Summary

Currently, BPF only contains three relocations:

R_BPF_NONE   for no relocation
R_BPF_64_64  for LD_imm64 and normal 64-bit data relocation
R_BPF_64_32  for call insn and normal 32-bit data relocation

Also .BTF and .BTF.ext sections contain symbols in allocated
program and data sections. These two sections reserved 32bit
space to hold the offset relative to the symbol's section.
When LLVM JIT is used, the LLVM ExecutionEngine RuntimeDyld
may attempt to resolve relocations for .BTF and .BTF.ext,
which we want to prevent. So we used R_BPF_NONE for such relocations.

This all works fine until when we try to do linking of
multiple objects.

. R_BPF_64_64 handling of LD_imm64 vs. normal 64-bit data
  is different, so lld target->relocate() needs more context
  to do a correct job.
. The same for R_BPF_64_32. More context is needed for
  lld target->relocate() to differentiate call insn vs.
  normal 32-bit data relocation.
. Since relocations in .BTF and .BTF.ext are set to R_BPF_NONE,
  they will not be relocated properly when multiple .BTF/.BTF.ext
  sections are merged by lld.

This patch intends to address this issue by adding additional
relocation kinds:

R_BPF_64_ABS64          for normal 64-bit data relocation
R_BPF_64_ABS32          for normal 32-bit data relocation
R_BPF_64_NODYLD32  for .BTF and .BTF.ext style relocations.

The old R_BPF_64_{64,32} semantics:

R_BPF_64_64       for LD_imm64 relocation
R_BPF_64_32       for call insn relocation

The existing R_BPF_64_64/R_BPF_64_32 mapping to numeric values
is maintained. They are the most common use cases for
bpf programs and we want to maintain backward compatibility
as much as possible.

ExecutionEngine RuntimeDyld BPF relocations are adjusted as well.
R_BPF_64_{ABS64,ABS32} relocations will be resolved properly and
other relocations will be ignored.
Two tests are added for RuntimeDyld. Not handling R_BPF_64_NODYLD32 in
RuntimeDyldELF.cpp will result in "Relocation type not implemented yet!"
fatal error.

FK_SecRel_4 usages in BPFAsmBackend.cpp and BPFELFObjectWriter.cpp
are removed as they are not triggered in BPF backend.
BPF backend used FK_SecRel_8 for LD_imm64 instruction operands.

Diff Detail

Event Timeline

yonghong-song created this revision.May 18 2021, 12:36 PM
yonghong-song requested review of this revision.May 18 2021, 12:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 12:36 PM

This patch will require bpf-next libbpf patch like below:

diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index ee426226928f..ccc5d4591801 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -28,6 +28,12 @@
 #ifndef R_BPF_64_64
 #define R_BPF_64_64 1
 #endif
+#ifndef R_BPF_64_LD64
+#define R_BPF_64_LD64 2
+#endif
+#ifndef R_BPF_64_PC32
+#define R_BPF_64_PC32 3
+#endif
 #ifndef R_BPF_64_32
 #define R_BPF_64_32 10
 #endif
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index b594a88620ce..3e3cd64025dd 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -892,7 +892,8 @@ static int linker_sanity_check_elf_relos(struct src_obj *obj, struct src_sec *se
                size_t sym_idx = ELF64_R_SYM(relo->r_info);
                size_t sym_type = ELF64_R_TYPE(relo->r_info);
 
-               if (sym_type != R_BPF_64_64 && sym_type != R_BPF_64_32) {
+               if (sym_type != R_BPF_64_64 && sym_type != R_BPF_64_32 &&
+                    sym_type != R_BPF_64_LD64 && sym_type != R_BPF_64_PC32) {
                        pr_warn("ELF relo #%d in section #%zu has unexpected type %zu in %s\n",
                                i, sec->sec_idx, sym_type, obj->filename);
                        return -EINVAL;
yonghong-song edited the summary of this revision. (Show Details)May 18 2021, 12:58 PM
MaskRay added a subscriber: MaskRay.EditedMay 18 2021, 3:33 PM

These ELF flags should be documented somewhere. Even a draft will help.

For example, riscv is using https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#relocations , though not as sophisticated as other psABIs (x86-64,aarch64,ppc64 ELFv2)

MaskRay added inline comments.May 18 2021, 3:34 PM
llvm/test/CodeGen/BPF/reloc-2.ll
2

For REL -r is not sufficient for testing. You should check the implicit addends as well.

These ELF flags should be documented somewhere. Even a draft will help.

For example, riscv is using https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#relocations , though not as sophisticated as other psABIs (x86-64,aarch64,ppc64 ELFv2)

This is a good suggestion. Currently, the main bpf compiler/instruction set related doc is living in the kernel tree

https://www.kernel.org/doc/Documentation/networking/filter.rst

I think that is not a good place to put relocation as kernel people really don't care relocations much and
compiler/tool people may not be interested to check the kernel doc.

We need to find a different place to document some bpf compiler/tool internal stuff. I will add a little bit more
documentation in the code and in the commit message at this time. Will consider to put relocation description (maybe others)
in a public accessible document as a followup.

yonghong-song added inline comments.May 18 2021, 5:36 PM
llvm/test/CodeGen/BPF/reloc-2.ll
2

okay. Will check the instruction as well.

yonghong-song edited the summary of this revision. (Show Details)
  • add more comments and descriptions for relocations.
yonghong-song edited the summary of this revision. (Show Details)
  • removed FK_SecRel_8 and used FK_SecRel_4 for LD_imm64 fixup kind which reflects better that fact only the first 4-byte insn->off is allowed for storing the section offset.
yonghong-song edited the summary of this revision. (Show Details)
yonghong-song edited the summary of this revision. (Show Details)
  • rename R_BPF_64_D{32,64} to R_BPF_64_ABS{32,64} to better reflect the intention of relocation. *ABS{32,64} names have been used by arm, aarch64 and amdgpu.
MaskRay added inline comments.May 20 2021, 7:52 PM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
954

This needs an ExecutionEngine test. Otherwise, don't touch the file in this patch.

llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp
45–46

FK_SecRel_4 is used by PE-COFF to represent SECREL relocation types.

"The 32-bit offset of the target from the beginning of its section. This is used to support debugging information and static thread local storage."

Why does it map to a regular looking R_BPF_64_64?

72

This doesn't make sense to me. The relocation type names shouldn't be aware of SHF_ALLOC/non-SHF_ALLOC.

yonghong-song added inline comments.May 20 2021, 8:17 PM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
954

I will add a test for this.

llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp
45–46

In AMDGPU/MCTargetDesc/R600MCCodeEmitter.cpp, I see

Fixups.push_back(MCFixup::create(offset, MO.getExpr(), FK_SecRel_4, MI.getLoc()));

It looks like backend can choose create a MCFixup with FK_SecRel_4. Do we have restrictions that we have to use a different one? Use FK_SecRel_4 seems a good fit here as only 4 bytes are permitted to write anyway.

72

I just wanted to choose a best-fit name. How about R_BPF_64_NODYLD_32 implying no runtime dynamic linking?

yonghong-song added inline comments.May 20 2021, 8:20 PM
llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp
45–46

I can certainly go back with FK_SecRel_8, but want to understand why I cannot use FK_SecRel_4.

yonghong-song edited the summary of this revision. (Show Details)
yonghong-song added a reviewer: MaskRay.
  • add a test for ExecutionEngine/RuntimeDyld for added BPF relocation case. Without the patch in RuntimeDyldELF.cpp, the test will crash
  • rename R_BPF_64_ALLOC32 to R_BPF_64_NODYLD32.
  • added @MaskRay as the reviewer
  • remaining known issues to be resolved:
    • can BPF backend use FK_SecRel_4 for R_BPF_64_64?
    • Is the relocation name R_BPF_64_NODYLD32 okay or we could have a better one?
MaskRay added inline comments.May 21 2021, 2:59 PM
llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp
45–46

My concern here is that FK_SecRel_* is no overloaded for entirely different semantics. Other targets use different variant kinds for such cases.

yonghong-song added inline comments.May 21 2021, 4:32 PM
llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp
45–46

Ok. Sounds reasonable since they are all related to similar tls use case. I will switch to FK_SecRel_8 in the next version. Thanks!

yonghong-song edited the summary of this revision. (Show Details)
  • use FK_SecRel_8 Fixup kind for LD_imm64 operands instead of FK_SecRel_4.
yonghong-song edited the summary of this revision. (Show Details)
  • enumerate all relocations in RuntimeDyld.c such that R_BPF_64_{ABS64,ABS32} relocations will be resolved and others will be ignored.
  • added one more test case that R_BPF_64_ABS64 is properly resolved.
  • add one test case to generate R_BPF_64_ABS64 relocation for global data.
ast accepted this revision.May 24 2021, 3:37 PM
This revision is now accepted and ready to land.May 24 2021, 3:37 PM
This revision was automatically updated to reflect the committed changes.