Thread (12 messages) 12 messages, 3 authors, 2022-01-12

Re: [PATCH bpf-next v3 1/3] libbpf: split bpf_core_apply_relo()

From: Andrii Nakryiko <hidden>
Date: 2021-12-23 00:33:39
Also in: bpf

On Fri, Dec 17, 2021 at 10:57 AM Mauricio Vásquez [off-list ref] wrote:
quoted hunk ↗ jump to hunk
BTFGen needs to run the core relocation logic in order to understand
what are the types in the target BTF that involved in a given
relocation.

Currently bpf_core_apply_relo() calculates and **applies** a relocation
to an instruction. Having both operations in the same function makes it
difficult to only calculate the relocation without patching the
instruction. This commit splits that logic in two different phases: (1)
calculate the relocation and (2) patch the instruction.

For the first phase bpf_core_apply_relo() is renamed to
bpf_core_calc_relo_res() who is now only on charge of calculating the
relocation, the second phase uses the already existing
bpf_core_patch_insn(). bpf_object__relocate_core() uses both of them and
the BTFGen will use only bpf_core_calc_relo_res().

Signed-off-by: Mauricio Vásquez <redacted>
Signed-off-by: Rafael David Tinoco <redacted>
Signed-off-by: Lorenzo Fontana <redacted>
Signed-off-by: Leonardo Di Donato <redacted>
---
 kernel/bpf/btf.c          | 11 +++++-
 tools/lib/bpf/libbpf.c    | 54 ++++++++++++++++-----------
 tools/lib/bpf/relo_core.c | 77 +++++++++++----------------------------
 tools/lib/bpf/relo_core.h | 42 ++++++++++++++++++---
 4 files changed, 99 insertions(+), 85 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index a17de71abd2e..5a8f6ef6a341 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6734,6 +6734,7 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
 {
        bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
        struct bpf_core_cand_list cands = {};
+       struct bpf_core_relo_res targ_res;
        struct bpf_core_spec *specs;
        int err;
@@ -6778,8 +6779,14 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
                 */
        }

-       err = bpf_core_apply_relo_insn((void *)ctx->log, insn, relo->insn_off / 8,
-                                      relo, relo_idx, ctx->btf, &cands, specs);
+       err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs,
+                                     &targ_res);
+       if (err)
+               goto out;
+
+       err = bpf_core_patch_insn((void *)ctx->log, insn, relo->insn_off / 8, relo, relo_idx,
+                                 &targ_res);
+
 out:
        kfree(specs);
        if (need_cands) {
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index cf862a19222b..77e2df13715a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5498,11 +5498,12 @@ static int record_relo_core(struct bpf_program *prog,
        return 0;
 }

-static int bpf_core_apply_relo(struct bpf_program *prog,
-                              const struct bpf_core_relo *relo,
-                              int relo_idx,
-                              const struct btf *local_btf,
-                              struct hashmap *cand_cache)
+static int bpf_core_calc_relo_res(struct bpf_program *prog,
bpf_core_calc_relo_res is almost indistinguishable from
bpf_core_calc_relo... Let's call this one bpf_core_resolve_relo()?
+                                 const struct bpf_core_relo *relo,
+                                 int relo_idx,
+                                 const struct btf *local_btf,
+                                 struct hashmap *cand_cache,
+                                 struct bpf_core_relo_res *targ_res)
[...]
quoted hunk ↗ jump to hunk
 static int
 bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
 {
        const struct btf_ext_info_sec *sec;
+       struct bpf_core_relo_res targ_res;
        const struct bpf_core_relo *rec;
        const struct btf_ext_info *seg;
        struct hashmap_entry *entry;
        struct hashmap *cand_cache = NULL;
        struct bpf_program *prog;
+       struct bpf_insn *insn;
        const char *sec_name;
        int i, err = 0, insn_idx, sec_idx;
@@ -5636,12 +5627,31 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
                        if (!prog->load)
                                continue;

-                       err = bpf_core_apply_relo(prog, rec, i, obj->btf, cand_cache);
+                       err = bpf_core_calc_relo_res(prog, rec, i, obj->btf, cand_cache, &targ_res);
                        if (err) {
                                pr_warn("prog '%s': relo #%d: failed to relocate: %d\n",
                                        prog->name, i, err);
                                goto out;
                        }
+
+                       if (rec->insn_off % BPF_INSN_SZ)
+                               return -EINVAL;
+                       insn_idx = rec->insn_off / BPF_INSN_SZ;
+                       /* adjust insn_idx from section frame of reference to the local
+                        * program's frame of reference; (sub-)program code is not yet
+                        * relocated, so it's enough to just subtract in-section offset
+                        */
+                       insn_idx = insn_idx - prog->sec_insn_off;
+                       if (insn_idx >= prog->insns_cnt)
+                               return -EINVAL;
+                       insn = &prog->insns[insn_idx];
this is sort of like sanity checks, let's do them before the core_calc
step, so after that it's a clean sequence of calc_relo + pathc_insn?
+
+                       err = bpf_core_patch_insn(prog->name, insn, insn_idx, rec, i, &targ_res);
+                       if (err) {
+                               pr_warn("prog '%s': relo #%d: failed to patch insn #%u: %d\n",
+                                       prog->name, i, insn_idx, err);
+                               goto out;
+                       }
                }
        }
