Re: [RFC PATCH bpf-next 1/6] bpf: Add gen_epilogue to bpf_verifier_ops

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

On 8/14/24 1:56 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-13 at 11:49 -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.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
> 
> Apart from the note below I don't see any obvious problems with this code.
> 
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19610,15 +19610,37 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>>    */
>>   static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>   {
>> +	struct bpf_subprog_info *subprogs = env->subprog_info;
>>   	const struct bpf_verifier_ops *ops = env->ops;
>> -	int i, cnt, size, ctx_field_size, delta = 0;
>> +	int i, cnt, size, ctx_field_size, delta = 0, epilogue_cnt = 0;
>>   	const int insn_cnt = env->prog->len;
>> -	struct bpf_insn insn_buf[16], *insn;
>> +	struct bpf_insn insn_buf[16], epilogue_buf[16], *insn;
>>   	u32 target_size, size_default, off;
>>   	struct bpf_prog *new_prog;
>>   	enum bpf_access_type type;
>>   	bool is_narrower_load;
>>   
>> +	if (ops->gen_epilogue) {
>> +		epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
>> +						 -(subprogs[0].stack_depth + 8));
>> +		if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
>> +			verbose(env, "bpf verifier is misconfigured\n");
>> +			return -EINVAL;
>> +		} else if (epilogue_cnt) {
>> +			/* Save the ARG_PTR_TO_CTX for the epilogue to use */
>> +			cnt = 0;
>> +			subprogs[0].stack_depth += 8;
> 
> Note: two other places that allocate additional stack
>        (optimize_bpf_loop(), do_misc_fixups())
>        also bump 'env->prog->aux->stack_depth'.

Thanks for pointing it out.

In this case, I will stay with calling .gen_epilogue in convert_ctx_accesses().

> 
>> +			insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_1,
>> +						      -subprogs[0].stack_depth);
>> +			insn_buf[cnt++] = env->prog->insnsi[0];
>> +			new_prog = bpf_patch_insn_data(env, 0, insn_buf, cnt);
>> +			if (!new_prog)
>> +				return -ENOMEM;
>> +			env->prog = new_prog;
>> +			delta += cnt - 1;
>> +		}
>> +	}
>> +
> 
> [...]
>