On Oct 14, 2021, at 10:53 AM, Kumar Kartikeya Dwivedi [off-list ref] wrote:

On Thu, Oct 14, 2021 at 10:09:43PM IST, Song Liu wrote:

quoted

quoted

On Oct 13, 2021, at 12:33 AM, Kumar Kartikeya Dwivedi [off-list ref] wrote:

This uses the bpf_kallsyms_lookup_name helper added in previous patches
to relocate typeless ksyms. The return value ENOENT can be ignored, and
the value written to 'res' can be directly stored to the insn, as it is
overwritten to 0 on lookup failure. For repeating symbols, we can simply
copy the previously populated bpf_insn.

Also, we need to take care to not close fds for typeless ksym_desc, so
reuse the 'off' member's space to add a marker for typeless ksym and use
that to skip them in cleanup_relos.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

[...]

quoted

}

+/* Expects:
+ * BPF_REG_8 - pointer to instruction
+ */
+static void emit_relo_ksym_typeless(struct bpf_gen *gen,
+				    struct ksym_relo_desc *relo, int insn)

This function has quite some duplicated logic as emit_relo_ksym_btf().
I guess we can somehow reuse the code here. Say, we pull changes from
3/8 first to handle weak type. Then we extend the function to handle
typeless. Would this work?

Ok, will put both into the same function in the next version. Though the part
between:

quoted

quoted

+{
+	struct ksym_desc *kdesc;
+
+	kdesc = get_ksym_desc(gen, relo);
+	if (!kdesc)
+		return;
+	/* try to copy from existing ldimm64 insn */
+	if (kdesc->ref > 1) {
+		move_blob2blob(gen, insn + offsetof(struct bpf_insn, imm), 4,
+			       kdesc->insn + offsetof(struct bpf_insn, imm));
+		move_blob2blob(gen, insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 4,
+			       kdesc->insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm));

this and ...

quoted

quoted

+		goto log;
+	}
+	/* remember insn offset, so we can copy ksym addr later */
+	kdesc->insn = insn;
+	/* skip typeless ksym_desc in fd closing loop in cleanup_relos */
+	kdesc->typeless = true;
+	emit_bpf_kallsyms_lookup_name(gen, relo);
+	emit(gen, BPF_JMP_IMM(BPF_JEQ, BPF_REG_7, -ENOENT, 1));
+	emit_check_err(gen);
+	/* store lower half of addr into insn[insn_idx].imm */
+	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_9, offsetof(struct bpf_insn, imm)));
+	/* store upper half of addr into insn[insn_idx + 1].imm */
+	emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_9, 32));
+	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_9,
+		      sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)));

... this won't overlap (so it will have to jump into the else branch to clear_src_reg).

e.g. it looks something like this:

if (kdesc->ref > 1) {
	move...
	if (!relo->is_typeless)
		...
		goto clear_src_reg;
}
kdesc->insn = insn;
...
if (relo->is_typeless) {
	...
} else {
	...
clear_src_reg:
	...
}

so it looked better to split into separate functions (maybe we can just move the
logging part to common helper? the rest is just duplicating the inital get and
move_blob2blob).

Yeah, I can tell it not very clean either way. A common helper might be the best 
option here. 

Thanks,
Song