On Thu, Oct 7, 2021 at 3:01 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

On Fri, Oct 08, 2021 at 03:15:10AM IST, Song Liu wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

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@gmail.com>

[...]

Everything above (trimmed) makes sense to me.

quoted

+/* 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