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

[Kelsey Skunberg]

On 2020-05-08 15:54:11 , Thadeu Lima de Souza Cascardo wrote:
> 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.

Hi Cascardo, 

Thanks for reviewing! I really appreciate it.

Seems reasonable for the NACK. I cc'd Maxine who reported the bug and
will update the bug accordingly. 

Thanks again. :)

- Kelsey

+ cc: maxime.leroy at 6wind.com

> 
> > [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