Re: BPF standardization

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 19 Jul 2024 09:12:58 +0000
Konstantin Ananyev <konstantin.ananyev@huawei.com> wrote:

> bpf_convert_filter() uses standard approach with XOR-ing itself:
> xor r0, r0, r0
> to reset some register values.
> Unfortunately linux verifier seems way too strict here and
> doesn't allow access to register with undefined value.
> It generates error log like that for this op:
> Failed to verify program: Permission denied (13)
> LOG: func#0 @0
> 0: R1=ctx(id=0,off=0,imm=0) R10=fp0
> 0: (af) r0 ^= r0
> R0 !read_ok
> processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> 
> To overcome that, simply replace XOR with itself to explicit
> mov32 r0, #0x0
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---
>  lib/bpf/bpf_convert.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/bpf/bpf_convert.c b/lib/bpf/bpf_convert.c
> index d7ff2b4325..eceaa19c76 100644
> --- a/lib/bpf/bpf_convert.c
> +++ b/lib/bpf/bpf_convert.c
> @@ -267,8 +267,11 @@ static int bpf_convert_filter(const struct bpf_insn *prog, size_t len,
>  		/* Classic BPF expects A and X to be reset first. These need
>  		 * to be guaranteed to be the first two instructions.
>  		 */
> -		*new_insn++ = EBPF_ALU64_REG(BPF_XOR, BPF_REG_A, BPF_REG_A);
> -		*new_insn++ = EBPF_ALU64_REG(BPF_XOR, BPF_REG_X, BPF_REG_X);
> +		//*new_insn++ = EBPF_ALU64_REG(BPF_XOR, BPF_REG_A, BPF_REG_A);
> +		//*new_insn++ = EBPF_ALU64_REG(BPF_XOR, BPF_REG_X, BPF_REG_X);
> +
> +		*new_insn++ = BPF_MOV32_IMM(BPF_REG_A, 0);
> +		*new_insn++ = BPF_MOV32_IMM(BPF_REG_X, 0);
>  
>  		/* All programs must keep CTX in callee saved BPF_REG_CTX.
>  		 * In eBPF case it's done by the compiler, here we need to

This was taken from how kernel converts cBPF and the prologue it generates.
Surprising that verifier gags?

see net/core/filter.c
	/* Classic BPF related prologue emission. */
	if (new_prog) {
		/* Classic BPF expects A and X to be reset first. These need
		 * to be guaranteed to be the first two instructions.
		 */
		*new_insn++ = BPF_ALU32_REG(BPF_XOR, BPF_REG_A, BPF_REG_A);
		*new_insn++ = BPF_ALU32_REG(BPF_XOR, BPF_REG_X, BPF_REG_X);

		/* All programs must keep CTX in callee saved BPF_REG_CTX.
		 * In eBPF case it's done by the compiler, here we need to
		 * do this ourself. Initial CTX is present in BPF_REG_ARG1.
		 */
		*new_insn++ = BPF_MOV64_REG(BPF_REG_CTX, BPF_REG_ARG1);
		if (*seen_ld_abs) {
			/* For packet access in classic BPF, cache skb->data
			 * in callee-saved BPF R8 and skb->len - skb->data_len
			 * (headlen) in BPF R9. Since classic BPF is read-only
			 * on CTX, we only need to cache it once.
			 */
			*new_insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data),
						  BPF_REG_D, BPF_REG_CTX,
						  offsetof(struct sk_buff, data));
			*new_insn++ = BPF_LDX_MEM(BPF_W, BPF_REG_H, BPF_REG_CTX,
						  offsetof(struct sk_buff, len));
			*new_insn++ = BPF_LDX_MEM(BPF_W, BPF_REG_TMP, BPF_REG_CTX,
						  offsetof(struct sk_buff, data_len));
			*new_insn++ = BPF_ALU32_REG(BPF_SUB, BPF_REG_H, BPF_REG_TMP);
		}