Re: [PATCH bpf-next v4 1/8] libbpf: split bpf_core_apply_relo()
flat view
From: Andrii Nakryiko <hidden>
Date: 2022-01-15 02:02:04
Also in:
bpf
On Wed, Jan 12, 2022 at 6:27 AM Mauricio Vásquez [off-list ref] wrote:
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
outdated name?
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().
same? BTW, this patch set breaks CI ([0]), please investigate [0] https://github.com/kernel-patches/bpf/runs/4797721812?check_suite_focus=true
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 | 13 ++++-- tools/lib/bpf/libbpf.c | 84 ++++++++++++++++++++++++--------------- tools/lib/bpf/relo_core.c | 79 +++++++++++------------------------- tools/lib/bpf/relo_core.h | 42 +++++++++++++++++--- 4 files changed, 122 insertions(+), 96 deletions(-)
[...]
quoted hunk ↗ jump to hunk
@@ -5661,12 +5642,53 @@ 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); + if (prog->obj->gen_loader) { + const struct btf_type *local_type; + const char *local_name, *spec_str; + + spec_str = btf__name_by_offset(obj->btf, rec->access_str_off); + if (!spec_str) + return -EINVAL; + + local_type = btf__type_by_id(obj->btf, rec->type_id); + if (!local_type) + return -EINVAL; + + local_name = btf__name_by_offset(obj->btf, local_type->name_off); + if (!local_name) + return -EINVAL; + + pr_debug("record_relo_core: prog %td insn[%d] %s %s %s final insn_idx %d\n", + prog - prog->obj->programs, insn_idx, + btf_kind_str(local_type), local_name, spec_str, insn_idx);
hmm, maybe let's just drop this pr_debug instead? that's a lot of code and checks just to emit this debug info.
quoted hunk ↗ jump to hunk
+ return record_relo_core(prog, rec, insn_idx); + } + + 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 validation probably is better to do before prog->obj->gen_loader check so that we don't silently do something bad in record_relo_core() if insn_idx is wrong? It doesn't change the rest of the logic, right? So there shouldn't be any harm or change of behavior.
quoted hunk ↗ jump to hunk
+ err = bpf_core_resolve_relo(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; } + + 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; + } } }
[...]