Re: [PATCH v4 bpf-next 3/9] bpf: Add gen_epilogue to bpf_verifier_ops

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/28/24 7:26 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This patch adds a .gen_epilogue to the bpf_verifier_ops. It is similar
>> to the existing .gen_prologue. Instead of allowing a subsystem
>> to run code at the beginning of a bpf prog, it allows the subsystem
>> to run code just before the bpf prog exit.
>>
>> One of the use case is to allow the upcoming bpf qdisc to ensure that
>> the skb->dev is the same as the qdisc->dev_queue->dev. The bpf qdisc
>> struct_ops implementation could either fix it up or drop the skb.
>> Another use case could be in bpf_tcp_ca.c to enforce snd_cwnd
>> has sane value (e.g. non zero).
>>
>> The epilogue can do the useful thing (like checking skb->dev) if it
>> can access the bpf prog's ctx. Unlike prologue, r1 may not hold the
>> ctx pointer. This patch saves the r1 in the stack if the .gen_epilogue
>> has returned some instructions in the "epilogue_buf".
>>
>> The existing .gen_prologue is done in convert_ctx_accesses().
>> The new .gen_epilogue is done in the convert_ctx_accesses() also.
>> When it sees the (BPF_JMP | BPF_EXIT) instruction, it will be patched
>> with the earlier generated "epilogue_buf". The epilogue patching is
>> only done for the main prog.
>>
>> Only one epilogue will be patched to the main program. When the
>> bpf prog has multiple BPF_EXIT instructions, a BPF_JA is used
>> to goto the earlier patched epilogue. Majority of the archs
>> support (BPF_JMP32 | BPF_JA): x86, arm, s390, risv64, loongarch,
>> powerpc and arc. This patch keeps it simple and always
>> use (BPF_JMP32 | BPF_JA).
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> @@ -19740,6 +19764,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>   			insn->code = BPF_STX | BPF_PROBE_ATOMIC | BPF_SIZE(insn->code);
>>   			env->prog->aux->num_exentries++;
>>   			continue;
>> +		} else if (insn->code == (BPF_JMP | BPF_EXIT) &&
>> +			   epilogue_cnt &&
>> +			   i + delta < subprogs[1].start) {
>> +			/* Generate epilogue for the main prog */
>> +			if (epilogue_idx) {
>> +				/* jump back to the earlier generated epilogue */
>> +				insn_buf[0] = BPF_JMP32_IMM(BPF_JA, 0,
>> +							    epilogue_idx - i - delta - 1, 0);
> 
> Nit: maybe add BPF_GOTOL macro or mention that this is a 'gotol' instruction in the comment?
>       (this is how it is called in llvm).

sgtm.

There is a BPF_JMP_A(OFF). BPF_JMP32_A(IMM) probably will be more consistent 
with the other existing macros.