[PATCH] app/testpmd: fix GENEVE parsing in csum forward mode

[Singh, Aman Deep]

On 1/18/2022 6:49 PM, Matan Azrad wrote:
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>> Sent: Tuesday, January 18, 2022 3:03 PM
>> To: Matan Azrad <matan at nvidia.com>; Raja Zidane <rzidane at nvidia.com>;
>> dev at dpdk.org
>> Cc: stable at dpdk.org
>> Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum forward mode
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 1/18/2022 12:55 PM, Matan Azrad wrote:
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>>>> Sent: Tuesday, January 18, 2022 2:28 PM
>>>> To: Matan Azrad <matan at nvidia.com>; Raja Zidane <rzidane at nvidia.com>;
>>>> dev at dpdk.org
>>>> Cc: stable at dpdk.org
>>>> Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum forward
>>>> mode
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 1/18/2022 11:27 AM, Matan Azrad wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>>>>>> Sent: Tuesday, January 18, 2022 11:52 AM
>>>>>> To: Raja Zidane <rzidane at nvidia.com>; dev at dpdk.org
>>>>>> Cc: Matan Azrad <matan at nvidia.com>; stable at dpdk.org
>>>>>> Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum
>>>>>> forward mode
>>>>>>
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> On 12/5/2021 3:44 AM, Raja Zidane wrote:
>>>>>>> The csum FWD mode parses any received packet to set mbuf offloads
>>>>>>> for the transmitting burst, mainly in the checksum/TSO areas.
>>>>>>> In the case of a tunnel header, the csum FWD tries to detect known
>>>>>>> tunnels by the standard definition using the header'sdata and
>>>>>>> fallback to check the packet type in the mbuf to see if the Rx
>>>>>>> port driver already sign the packet as a tunnel.
>>>>>>> In the fallback case, the csum assumes the tunnel is VXLAN and
>>>>>>> parses the tunnel as VXLAN.
>>>>>> As far as I can see there is a VXLAN port check in 'parse_vxlan()',
>>>>>> why it is not helping?
>>>>>>
>>>>> The problem is not the vxlan check but the tunnel type in mbuf that
>>>>> caused the
>>>> packet to be detected as vxlan(default) before checking GENEVE tunnel case.
>>>> Check is as following:
>>>>
>>>>            if (udp_hdr->dst_port != _htons(RTE_VXLAN_DEFAULT_PORT) &&
>>>>                    RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0)
>>>>                    return;
>>>>
>>>> Do you what is the intention for the "RTE_ETH_IS_TUNNEL_PKT(pkt_type) ==
>> 0"
>>>> check?
>>>> Why vxlan parsing doesn't stop when it is not default port?
>>> Maybe some drivers set the tunnel type for vxlan packets coming after non-
>> standard vxlan port.
>> But checking the tunnel flag to say that it is vxlan is too broad, isn't it?
>> And this is the problem you are having.
>>
>> Can there be any way to detect and check non-standard vxlan port?
> Maybe yes, but it is probably more complex solusion.
>
> See this patch:
> https://patches.dpdk.org/project/dpdk/patch/1423819371-24222-13-git-send-email-olivier.matz@6wind.com/
>
> comments there:
> 1. check udp destination port, 4789 is the default vxlan port (rfc7348).
> 2. currently, this flag is set by i40e only if the packet is vxlan
>
> And maybe another driver assumes more ports here.
>
> Collecting all the non-standard ports can start from i40 maintainers😊
I think, here we should check for supported tunnel types only.  The i40 
driver setting a Tunnel flag, does not means that it VxLan type only.
As in your case those were GENEVE packets getting treated as VxLan.

Making parse_vxlan() udp_dst_port specific can avoid this issue.

>>>>>>> When the GENEVE tunnel was added to the known tunnels in csum, its
>>>>>>> parsing trial was wrongly located after the pkt type detection,
>>>>>>> causing the csum to parse the GENEVE header as VXLAN when the Rx
>>>>>>> port set the tunnel packet type.
>>>>>>>
>>>>>>> Locate the GENEVE parsing trial before the packet type detection.
>>>>>>>
>>>>>>> Fixes: ea0e711b8ae0 ("app/testpmd: add GENEVE parsing")
>>>>>>> Cc: stable at dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Raja Zidane <rzidane at nvidia.com>
>>>>>>> ---
>>>>>>> Acked-by: Matan Azrad <matan at nvidia.com>
>>>>>> Ack should be before '---' to be part of the commit log, otherwise
>>>>>> it is dropped when applied as comment.
>>>>>>
>>>>>>>      app/test-pmd/csumonly.c | 16 ++++++++++------
>>>>>>>      1 file changed, 10 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>>>>>>> index 2aeea243b6..fe810fecdd 100644
>>>>>>> --- a/app/test-pmd/csumonly.c
>>>>>>> +++ b/app/test-pmd/csumonly.c
>>>>>>> @@ -254,7 +254,10 @@ parse_gtp(struct rte_udp_hdr *udp_hdr,
>>>>>>>          info->l2_len += RTE_ETHER_GTP_HLEN;
>>>>>>>      }
>>>>>>>
>>>>>>> -/* Parse a vxlan header */
>>>>>>> +/*
>>>>>>> + * Parse a vxlan header.
>>>>>>> + * If a tunnel is detected in 'pkt_type' it will be parsed by default as vxlan.
>>>>>>> + */
>>>>>>>      static void
>>>>>>>      parse_vxlan(struct rte_udp_hdr *udp_hdr,
>>>>>>>              struct testpmd_offload_info *info, @@ -912,17 +915,18
>>>>>>> @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>>>>>>>                                                  RTE_MBUF_F_TX_TUNNEL_VXLAN_GPE;
>>>>>>>                                          goto tunnel_update;
>>>>>>>                                  }
>>>>>>> -                             parse_vxlan(udp_hdr, &info,
>>>>>>> -                                         m->packet_type);
>>>>>>> +                             parse_geneve(udp_hdr, &info);
>>>>>>>                                  if (info.is_tunnel) {
>>>>>>>                                          tx_ol_flags |=
>>>>>>> -                                             RTE_MBUF_F_TX_TUNNEL_VXLAN;
>>>>>>> +
>>>>>>> + RTE_MBUF_F_TX_TUNNEL_GENEVE;
>>>>>>>                                          goto tunnel_update;
>>>>>>>                                  }
>>>>>>> -                             parse_geneve(udp_hdr, &info);
>>>>>>> +                             /* Always keep last. */
>>>>>>> +                             parse_vxlan(udp_hdr, &info,
>>>>>>> +                                         m->packet_type);
>>>>>>>                                  if (info.is_tunnel) {
>>>>>>>                                          tx_ol_flags |=
>>>>>>> -                                             RTE_MBUF_F_TX_TUNNEL_GENEVE;
>>>>>>> +
>>>>>>> + RTE_MBUF_F_TX_TUNNEL_VXLAN;
>>>>>>>                                          goto tunnel_update;
>>>>>>>                                  }
>>>>>>>                          } else if (info.l4_proto == IPPROTO_GRE) {