On Thu, Oct 7, 2021 at 3:01 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > [...]
> >
> > 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 for these explanations.It will be helpful to include them in
the commit log.
Song
