Re: [PATCH/v4] bpf: add bpf_skb_adjust_room flag BPF_F_ADJ_ROOM_ENCAP_L2_ETH

quoted

Instead of untyped macros, I'd define encap_ipv4 as a function that
calls __encap_ipv4.

And no need for encap_ipv4_with_ext_proto equivalent to __encap_ipv4.

I defined these macros to try to keep the existing  invocation for encap_ipv4/6
as the same, if we define this as a function all invocation should be modified?

You can leave the existing invocations the same and make the new
callers caller __encap_ipv4 directly, which takes one extra argument?
Adding a __ prefixed variant with extra args is a common pattern.

quoted

quoted

       /* add L2 encap (if specified) */
+       l2_hdr = (__u8 *)&h_outer + olen;
       switch (l2_proto) {
       case ETH_P_MPLS_UC:
-               *((__u32 *)((__u8 *)&h_outer + olen)) = mpls_label;
+               *(__u32 *)l2_hdr = mpls_label;
               break;
       case ETH_P_TEB:
-               if (bpf_skb_load_bytes(skb, 0, (__u8 *)&h_outer + olen,
-                                      ETH_HLEN))

This is non-standard indentation? Here and elsewhere.

I thinks it’s a previous issue.

Ah right. Bad example. How about in __encap_vxlan_eth

+               return encap_ipv4_with_ext_proto(skb, IPPROTO_UDP,
+                               ETH_P_TEB, EXTPROTO_VXLAN);

quoted

quoted

@@ -278,13 +321,24 @@ static __always_inline int encap_ipv6(struct __sk_buff *skb, __u8 encap_proto,
       }

       /* add L2 encap (if specified) */
+       l2_hdr = (__u8 *)&h_outer + olen;
       switch (l2_proto) {
       case ETH_P_MPLS_UC:
-               *((__u32 *)((__u8 *)&h_outer + olen)) = mpls_label;
+               *(__u32 *)l2_hdr = mpls_label;
               break;
       case ETH_P_TEB:
-               if (bpf_skb_load_bytes(skb, 0, (__u8 *)&h_outer + olen,
-                                      ETH_HLEN))
+               flags |= BPF_F_ADJ_ROOM_ENCAP_L2_ETH;

This is a change also for the existing case. Correctly so, I imagine.
But the test used to pass with the wrong protocol?

Yes all tests pass. I’m not sure should we add this flag for the existing tests
which encap eth as the l2 header or only for the Vxlan test?

It is correct in both cases. If it does not break anything, I would do both.

Thanks,

  Willem