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

[Eduard Zingerman <eddyz87@gmail.com>]

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

> +				cnt = 1;
> +			} else {
> +				memcpy(insn_buf, epilogue_buf,
> +				       epilogue_cnt * sizeof(*epilogue_buf));
> +				cnt = epilogue_cnt;
> +				/* epilogue_idx cannot be 0. It must have at
> +				 * least one ctx ptr saving insn before the
> +				 * epilogue.
> +				 */
> +				 epilogue_idx = i + delta;
> +			}
> +			goto patch_insn_buf;
>  		} else {
>  			continue;
>  		}

[...]