Hi Ilya, No problem! Thank you for reviewing my latest patch. Fixed the minor review comments, changed the complex system test to a simpler one as per your suggestion and submitted v4.
Thanks Thilak Raj S -----Original Message----- From: Ilya Maximets <[email protected]> Sent: 25 April 2022 14:08 To: Thilak Raj Surendra Babu <[email protected]>; [email protected] Cc: [email protected]; Rosemarie O'Riorden <[email protected]> Subject: Re: [PATCH v3] ofproto-dpif-xlate: Clear out vlan flow fields while processing native tunnel. On 4/21/22 04:11, Thilak Raj Surendra Babu wrote: > Hi Ilya, > If there are no further review comments, can you please merge this patch? Hi. Sorry for replies taking so long, I was on PTO for a last couple of weeks. Unfortunately, there was no replies form others either. See some comments inline. > > Thanks > Thilak Raj S > > > -----Original Message----- > From: Thilak Raj Surendra Babu <[email protected]> > Sent: 07 April 2022 14:08 > To: [email protected] > Cc: Thilak Raj Surendra Babu <[email protected]>; Rosemarie > O'Riorden <[email protected]> > Subject: [PATCH v3] ofproto-dpif-xlate: Clear out vlan flow fields while > processing native tunnel. > > When a packet is received over an access port that needs to be sent over a > vxlan tunnel,the access port VLAN id is used in the lookup leading to a wrong > packet being crafted and sent over the tunnel. > Clear out the flow 's VLAN field as it should not be used while performing > mac lookup for the outer tunnel and also at this point the VLAN action > related to inner flow is already committed. > > Version 3: > - Address review comments. > - Add unit-test patch from Rosemarie O'Riorden. > > Version 2: > - Fixed line length warning from checkpatch. > - Dropped unrelated changes. Nit: This version log should be below the first '---', so it will not be part of the commit messages. > Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc actions at xlate.") > Signed-off-by: Thilak Raj Surendra Babu <[email protected]> > Signed-off-by: Rosemarie O'Riorden <[email protected]> > Co-authored-by: Rosemarie O'Riorden <[email protected]> > --- > ofproto/ofproto-dpif-xlate.c | 3 ++ > tests/system-traffic.at | 61 ++++++++++++++++++++++++++++++++++++ > tests/tunnel-push-pop.at | 52 ++++++++++++++++++++++++++++++ > 3 files changed, 116 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c > b/ofproto/ofproto-dpif-xlate.c index 5a770f171..7ce61b176 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3542,6 +3542,9 @@ propagate_tunnel_data_to_flow__(struct flow *dst_flow, > dst_flow->dl_dst = dmac; > dst_flow->dl_src = smac; > > + /* Clear VLAN entries which do not apply for tunnel flows. */ > + memset(dst_flow->vlans, 0, sizeof(dst_flow->vlans)); Nit: Please, don't parenthesize the argument of the 'sizeof'. > + > dst_flow->packet_type = htonl(PT_ETH); > dst_flow->nw_dst = src_flow->tunnel.ip_dst; > dst_flow->nw_src = src_flow->tunnel.ip_src; diff --git > a/tests/system-traffic.at b/tests/system-traffic.at index > 4a7fa49fc..f1a146b97 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -259,6 +259,67 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i > 0.3 -w 2 10.1.1.100 | FORMAT_PI OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP > > +AT_SETUP([datapath - ping vlan over vxlan tunnel]) > +OVS_CHECK_TUNNEL_TSO() > +OVS_CHECK_VXLAN() > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-underlay]) > + > +AT_CHECK([ovs-vsctl -- add-port br0 patch0 -- set interface patch0 > +type=patch dnl > +options:peer=patch1 -- add-port br-underlay patch1 -- set interface > +patch1 dnl type=patch options:peer=patch0]) AT_CHECK([ovs-ofctl > +add-flow br0 "actions=normal"]) AT_CHECK([ovs-ofctl add-flow > +br-underlay "table=0,priority=10,actions=normal"]) > + > +ADD_NAMESPACES(at_ns0) > + > +dnl Set up underlay link from host into the namespace using veth pair. > +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") AT_CHECK([ip addr > +add dev br-underlay "172.31.1.100/24"]) AT_CHECK([ip link set dev > +br-underlay up]) > + > +dnl Add a rule to forward arp packets towards tunnels remote endpoint > +AT_CHECK([ovs-ofctl add-flow br-underlay > +"in_port=br-underlay,priority=12,dnl > +dl_type=0x806,nw_dst=172.31.1.1,actions=output:ovs-p0"]) > + > +dnl Set up tunnel endpoints on OVS outside the namespace and with a > +native dnl linux device inside the namespace. > +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], > [10.1.1.1/24], > + [id 0 dstport 4789]) NS_EXEC([at_ns0], [ip link add > +link at_vxlan1 name at_vxlan1.100 type NS_CHECK_EXEC() should be used instead of plain NS_EXEC(). > +vlan id dnl > +100]) > +NS_EXEC([at_ns0], [ip addr flush dev at_vxlan1]) NS_EXEC([at_ns0], > +[ip addr add dev at_vxlan1.100 "10.1.1.30/24"]) NS_EXEC([at_ns0], [ip > +link set dev at_vxlan1.100 up]) > + > +dnl add OVS tunnel > +ADD_OVS_TUNNEL([vxlan], [br-underlay], [at_vxlan0], [172.31.1.1], > +[10.1.1.100/24]) > + > +AT_CHECK([ovs-ofctl add-flow br-underlay > +"priority=12,dl_type=0x806,dnl > +nw_dst=10.1.1.30,actions=output:at_vxlan0"]) > +AT_CHECK([ovs-ofctl add-flow br-underlay > +"priority=12,dl_type=0x800,dnl > +nw_dst=10.1.1.30,actions=output:at_vxlan0"]) > +AT_CHECK([ovs-ofctl mod-port br-underlay at_vxlan0 no-flood]) > + > +ADD_NAMESPACES(at_ns1) > +ADD_VETH(p1, at_ns1, br0, "10.1.1.10/24") > + > +AT_CHECK([ovs-vsctl set port ovs-p1 tag=100]) > + > +dnl First, check the underlay > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | > +FORMAT_PING], [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +dnl Check if overlay network is reachable NS_CHECK_EXEC([at_ns1], > +[ping -q -c 3 -i 0.3 -w 2 10.1.1.30 | FORMAT_PING], [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) The test seems to work now, but it's really hard to understand the topology and the path of acual packets within OVS. Can we take the same topology as in the previous 'ping over vxlan tunnel' test, but add vlan tags to OVS ports 'br0' and 'br-underlay', i.e. AT_CHECK([ovs-vsctl set port br0 tag=100]) AT_CHECK([ovs-vsctl set port br-underlay tag=42]) And add a vlan interface inside the at_ns0 namespace, p0.42 with: ADD_VLAN(p0, at_ns0, 42, "172.31.1.1/24") (the ip of the p0 itself should be changed to, e.g., 172.31.2.1). This way the inner packet will have vlan 100, then encapsulated and vlan 42 will be pushed on top. Such topology should still reproduce the issue, but it's much simpler, IMO. What do you think? > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([datapath - ping over vxlan6 tunnel]) > OVS_CHECK_TUNNEL_TSO() > OVS_CHECK_VXLAN_UDP6ZEROCSUM() > diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index > 57589758f..b1bc328c3 100644 > --- a/tests/tunnel-push-pop.at > +++ b/tests/tunnel-push-pop.at > @@ -842,3 +842,55 @@ Datapath actions: 7 > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([tunnel_push_pop - VXLAN access port]) > + > +dnl Create bridge that has a MAC address OVS_VSWITCHD_START([set > +bridge > +br0 dnl > + datapath_type=dummy -- set Interface dnl > + br0 other-config:hwaddr=aa:55:aa:55:00:00]) > +AT_CHECK([ovs-vsctl add-port br0 p8 -- set Interface p8 dnl > + type=dummy ofport_request=8]) > + > +dnl Create another bridge > +AT_CHECK([ovs-vsctl add-br ovs-tun0 -- set bridge ovs-tun0 > +datapath_type=dummy]) > + > +dnl Add VXLAN port to this bridge > +AT_CHECK([ovs-vsctl add-port ovs-tun0 tun0 -- set int tun0 dnl > + type=vxlan options:remote_ip=10.0.0.11 -- add-port ovs-tun0 p7 dnl > + -- set interface p7 type=dummy ofport_request=7]) > + > +dnl Set VLAN tags, so that br0 and its port p8 have the same tag, dnl > +but ovs-tun0's port p7 has a different tag AT_CHECK([ovs-vsctl set > +port > +p8 tag=42 -- set port br0 tag=42 dnl > + -- set port p7 tag=200]) > + > +dnl Set IP address and route for br0 > +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 10.0.0.2/24], [0], [OK > +]) > +AT_CHECK([ovs-appctl ovs/route/add 10.0.0.11/24 br0], [0], [OK > +]) > + > + > +dnl Send an ARP reply to port b8 on br0, so that packets will be > +forwarded dnl to learned port AT_CHECK([ovs-ofctl add-flow br0 > +action=normal]) > + > + > +AT_CHECK([ovs-appctl netdev-dummy/receive p8 'in_port(8),dnl > + > +eth(src=aa:55:aa:66:00:00,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),dnl > + > +arp(sip=10.0.0.11,tip=10.0.0.2,op=2,sha=aa:55:aa:66:00:00,tha=00:00:00: > +00:00:00)']) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-tun0 in_port=p7], [0], > +[stdout]) AT_CHECK([tail -2 stdout], [0], [dnl > +Megaflow: recirc_id=0,eth,in_port=7,dl_src=00:00:00:00:00:00,dnl > +dl_dst=00:00:00:00:00:00,dl_type=0x0000 > +Datapath actions: > +push_vlan(vid=200,pcp=0),1,clone(tnl_push(tnl_port(4789),dnl > +header(size=50,type=4,eth(dst=aa:55:aa:66:00:00,src=aa:55:aa:55:00:00 > +,d > +nl > +dl_type=0x0800),ipv4(src=10.0.0.2,dst=10.0.0.11,proto=17,tos=0,ttl=64 > +,d > +nl > +frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0x8000000,vni=0 > +x0 > +)),dnl > +out_port(100)),8) > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.34.1 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
