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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
Subject: Re: [PATCH] bpf: in bpf_skb_adjust_room correct inner protocol for
 vxlan
From:   =?utf-8?B?6buE5a2m5qOu?= <hxseverything@gmail.com>
In-Reply-To: <CA+FuTScScC2o6uDjua0T3Eucbjt8-YPf65h3xgxMpTtWvgjWmg@mail.gmail.com>
Date:   Tue, 9 Feb 2021 18:41:41 +0800
Cc:     David Miller <davem@davemloft.net>, bpf <bpf@vger.kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Network Development <netdev@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        chengzhiyong <chengzhiyong@kuaishou.com>,
        wangli <wangli09@kuaishou.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8552C5F8-8410-4E81-8AF4-7018878AFCDC@gmail.com>
References: <20210208113810.11118-1-hxseverything@gmail.com>
 <CA+FuTScScC2o6uDjua0T3Eucbjt8-YPf65h3xgxMpTtWvgjWmg@mail.gmail.com>
To:     Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Precedence: bulk

Appreciate for your reply Willem!

The original intention of this commit is that when we use =
bpf_skb_adjust_room  to encapsulate=20
Vxlan packets, we find some powerful device features disabled.=20

Setting the inner_protocol directly as skb->protocol is the root cause.

I understand that it=E2=80=99s not easy to handle all tunnel protocol in =
one bpf helper function. But for my
immature idea, when pushing Ethernet header, setting the inner_protocol =
as ETH_P_TEB may
be better.

Now the flag BPF_F_ADJ_ROOM_ENCAP_L4_UDP includes many udp tunnel types( =
e.g.=20
udp+mpls, geneve, vxlan). Adding an independent flag to represents Vxlan =
looks a little=20
reduplicative. What=E2=80=99s your suggestion?

Thanks again for your reply!


> 2021=E5=B9=B42=E6=9C=888=E6=97=A5 =E4=B8=8B=E5=8D=889:06=EF=BC=8CWillem =
de Bruijn <willemdebruijn.kernel@gmail.com> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> On Mon, Feb 8, 2021 at 7:16 AM huangxuesen <hxseverything@gmail.com> =
wrote:
>>=20
>> From: huangxuesen <huangxuesen@kuaishou.com>
>>=20
>> When pushing vxlan tunnel header, set inner protocol as ETH_P_TEB in =
skb
>> to avoid HW device disabling udp tunnel segmentation offload, just =
like
>> vxlan_build_skb does.
>>=20
>> Drivers for NIC may invoke vxlan_features_check to check the
>> inner_protocol in skb for vxlan packets to decide whether to disable
>> NETIF_F_GSO_MASK. Currently it sets inner_protocol as the original
>> skb->protocol, that will make mlx5_core disable TSO and lead to huge
>> performance degradation.
>>=20
>> Signed-off-by: huangxuesen <huangxuesen@kuaishou.com>
>> Signed-off-by: chengzhiyong <chengzhiyong@kuaishou.com>
>> Signed-off-by: wangli <wangli09@kuaishou.com>
>> ---
>> net/core/filter.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 255aeee72402..f8d3ba3fe10f 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3466,7 +3466,12 @@ static int bpf_skb_net_grow(struct sk_buff =
*skb, u32 off, u32 len_diff,
>>                skb->inner_mac_header =3D inner_net - inner_mac_len;
>>                skb->inner_network_header =3D inner_net;
>>                skb->inner_transport_header =3D inner_trans;
>> -               skb_set_inner_protocol(skb, skb->protocol);
>> +
>> +               if (flags & BPF_F_ADJ_ROOM_ENCAP_L4_UDP &&
>> +                   inner_mac_len =3D=3D ETH_HLEN)
>> +                       skb_set_inner_protocol(skb, =
htons(ETH_P_TEB));
>=20
> This may be used by vxlan, but it does not imply it.
>=20
> Adding ETH_HLEN bytes likely means pushing an Ethernet header, but =
same point.
>=20
> Conversely, pushing an Ethernet header is not limited to UDP encap.
>=20
> This probably needs a new explicit BPF_F_ADJ_ROOM_.. flag, rather than
> trying to infer from imprecise heuristics.