Re: [PATCH bpf-next v4 1/8] libbpf: split bpf_core_apply_relo()

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

On Wed, Jan 12, 2022 at 6:27 AM Mauricio Vásquez <mauricio@kinvolk.io> 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@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  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.

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

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

[...]