bpf: Inline assembly for packet context access

[qmonnet]

• bpf: tests: Minimise calls to ctx_data{,_end}() in prep for inline asm
• bpf: Avoid 32bit assignment of packet pointer

Given that ctx->{data,data_end,data_meta} are 32-bits fields, LLVM sometimes generates 32-bit assignments for those pointers. This leads to verifier errors as the verifier is unable to track the packet pointers through the 32-bit assignments, as show below.

; return (void *)(unsigned long)ctx->data;
2: (61) r9 = *(u32 *)(r7 +76)
; R7_w=ctx(id=0,off=0,imm=0) R9_w=pkt(id=0,off=0,r=0,imm=0)
; return (void *)(unsigned long)ctx->data;
3: (bc) w6 = w9
; R6_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R9_w=pkt(id=0,off=0,r=0,imm=0)
; if (data + tot_len > data_end)
4: (bf) r2 = r6
; R2_w=inv(id=1,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv(id=1,umax_value=4294967295,var_off=(0x0; 0xffffffff))
5: (07) r2 += 54
; R2_w=inv(id=0,umin_value=54,umax_value=4294967349,var_off=(0x0; 0x1ffffffff))
; if (data + tot_len > data_end)
6: (2d) if r2 > r1 goto pc+466
; R1_w=pkt_end(id=0,off=0,imm=0) R2_w=inv(id=0,umin_value=54,umax_value=4294967349,var_off=(0x0; 0x1ffffffff))
; tmp = a->d1 - b->d1;
7: (71) r2 = *(u8 *)(r6 +22)
R6 invalid mem access 'inv'

As a workaround, Yonghong Song suggested to read those fields using asm bytecode. With that trick, LLVM is unaware that the original field is actually on 32 bits and can't perform the above "optimisation".

This workaround is needed only now because code for IPv6 masquerading, coming in a future PR, will cause this bytecode to be generated (snat_v6_needed).

Preliminary to this change, we also need some adjustments in the BPF unit tests, to avoid resetting range tracking for packet pointers by fetching them multiple times (these multiple fetches were previously optimised out by the compiler, but also drop this optimisation by switching to inline assembly).

bpf: Use inline assembly for packet context access, to prevent some undesirable optimizations from LLVM

[qmonnet]

/test

[qmonnet]

/test-1.26-net-next
(Previous run hit #15455)

[qmonnet]

Checkpatch report is a formatting nit that we can ignore in this case - we prefer to keep related definitions close together in this commit rather than adding newlines between them.

[dylandreimerink]

Looks good to me!

[gentoo-root]

Looks really good to me! I'm glad we won't have verifier issues with packet pointers anymore.

We can probably revert the workaround as a follow-up? Also, I believe there are a few revalidate_data calls in the code that can be ditched after your patch.

--

No need to change formatting in the code, but I wonder why checkpatch complains on this:

  Warning: WARNING:LINE_CONTINUATIONS: Avoid unnecessary line continuations
  #86: FILE: bpf/include/bpf/ctx/skb.h:72:
  +	 */									\

Or does it actually mean the previous lines of the multiline comment, not the last one?

[qmonnet]

We can probably revert the workaround as a follow-up? Also, I believe there are a few revalidate_data calls in the code that can be ditched after your patch.

Yes good call, I can look into that next.

No need to change formatting in the code, but I wonder why checkpatch complains on this:

  Warning: WARNING:LINE_CONTINUATIONS: Avoid unnecessary line continuations
  #86: FILE: bpf/include/bpf/ctx/skb.h:72:
  +	 */									\

Or does it actually mean the previous lines of the multiline comment, not the last one?

The comment in checkpatch.pl says:

# check for line continuations outside of #defines, preprocessor #, and asm

so my guess is that between the #define and the asm, there's something causing it to loose track of the fact we're in a #define, and to complain about the trailing backslash. The code is here if you want, but for my part I didn't feel like going through the whole block in Perl 😅