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

[Eduard Zingerman <eddyz87@gmail.com>]

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'.

> +			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;
> +		}
> +	}
> +

[...]