[...]
quoted hunk ↗ jump to hunk
 {
        __u32 orig_val, new_val;
        __u8 class;
@@ -1177,18 +1152,18 @@ static void bpf_core_dump_spec(const char *prog_name, int level, const struct bp
  *    between multiple relocations for the same type ID and is updated as some
  *    of the candidates are pruned due to structural incompatibility.
  */
-int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
-                            int insn_idx,
-                            const struct bpf_core_relo *relo,
-                            int relo_idx,
-                            const struct btf *local_btf,
-                            struct bpf_core_cand_list *cands,
-                            struct bpf_core_spec *specs_scratch)
+int bpf_core_calc_relo_insn(const char *prog_name,
please update the comment for this function, it's not "CO-RE relocate
single instruction" anymore, it's more like "Calculate CO-RE
relocation target result" or something along those lines.
quoted hunk ↗ jump to hunk
+                           const struct bpf_core_relo *relo,
+                           int relo_idx,
+                           const struct btf *local_btf,
+                           struct bpf_core_cand_list *cands,
+                           struct bpf_core_spec *specs_scratch,
+                           struct bpf_core_relo_res *targ_res)
 {
        struct bpf_core_spec *local_spec = &specs_scratch[0];
        struct bpf_core_spec *cand_spec = &specs_scratch[1];
        struct bpf_core_spec *targ_spec = &specs_scratch[2];
-       struct bpf_core_relo_res cand_res, targ_res;
+       struct bpf_core_relo_res cand_res;
        const struct btf_type *local_type;
        const char *local_name;
        __u32 local_id;
@@ -1223,12 +1198,12 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
        /* TYPE_ID_LOCAL relo is special and doesn't need candidate search */
        if (relo->kind == BPF_CORE_TYPE_ID_LOCAL) {
                /* bpf_insn's imm value could get out of sync during linking */
-               memset(&targ_res, 0, sizeof(targ_res));
-               targ_res.validate = false;
-               targ_res.poison = false;
-               targ_res.orig_val = local_spec->root_type_id;
-               targ_res.new_val = local_spec->root_type_id;
-               goto patch_insn;
+               memset(targ_res, 0, sizeof(*targ_res));
+               targ_res->validate = true;
hm.. original code sets it to false here, please don't regress the logic
+               targ_res->poison = false;
+               targ_res->orig_val = local_spec->root_type_id;
+               targ_res->new_val = local_spec->root_type_id;
+               return 0;
        }

        /* libbpf doesn't support candidate search for anonymous types */
[...]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help