| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHap4zunMHS_FcOeDwmXv38zX0AHgf2v7Hr8bee9fyRcvah=aQ@mail.gmail.com>
Date: Fri, 21 Jan 2022 15:40:54 -0500
From: Mauricio Vásquez Bernal <mauricio@...volk.io>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Quentin Monnet <quentin@...valent.com>,
Rafael David Tinoco <rafaeldtinoco@...il.com>,
Lorenzo Fontana <lorenzo.fontana@...stic.co>,
Leonardo Di Donato <leonardo.didonato@...stic.co>
Subject: Re: [PATCH bpf-next v4 1/8] libbpf: split bpf_core_apply_relo()
On Fri, Jan 14, 2022 at 9:02 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Wed, Jan 12, 2022 at 6:27 AM Mauricio Vásquez <mauricio@...volk.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@...volk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@...asec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@...stic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@...stic.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.
>
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;
> > + }
> > }
> > }
> >
>
> [...]
Powered by blists - more mailing lists