On Fri, Jan 14, 2022 at 9:02 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, Jan 12, 2022 at 6:27 AM Mauricio Vásquez <mauricio@xxxxxxxxxx> 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 <mauricio@xxxxxxxxxx> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@xxxxxxxxxxx> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@xxxxxxxxxx> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@xxxxxxxxxx> > > --- > > 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(-) > > > > [...] > > > @@ -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. > Makes sense. > > + return record_relo_core(prog, rec, insn_idx); This was the reason why the CI was failing. This should not "return" but "continue" here, otherwise only a single relocation will be recorded. > > + } > > + > > + 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. > I agree. > > + 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; > > + } > > } > > } > > > > [...]
