Re: [PATCH bpf-next v2 08/11] libbpf: support local function pointer relocation

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Feb 23, 2021 at 10:56 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/23/21 12:03 AM, Andrii Nakryiko wrote:
> > On Wed, Feb 17, 2021 at 12:56 PM Yonghong Song <yhs@fb.com> 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@fb.com>
> >> ---
> >>   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.

I don't really like tailoring libbpf generic SUBPROG_ADDR relocation
to one current specific use case, though. Taking the address of
SUBPROG_ADDR is not, conceptually, tied with passing it to for_each as
a callback. E.g., what if you just want to compare two registers
pointing to subprogs, without actually passing them to for_each(). I
don't know if it's possible right now, but I don't see why that
shouldn't be supported. In the latter case, adding arbitrary
restrictions about static vs global functions doesn't make much sense.

So let's teach libbpf the right logic without assuming any specific
use case. It pays off in the long run.

>
> >
> >> +           (!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.

Yeah, not a big deal, I was under the impression we do that for other
BPF_PSEUDO cases, but checking other parts of libbpf, doesn't seem
like that's the case.

>
> >
> >> +                       /* 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.

given the "generic support" comment above, I think we should still
support this case as well. WDYT?

>
> >
> >> +                               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
> >>