On 4/8/22 07:49, Thilak Raj Surendra Babu wrote:
> 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.
For the 'Fixes' tag it's usually enough to look at 'git blame'.
Since you know the root cause of the issue, you, likely, can
say at a glance if a certain commit introduced it. There are
more complex cases where the issue was actually introduced by
a commit that doesn't touch the code you're changing. For these
you need a deeper analysis or actual testing. But these are
rare cases.
For this patch, the tag should be:
Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc
actions at xlate.")
I can add it on commit, if there will be no more review
comments for v3. So, no need to send v4 just for this.
Best regards, Ilya Maximets.
>
> 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
