Hi Ilya, I addressed most of the review comments in my v3 patch. I am not sure what I need to do for the fixes tag as I am not sure when this bug was introduced. I noticed this in ovs 2.14.
Thanks Thilak Raj S -----Original Message----- From: Thilak Raj Surendra Babu Sent: 06 April 2022 10:11 To: Rosemarie O'Riorden <[email protected]>; Ilya Maximets <[email protected]> Cc: [email protected]; Rosemarie O'Riorden <[email protected]> Subject: RE: [ovs-dev] [PATCH v2] netdev:clear out vlan flow fields while processing native tunnel Thank you, Rosemarie, for your patch, and thank you Ilya for reviewing my patch. I will re-work this and provide an updated patch. Thanks Thilak Raj S -----Original Message----- From: Rosemarie O'Riorden <[email protected]> Sent: 06 April 2022 07:30 To: Ilya Maximets <[email protected]> Cc: Thilak Raj Surendra Babu <[email protected]>; [email protected]; Rosemarie O'Riorden <[email protected]> Subject: Re: [ovs-dev] [PATCH v2] netdev:clear out vlan flow fields while processing native tunnel Hello, Here is my unit test: diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index 57589758f..bfb0f440b 100644 --- a/tests/tunnel-push-pop.at +++ b/tests/tunnel-push-pop.at @@ -842,3 +842,52 @@ 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=0x0 +)),dnl +out_port(100)),8) +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP On Tue, Apr 5, 2022 at 2:35 PM Ilya Maximets <[email protected]> wrote: > > Hi. Thanks for the patch! > > One general comment for the subject line: > 'netdev' doesn't seem to be a correct 'area' for the change. > It should be 'ofproto-dpif-xlate' instead. And, please, add the space > after the ':'. 'summary' should preferably start with a capital > letter and end with a period. > > More details on how to format the patch here: > Documentation/internals/contributing/submitting-patches.rst > > See some more comments inline. > > On 4/5/22 01:01, Thilak Raj Surendra Babu wrote: > > 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. > > Please, limit the line length for a commit message. > 72 characters is the most common limit for it. > > > > > Please, add a 'Fixes' tag. > > > Signed-off-by: Thilak Raj Surendra Babu <[email protected]> > > --- > > While sending new versions of a patch, please, add here a brief > explanation of what changed between versions. > E.g.: > > Version 2: > - Fixed line length warning from checkpatch. > - Dropped unrelated changes. > > > ofproto/ofproto-dpif-xlate.c | 3 ++ > > tests/system-traffic.at | 54 ++++++++++++++++++++++++++++++++++++ > > I see you added a system test. That is OK, good to have one. > However, the issue doesn't require any actual network to be > reproduced. So, we should have a unit test for it. > > I know that Rosemarie worked on the same problem over the past few > weeks, and she already has a working unit test. So, I think it would > be great if you can cooperate and include the unit test into the > patch. > > Rosemarie, could you share what you have? > > > 2 files changed, 57 insertions(+) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c > > b/ofproto/ofproto-dpif-xlate.c index bfd4960dd..34f74aade 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -3541,6 +3541,9 @@ propagate_tunnel_data_to_flow__(struct flow > > *dst_flow, { > > dst_flow->dl_dst = dmac; > > dst_flow->dl_src = smac; > > An empty line here would be nice. > > > + /* Clear VLAN entries which do not apply for tunnel flows */ > > Comments should end with a period. > > > + memset (dst_flow->vlans, 0, > > No need for extra space between the function name and the '('. > > > + sizeof(union flow_vlan_hdr) * FLOW_MAX_VLAN_HEADERS); > > Please, avoid use of sizeof with the type as an argument. > Use the expression (actual object) as an argument instead. > > For more details see the coding-style guide: > Documentation/internals/contributing/coding-style.rst > > > > > dst_flow->packet_type = htonl(PT_ETH); > > dst_flow->nw_dst = src_flow->tunnel.ip_dst; diff --git > > a/tests/system-traffic.at b/tests/system-traffic.at index > > 4a7fa49fc..b0003c2e5 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -259,6 +259,60 @@ 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 > > > > + > > This extra space is unnecessary. > > > +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 options:peer=patch1 -- add-port br-underlay patch1 -- > > +set interface patch1 type=patch options:peer=patch0]) > > + > > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > > +AT_CHECK([ovs-ofctl add-flow br-underlay "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 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]) > > + > > +ADD_OVS_TUNNEL([vxlan], [br-underlay], [at_vxlan0], [172.31.1.1], > > +[10.1.1.100/24]) > > + > > +NS_EXEC([at_ns0], [ip link add link at_vxlan1 name at_vxlan1.100 > > +type vlan id 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]) > > + > > +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 > > +]) > > + > > +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 > > +]) > > + > > +OVS_TRAFFIC_VSWITCHD_STOP(["/ofproto_dpif_xlate(revalidator.*)|WARN > > +|over max translation depth 64.*/d"]) > > Looks like you have a loop in the test topology. That is not good. > Please, remove the loop and you'll not need to filter out warnings. > > > +AT_CLEANUP > > + > > + > > + > > One empty line is enough. > > > AT_SETUP([datapath - ping over vxlan6 tunnel]) > > OVS_CHECK_TUNNEL_TSO() > > OVS_CHECK_VXLAN_UDP6ZEROCSUM() > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
