On 2/8/21 10:52 AM, Andrii Nakryiko wrote:
On Thu, Feb 4, 2021 at 5:54 PM Yonghong Song <yhs@xxxxxx> wrote:A new relocation RELO_LOCAL_FUNC 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 | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 2abbc3800568..a5146c9e3e06 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_LOCAL_FUNC,libbpf internally is using SUBPROG notation. I think "LOCAL" part is confusing, so I'd drop it. How about just RELO_SUBPROG? We can separately refactor these names to distinguish RELO_CALL from the new one. It would be more clear if RELO_CALL was called RELO_SUBPROG_CALL, and the new one either RELO_SUBPROG_ADDR or RELO_SUBPROG_REF (as in subprog reference)
Yes, we can use RELO_SUBPROG_ADDR.
}; struct reloc_desc { @@ -574,6 +575,12 @@ static bool insn_is_subprog_call(const struct bpf_insn *insn) insn->off == 0; } +static bool insn_is_pseudo_func(const struct bpf_insn *insn) +{ + return insn->code == (BPF_LD | BPF_IMM | BPF_DW) &&there is is_ldimm64() function for this check (just move it up here, it's a single-liner)
I did not know it. Will use in the next revision.
+ 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, @@ -3395,6 +3402,16 @@ static int bpf_program__record_reloc(struct bpf_program *prog, return 0; } + if (insn->code == (BPF_LD | BPF_IMM | BPF_DW) &&just move this check below the next if that checks !is_ldimm64, no need to do it here early.
Okay.
+ GELF_ST_BIND(sym->st_info) == STB_LOCAL && + GELF_ST_TYPE(sym->st_info) == STT_SECTION && + shdr_idx == obj->efile.text_shndx) {see above how RELO_CALL is handled: shdr_idx != 0 check is missing. We also validate that sym->st_value is multiple of BPF_INSN_SZ.
Okay. Will add additional checking.
+ reloc_desc->type = RELO_LOCAL_FUNC; + reloc_desc->insn_idx = insn_idx; + reloc_desc->sym_off = sym->st_value; + return 0; + } + if (insn->code != (BPF_LD | BPF_IMM | BPF_DW)) {feel free to use is_ldimm64 here as well, thanks!pr_warn("prog '%s': invalid relo against '%s' for insns[%d].code 0x%x\n", prog->name, sym_name, insn_idx, insn->code); @@ -6172,6 +6189,9 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog) } relo->processed = true; break; + case RELO_LOCAL_FUNC: + insn[0].src_reg = BPF_PSEUDO_FUNC; + /* fallthrough */fallthrough into an empty break clause seems a bit weird... just break and leave the same comment as below?
Yes, "break" seems cleaner.
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_LOCAL_FUNC) { 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,15 @@ 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. */ - sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1; + 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;nit: keep it on a single line, it still fits within 100 characters and is easier to visually compare to RELO_CALL case.
Okay.
} else { /* if subprogram call is to a static function within * the same ELF section, there won't be any relocationdon't we have to adjust insn->imm for this case as well? Let's add selftests to make sure this works.
This is for relo == NULL. I think my code (RELO_LOCAL_FUNC or RELO_SUBPROG_ADDR) won't hit this since there always relocations. That is why I didn't do anything here.
-- 2.24.1
