Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210303123338.99089-1-hxseverything@gmail.com>
 <CA+FuTSfY0y7Y2XSKO-rqPY5mX83NWgAWbQeVukFA94eJVu2B2g@mail.gmail.com> <5D5B444A-FE98-46CF-80D2-DEEBE9C1D74A@gmail.com>
In-Reply-To: <5D5B444A-FE98-46CF-80D2-DEEBE9C1D74A@gmail.com>
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date:   Wed, 3 Mar 2021 22:40:22 -0500
Message-ID: <CA+FuTSdLQPvEChSDDAoN+qk2vOVmhA1OCvJ5T38OvOY1=5HQpQ@mail.gmail.com>
Subject: Re: [PATCH/v4] bpf: add bpf_skb_adjust_room flag BPF_F_ADJ_ROOM_ENCAP_L2_ETH
To:     Xuesen Huang <hxseverything@gmail.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        David Miller <davem@davemloft.net>, bpf <bpf@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Zhiyong Cheng <chengzhiyong@kuaishou.com>,
        Li Wang <wangli09@kuaishou.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

> > 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 mod=
ified?

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.

> >>        /* add L2 encap (if specified) */
> >> +       l2_hdr =3D (__u8 *)&h_outer + olen;
> >>        switch (l2_proto) {
> >>        case ETH_P_MPLS_UC:
> >> -               *((__u32 *)((__u8 *)&h_outer + olen)) =3D mpls_label;
> >> +               *(__u32 *)l2_hdr =3D 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=E2=80=99s 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);

> >> @@ -278,13 +321,24 @@ static __always_inline int encap_ipv6(struct __s=
k_buff *skb, __u8 encap_proto,
> >>        }
> >>
> >>        /* add L2 encap (if specified) */
> >> +       l2_hdr =3D (__u8 *)&h_outer + olen;
> >>        switch (l2_proto) {
> >>        case ETH_P_MPLS_UC:
> >> -               *((__u32 *)((__u8 *)&h_outer + olen)) =3D mpls_label;
> >> +               *(__u32 *)l2_hdr =3D mpls_label;
> >>                break;
> >>        case ETH_P_TEB:
> >> -               if (bpf_skb_load_bytes(skb, 0, (__u8 *)&h_outer + olen=
,
> >> -                                      ETH_HLEN))
> >> +               flags |=3D 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=E2=80=99m 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