Re: [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader

From: Song Liu <songliubraving@fb.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin Lau" <kafai@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader
Date: Thu, 14 Oct 2021 19:12:43 +0000	[thread overview]
Message-ID: <E1888EEB-5059-4CFF-AE27-E4F5DDEB7581@fb.com> (raw)
In-Reply-To: <20211014175341.eitbn6ujf4zjkrs7@apollo.localdomain>

> On Oct 14, 2021, at 10:53 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> 
> On Thu, Oct 14, 2021 at 10:09:43PM IST, Song Liu wrote:
>> 
>> 
>>> On Oct 13, 2021, at 12:33 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> 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>
>> [...]
>>> }
>>> 
>>> +/* 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:
> 
>>> +{
>>> +	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 ...
> 
>>> +		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