diff mbox series

[ovs-dev,ovs-dev,v2] tunnel: Remove padding from packet when encapsulating.

Message ID 20220125052508.93060-1-xiangxia.m.yue@gmail.com
State Changes Requested
Headers show
Series [ovs-dev,ovs-dev,v2] tunnel: Remove padding from packet when encapsulating. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Tonghao Zhang Jan. 25, 2022, 5:25 a.m. UTC 
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

The old version of openvswitch doesn't remove the padding from
packet before L3+ conntrack processing and then packets are dropped
in linux kernel stack. The patch [1] fixes the issue. We fix this
issue on gateway which running ovs-dpdk as a quick workaround. Padding
should be removed because tunnel size + inner size > 64B.
More detailes, see [1]

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=9382fe71c0058465e942a633869629929102843d
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
v2: add OVS_UNLIKELY
v1: this version was submitted a year ago
http://patchwork.ozlabs.org/project/openvswitch/patch/20201214030936.87354-1-xiangxia.m.yue@gmail.com/
---
 lib/netdev-native-tnl.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Harold Huang Jan. 25, 2022, 7:16 a.m. UTC | #1 
Looks good to me.

Reviewed-by: Harold Huang <baymaxhuang@gmail.com>

<xiangxia.m.yue@gmail.com> 于2022年1月25日周二 13:25写道：
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> The old version of openvswitch doesn't remove the padding from
> packet before L3+ conntrack processing and then packets are dropped
> in linux kernel stack. The patch [1] fixes the issue. We fix this
> issue on gateway which running ovs-dpdk as a quick workaround. Padding
> should be removed because tunnel size + inner size > 64B.
> More detailes, see [1]
>
> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=9382fe71c0058465e942a633869629929102843d
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> v2: add OVS_UNLIKELY
> v1: this version was submitted a year ago
> http://patchwork.ozlabs.org/project/openvswitch/patch/20201214030936.87354-1-xiangxia.m.yue@gmail.com/
> ---
>  lib/netdev-native-tnl.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index b89dfdd52a86..e23d71d4aec1 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -149,11 +149,15 @@ void *
>  netdev_tnl_push_ip_header(struct dp_packet *packet,
>                 const void *header, int size, int *ip_tot_size)
>  {
> +    int padding = dp_packet_l2_pad_size(packet);
>      struct eth_header *eth;
>      struct ip_header *ip;
>      struct ovs_16aligned_ip6_hdr *ip6;
>
>      eth = dp_packet_push_uninit(packet, size);
> +    if (OVS_UNLIKELY(padding)) {
> +        dp_packet_set_size(packet, dp_packet_size(packet) - padding);
> +    }
>      *ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
>
>      memcpy(eth, header, size);
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Tonghao Zhang Feb. 10, 2022, 1:21 a.m. UTC | #2 
On Tue, Jan 25, 2022 at 3:16 PM Harold Huang <baymaxhuang@gmail.com> wrote:
>
> Looks good to me.
>
> Reviewed-by: Harold Huang <baymaxhuang@gmail.com>
Hi Ilya, do you have an opinion?

> <xiangxia.m.yue@gmail.com> 于2022年1月25日周二 13:25写道：
> >
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > The old version of openvswitch doesn't remove the padding from
> > packet before L3+ conntrack processing and then packets are dropped
> > in linux kernel stack. The patch [1] fixes the issue. We fix this
> > issue on gateway which running ovs-dpdk as a quick workaround. Padding
> > should be removed because tunnel size + inner size > 64B.
> > More detailes, see [1]
> >
> > [1] - https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=9382fe71c0058465e942a633869629929102843d
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> > v2: add OVS_UNLIKELY
> > v1: this version was submitted a year ago
> > http://patchwork.ozlabs.org/project/openvswitch/patch/20201214030936.87354-1-xiangxia.m.yue@gmail.com/
> > ---
> >  lib/netdev-native-tnl.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> > index b89dfdd52a86..e23d71d4aec1 100644
> > --- a/lib/netdev-native-tnl.c
> > +++ b/lib/netdev-native-tnl.c
> > @@ -149,11 +149,15 @@ void *
> >  netdev_tnl_push_ip_header(struct dp_packet *packet,
> >                 const void *header, int size, int *ip_tot_size)
> >  {
> > +    int padding = dp_packet_l2_pad_size(packet);
> >      struct eth_header *eth;
> >      struct ip_header *ip;
> >      struct ovs_16aligned_ip6_hdr *ip6;
> >
> >      eth = dp_packet_push_uninit(packet, size);
> > +    if (OVS_UNLIKELY(padding)) {
> > +        dp_packet_set_size(packet, dp_packet_size(packet) - padding);
> > +    }
> >      *ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
> >
> >      memcpy(eth, header, size);
> > --
> > 2.27.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Feb. 10, 2022, 1:43 p.m. UTC | #3 
On 2/10/22 02:21, Tonghao Zhang wrote:
> On Tue, Jan 25, 2022 at 3:16 PM Harold Huang <baymaxhuang@gmail.com> wrote:
>>
>> Looks good to me.
>>
>> Reviewed-by: Harold Huang <baymaxhuang@gmail.com>
> Hi Ilya, do you have an opinion?
> 
>> <xiangxia.m.yue@gmail.com> 于2022年1月25日周二 13:25写道：
>>>
>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>
>>> The old version of openvswitch doesn't remove the padding from
>>> packet before L3+ conntrack processing and then packets are dropped
>>> in linux kernel stack. The patch [1] fixes the issue. We fix this
>>> issue on gateway which running ovs-dpdk as a quick workaround. Padding
>>> should be removed because tunnel size + inner size > 64B.

Hi.  So, the sequence of events is following:

1. Packet arrives to the OVS userspace datapath, packet has padding.
2. OVS encapsulates it and sends to other host that runs OVS with kernel
   datapath.  The kernel version is old and doesn't have fix [1], i.e
   kernel version is not 4.9.104+, 4.14.100+ or 4.16+.
3. Kernel datapath decapsulates the packet and sends to conntrack.
4. conntrack drops the packet as invalid, because it doesn't expect
   L2 padding.

Is that correct?

Some comments inline.

>>> More detailes, see [1]
>>>
>>> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=9382fe71c0058465e942a633869629929102843d
>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>> ---
>>> v2: add OVS_UNLIKELY
>>> v1: this version was submitted a year ago
>>> http://patchwork.ozlabs.org/project/openvswitch/patch/20201214030936.87354-1-xiangxia.m.yue@gmail.com/
>>> ---
>>>  lib/netdev-native-tnl.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>>> index b89dfdd52a86..e23d71d4aec1 100644
>>> --- a/lib/netdev-native-tnl.c
>>> +++ b/lib/netdev-native-tnl.c
>>> @@ -149,11 +149,15 @@ void *
>>>  netdev_tnl_push_ip_header(struct dp_packet *packet,
>>>                 const void *header, int size, int *ip_tot_size)
>>>  {
>>> +    int padding = dp_packet_l2_pad_size(packet);
>>>      struct eth_header *eth;
>>>      struct ip_header *ip;
>>>      struct ovs_16aligned_ip6_hdr *ip6;
>>>
>>>      eth = dp_packet_push_uninit(packet, size);
>>> +    if (OVS_UNLIKELY(padding)) {

Would be great to have a short comment here saying why we need to trim the packet.
e.g. "Older kernels may drop the packet after decapsulation if it contains
unexpected L2 padding."

>>> +        dp_packet_set_size(packet, dp_packet_size(packet) - padding);

It doesn't seem intuitive to trim after we pushed the header.  Can we do
that before instead?

>>> +    }
>>>      *ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
>>>
>>>      memcpy(eth, header, size);
>>> --
>>> 2.27.0
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> 
>
diff mbox series

Patch

diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index b89dfdd52a86..e23d71d4aec1 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -149,11 +149,15 @@  void *
 netdev_tnl_push_ip_header(struct dp_packet *packet,
                const void *header, int size, int *ip_tot_size)
 {
+    int padding = dp_packet_l2_pad_size(packet);
     struct eth_header *eth;
     struct ip_header *ip;
     struct ovs_16aligned_ip6_hdr *ip6;
 
     eth = dp_packet_push_uninit(packet, size);
+    if (OVS_UNLIKELY(padding)) {
+        dp_packet_set_size(packet, dp_packet_size(packet) - padding);
+    }
     *ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
 
     memcpy(eth, header, size);