NACK: [X/B/E/F][PATCH 0/2] openvswitch: Same tcp session encapsulated with different udp src port for ovs vxlan tunnel

[Thadeu Lima de Souza Cascardo]

On Fri, May 08, 2020 at 12:10:49PM -0600, Kelsey Skunberg wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860986
> 
> [SRU Justification]
> 

Hi, Kelsey.

I am a little concerned there is no test case for this issue. It also requires
a patched openvswitch to make sense. Of course, one could use a version that is
not present in the archive, so that is not enough reason not to patch it in our
kernel.

However, the xenial backport is skipping one netlink attribute, which will
introduce an ABI incompatibility, just breaking openvswitch and not really
fixing the issue. Hence, my nacking. I decided for nacking for the other series
as well, because this demonstrates this has not been tested enough, which I
would rather see.

Thanks.
Cascardo.

> [Impact]
> 
> Information below is from the Bug Description.
> 
> Packets encapsulated into a vxlan tunnel with openvswitch don't have the same
> udp source port for the first packet and the following ones of the same TCP
> flow in a DOCKER scenario usecase.
> 
> In fact, when using the kernel datapath, the upcall don't include skb hash info
> relatived. As VXLAN module uses the skb hash to select UDP src port, the source
> port is different for the first packet.
> 
> More information can be found here:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364062.html
> 
> This has been fixed in v5.5 by the following upstream commit: bd1903b7c4596
> ("net: openvswitch: add hash info to upcall")
> 
> https://github.com/torvalds/linux/commit/bd1903b7c4596
> 
> The bug exists since the beginning of vxlan support in openvswitch.
> 
> 
> [Fix]
> 
> Backport the requested patch to Focal (5.4), Eoan (5.3), Bionic (4.15) and
> Xenial (4.4).
> 
> To fix this issue, some patches needs to be back-ported on openvswitch too. See
> the following bug:
> 
> https://bugs.launchpad.net/bugs/1860987
> 
> [Risk of Regression]
> 
> This patch only add hash information when we do upcall, thus the risk should be
> low.
> 
> 
> KelseyS: Verfied patch applies to current master-next branches and succesfully
> builds. [B/E/F][PATCH 1/2] is un-touched after cherry-pick.  [X][PATCH 2/2] has
> context canges to apply cleanly to Xenial/master-next.
> 
> 
> Tonghao Zhang (1):
>   net: openvswitch: add hash info to upcall
> 
>  include/uapi/linux/openvswitch.h |  4 +++-
>  net/openvswitch/datapath.c       | 26 +++++++++++++++++++++++++-
>  net/openvswitch/datapath.h       | 12 ++++++++++++
>  3 files changed, 40 insertions(+), 2 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team