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.
> + (!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.
> + !(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;
}
> 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?
> + /* 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?
> + 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
>
