On 2/23/21 12:03 AM, Andrii Nakryiko wrote:
On Wed, Feb 17, 2021 at 12:56 PM Yonghong Song <yhs@xxxxxx> wrote:
A new relocation RELO_SUBPROG_ADDR is added to capture
local (static) function pointers loaded with ld_imm64
insns. Such ld_imm64 insns are marked with
BPF_PSEUDO_FUNC and will be passed to kernel so
kernel can replace them with proper actual jited
func addresses.
Signed-off-by: Yonghong Song <yhs@xxxxxx>
---
tools/lib/bpf/libbpf.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 21a3eedf070d..772c7455f1a2 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -188,6 +188,7 @@ enum reloc_type {
RELO_CALL,
RELO_DATA,
RELO_EXTERN,
+ RELO_SUBPROG_ADDR,
};
struct reloc_desc {
@@ -579,6 +580,11 @@ static bool is_ldimm64(struct bpf_insn *insn)
return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
}
+static bool insn_is_pseudo_func(struct bpf_insn *insn)
+{
+ return is_ldimm64(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
+}
+
static int
bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog,
const char *name, size_t sec_idx, const char *sec_name,
@@ -3406,6 +3412,16 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
return -LIBBPF_ERRNO__RELOC;
}
+ if (GELF_ST_BIND(sym->st_info) == STB_LOCAL &&
+ GELF_ST_TYPE(sym->st_info) == STT_SECTION &&
STB_LOCAL + STT_SECTION is a section symbol. But STT_FUNC symbol could
be referenced as well, no? So this is too strict.
Yes, STT_FUNC symbol could be referenced but we do not have use
case yet. If we encode STT_FUNC (global), the kernel will reject
it. We can extend libbpf to support STT_FUNC once we got a use
case.
+ (!shdr_idx || shdr_idx == obj->efile.text_shndx) &&
this doesn't look right, shdr_idx == 0 is a bad condition and should
be rejected, not accepted.
it is my fault. Will fix in the next revision.
+ !(sym->st_value % BPF_INSN_SZ)) {
+ reloc_desc->type = RELO_SUBPROG_ADDR;
+ reloc_desc->insn_idx = insn_idx;
+ reloc_desc->sym_off = sym->st_value;
+ return 0;
+ }
+
So see code right after sym_is_extern(sym) check. It checks for valid
shrd_idx, which is good and would be good to use that. After that we
can assume shdr_idx is valid and we can make a simple
obj->efile.text_shndx check then and use that as a signal that this is
SUBPROG_ADDR relocation (instead of deducing that from STT_SECTION).
And !(sym->st_value % BPF_INSN_SZ) should be reported as an error, not
silently skipped. Again, just how BPF_JMP | BPF_CALL does it. That way
it's more user-friendly, if something goes wrong. So it will look like
this:
if (shdr_idx == obj->efile.text_shndx) {
/* check sym->st_value, pr_warn(), return error */
reloc_desc->type = RELO_SUBPROG_ADDR;
...
return 0;
}
Okay. Will do similar checking to insn->code == (BPF_JMP | BPF_CALL)
in the next revision.
if (sym_is_extern(sym)) {
int sym_idx = GELF_R_SYM(rel->r_info);
int i, n = obj->nr_extern;
@@ -6172,6 +6188,10 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
}
relo->processed = true;
break;
+ case RELO_SUBPROG_ADDR:
+ insn[0].src_reg = BPF_PSEUDO_FUNC;
BTW, doesn't Clang emit instruction with BPF_PSEUDO_FUNC set properly
already? If not, why not?
This is really a contract between libbpf and kernel, similar to
BPF_PSEUDO_MAP_FD/BPF_PSEUDO_MAP_VALUE/BPF_PSEUDO_BTF_ID.
Adding encoding in clang is not needed as this is simply a load
of function address as far as clang concerned.
+ /* will be handled as a follow up pass */
+ break;
case RELO_CALL:
/* will be handled as a follow up pass */
break;
@@ -6358,11 +6378,11 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog,
for (insn_idx = 0; insn_idx < prog->sec_insn_cnt; insn_idx++) {
insn = &main_prog->insns[prog->sub_insn_off + insn_idx];
- if (!insn_is_subprog_call(insn))
+ if (!insn_is_subprog_call(insn) && !insn_is_pseudo_func(insn))
continue;
relo = find_prog_insn_relo(prog, insn_idx);
- if (relo && relo->type != RELO_CALL) {
+ if (relo && relo->type != RELO_CALL && relo->type != RELO_SUBPROG_ADDR) {
pr_warn("prog '%s': unexpected relo for insn #%zu, type %d\n",
prog->name, insn_idx, relo->type);
return -LIBBPF_ERRNO__RELOC;
@@ -6374,8 +6394,22 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog,
* call always has imm = -1, but for static functions
* relocation is against STT_SECTION and insn->imm
* points to a start of a static function
+ *
+ * for local func relocation, the imm field encodes
+ * the byte offset in the corresponding section.
+ */
+ if (relo->type == RELO_CALL)
+ sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1;
+ else
+ sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm / BPF_INSN_SZ + 1;
+ } else if (insn_is_pseudo_func(insn)) {
+ /*
+ * RELO_SUBPROG_ADDR relo is always emitted even if both
+ * functions are in the same section, so it shouldn't reach here.
*/
- sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1;
+ pr_warn("prog '%s': missing relo for insn #%zu, type %d\n",
nit: "missing subprog addr relo" to make it clearer?
sure. will do.
+ prog->name, insn_idx, relo->type);
+ return -LIBBPF_ERRNO__RELOC;
} else {
/* if subprogram call is to a static function within
* the same ELF section, there won't be any relocation
--
2.24.1
