On Tue, 2024-08-13 at 11:49 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
Apart from the note below I don't see any obvious problems with this code.
Reviewed-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
[...]
> --- 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;
> + }
> + }
> +
[...]
