On Mon, Feb 8, 2021 at 10:56 PM Yonghong Song [off-list ref] wrote:

On 2/8/21 10:52 AM, Andrii Nakryiko wrote:

quoted

On Thu, Feb 4, 2021 at 5:54 PM Yonghong Song [off-list ref] wrote:

quoted

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 <redacted>
---
  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.

quoted

quoted

  };

  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.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

                 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.

quoted

quoted

                 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.

quoted

quoted

                 } else {
                         /* if subprogram call is to a static function within
                          * the same ELF section, there won't be any relocation

don'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.

Hm.. is it handled differently from calls then? When there is a call
from one static subprog to another static subprog, there is no
relocation (because the compiler can guarantee that both functions are
in the same section and their relative location won't change,
presumably). Why is this not the case for getting the address of a
function here?

Regardless, if this can't happen, then let's check and error out,
instead of silently producing invalid values.

quoted

quoted

--
2.24.1