netdev - Re: [PATCH bpf-next v1 2/6] libbpf: Add typeless and weak ksym support to gen

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211007220114.igqbftwpox3batj2@apollo.localdomain>
Date:   Fri, 8 Oct 2021 03:31:14 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Song Liu <song@...nel.org>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v1 2/6] libbpf: Add typeless and weak ksym
 support to gen_loader

On Fri, Oct 08, 2021 at 03:15:10AM IST, Song Liu wrote:
> On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@...il.com> wrote:
> >
> > This patch adds typeless and weak ksym support to BTF_KIND_VAR
> > relocation code in gen_loader. For typeless ksym, we use the newly added
> > bpf_kallsyms_lookup_name helper.
> >
> > For weak ksym, we simply skip error check, and fix up the srg_reg for
> > the insn, as keeping it as BPF_PSEUDO_BTF_ID for weak ksym with its
> > insn[0].imm and insn[1].imm set as 0 will cause a failure.  This is
> > consistent with how libbpf relocates these two cases of BTF_KIND_VAR.
> >
> > We also modify cleanup_relos to check for typeless ksyms in fd closing
> > loop, since those have no fd associated with the ksym. For this we can
> > reuse the unused 'off' member of ksym_desc.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> [...]
>
> Everything above (trimmed) makes sense to me.
>
> > +/* Expects:
> > + * BPF_REG_8 - pointer to instruction
> > + */
> > +static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn)
> > +{
>
> But I don't quite follow why we need these changes to emit_relo_ksym_btf.
> Maybe we should have these changes in a separate patch and add some
> more explanations?
>

Before, if the bpf_btf_find_by_name_kind call failed, we just bailed out due to
the emit_check_err. Now, if it is weak, the error check is conditional, so
we set 0 as the default values and skip the store for btf_id and btf_fd if the
btf lookup failed. Till here, it is similar to the case for emit_relo_kfunc_btf.

Note that we only reach this path once for each unique symbol: the next time, we
enter the kdesc->ref > 1 branch, which copies from the existing insn.

Regarding src_reg stuff: in bpf_object__relocate_data, for obj->gen_loader,
ext->is_set is always true. For the normal libbpf case, it is only true if the
lookup succeeded for BTF (in bpf_object__resolve_ksym_var_btf_id). So depending
on if ext->is_set, it skips assigning BPF_PSEUDO_BTF_ID to src_reg and zeroes
out insn[0].imm and insn[1].imm. Also, the case for ext->is_set = false for
libbpf is only reached if we don't fail on lookup error, and that depends on
ext->is_weak. TLDR; ext->is_weak and lookup failure means src_reg is not
assigned.

For gen_loader, since this src_reg assignment is always there, we need to clear
it for the case where lookup failed, hence the:
-log:
+	emit(gen, BPF_JMP_IMM(BPF_JSGE, BPF_REG_7, 0, 3));

otherwise we end up with src_reg = BPF_PSEUDO_BTF_ID, imm[0] = 0, imm[1] = 0,
which ends up failing the load.

Similarly, we jump over the src_reg adjustment from the kdesc->ref > 1 case if
imm is not equal to 0 (if it were 0, then this is weak ksym). Error check
ensures this instruction is only reached if relo->is_weak (for the same symbol),
so we don't need to check it again there.

Doing it the other way around (not assigning BPF_PSEUDO_BTF_ID by default for
gen_loader) would still involve writing to it in the success case, so IMO
touching it seems unavoidable. If there are better ideas, please lmk.

I added the debug statements so that the selftest reloc result can be inspected
easily, but not sure I can/should verify it from the selftest itself.  I'll
split typeless and weak ksym support into separate patches next time, and
explain this in the commit message.

> Thanks,
> Song
>
> [...]

--
Kartikeya