[v5,2/2] testpmd: fix l4 sw csum over multi segments
Checks
Commit Message
In csum forwarding mode, software UDP/TCP csum calculation only takes
the first segment into account while using the whole packet length so
the calculation will read invalid memory region with multi-segments
packets and will get wrong value.
This patch fixes this issue.
Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
---
app/test-pmd/csumonly.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
Comments
On 1/6/2022 4:03 PM, Xiaoyun Li wrote:
> In csum forwarding mode, software UDP/TCP csum calculation only takes
> the first segment into account while using the whole packet length so
> the calculation will read invalid memory region with multi-segments
> packets and will get wrong value.
> This patch fixes this issue.
>
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
Can you please check following check-git-log.sh warnings:
./devtools/check-git-log.sh -2
Wrong headline label:
testpmd: fix l4 sw csum over multi segments
Wrong headline case:
"testpmd: fix l4 sw csum over multi segments": l4 --> L4
Wrong headline case:
"testpmd: fix l4 sw csum over multi segments": sw --> SW
Missing 'Fixes' tag:
testpmd: fix l4 sw csum over multi segments
> ---
> app/test-pmd/csumonly.c | 41 ++++++++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 2aeea243b6..0fbe1f1be7 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -96,12 +96,13 @@ struct simple_gre_hdr {
> } __rte_packed;
>
> static uint16_t
> -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
> +get_udptcp_checksum(struct rte_mbuf *m, void *l3_hdr, uint16_t l4_off,
> + uint16_t ethertype)
> {
> if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> - return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> + return rte_ipv4_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
> else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> - return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> + return rte_ipv6_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
> }
>
> /* Parse an IPv4 header to fill l3_len, l4_len, and l4_proto */
> @@ -460,7 +461,7 @@ parse_encap_ip(void *encap_ip, struct testpmd_offload_info *info)
> * depending on the testpmd command line configuration */
> static uint64_t
> process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> - uint64_t tx_offloads)
> + uint64_t tx_offloads, struct rte_mbuf *m)
> {
> struct rte_ipv4_hdr *ipv4_hdr = l3_hdr;
> struct rte_udp_hdr *udp_hdr;
> @@ -468,6 +469,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> struct rte_sctp_hdr *sctp_hdr;
> uint64_t ol_flags = 0;
> uint32_t max_pkt_len, tso_segsz = 0;
> + uint16_t l4_off;
>
> /* ensure packet is large enough to require tso */
> if (!info->is_tunnel) {
> @@ -510,9 +512,15 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
> } else {
> + if (info->is_tunnel)
> + l4_off = info->l2_len +
> + info->outer_l3_len +
> + info->l2_len + info->l3_len;
> + else
> + l4_off = info->l2_len + info->l3_len;
> udp_hdr->dgram_cksum = 0;
> udp_hdr->dgram_cksum =
> - get_udptcp_checksum(l3_hdr, udp_hdr,
> + get_udptcp_checksum(m, l3_hdr, l4_off,
> info->ethertype);
> }
> }
> @@ -527,9 +535,14 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
> } else {
> + if (info->is_tunnel)
> + l4_off = info->l2_len + info->outer_l3_len +
> + info->l2_len + info->l3_len;
> + else
> + l4_off = info->l2_len + info->l3_len;
> tcp_hdr->cksum = 0;
> tcp_hdr->cksum =
> - get_udptcp_checksum(l3_hdr, tcp_hdr,
> + get_udptcp_checksum(m, l3_hdr, l4_off,
> info->ethertype);
> }
> #ifdef RTE_LIB_GSO
> @@ -557,7 +570,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> /* Calculate the checksum of outer header */
> static uint64_t
> process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
> - uint64_t tx_offloads, int tso_enabled)
> + uint64_t tx_offloads, int tso_enabled, struct rte_mbuf *m)
> {
> struct rte_ipv4_hdr *ipv4_hdr = outer_l3_hdr;
> struct rte_ipv6_hdr *ipv6_hdr = outer_l3_hdr;
> @@ -611,12 +624,9 @@ process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
> /* do not recalculate udp cksum if it was 0 */
> if (udp_hdr->dgram_cksum != 0) {
> udp_hdr->dgram_cksum = 0;
> - if (info->outer_ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> - udp_hdr->dgram_cksum =
> - rte_ipv4_udptcp_cksum(ipv4_hdr, udp_hdr);
> - else
> - udp_hdr->dgram_cksum =
> - rte_ipv6_udptcp_cksum(ipv6_hdr, udp_hdr);
> + udp_hdr->dgram_cksum = get_udptcp_checksum(m, outer_l3_hdr,
> + info->l2_len + info->outer_l3_len,
> + info->outer_ethertype);
> }
>
> return ol_flags;
> @@ -957,7 +967,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>
> /* process checksums of inner headers first */
> tx_ol_flags |= process_inner_cksums(l3_hdr, &info,
> - tx_offloads);
> + tx_offloads, m);
>
> /* Then process outer headers if any. Note that the software
> * checksum will be wrong if one of the inner checksums is
> @@ -965,7 +975,8 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> if (info.is_tunnel == 1) {
> tx_ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
> tx_offloads,
> - !!(tx_ol_flags & RTE_MBUF_F_TX_TCP_SEG));
> + !!(tx_ol_flags & RTE_MBUF_F_TX_TCP_SEG),
> + m);
> }
>
> /* step 3: fill the mbuf meta data (flags and header lengths) */
On 06/01/2022 16:03, Xiaoyun Li wrote:
> In csum forwarding mode, software UDP/TCP csum calculation only takes
> the first segment into account while using the whole packet length so
> the calculation will read invalid memory region with multi-segments
> packets and will get wrong value.
> This patch fixes this issue.
There should be a 'Fixes:' tag and assuming you want this patch
backported to LTS, 'Cc: stable@dpdk.org' too.
On 1/21/2022 5:09 PM, Kevin Traynor wrote:
> On 06/01/2022 16:03, Xiaoyun Li wrote:
>> In csum forwarding mode, software UDP/TCP csum calculation only takes
>> the first segment into account while using the whole packet length so
>> the calculation will read invalid memory region with multi-segments
>> packets and will get wrong value.
>> This patch fixes this issue.
>
> There should be a 'Fixes:' tag and assuming you want this patch backported to LTS, 'Cc: stable@dpdk.org' too.
>
It can't be, because fix relies on new APIs added in this release (patch 1/2).
Hi
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Friday, January 21, 2022 15:17
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; olivier.matz@6wind.com;
> mb@smartsharesystems.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stephen@networkplumber.org;
> Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Cc: dev@dpdk.org; Pai G, Sunil <sunil.pai.g@intel.com>
> Subject: Re: [PATCH v5 2/2] testpmd: fix l4 sw csum over multi segments
>
> On 1/6/2022 4:03 PM, Xiaoyun Li wrote:
> > In csum forwarding mode, software UDP/TCP csum calculation only takes
> > the first segment into account while using the whole packet length so
> > the calculation will read invalid memory region with multi-segments
> > packets and will get wrong value.
> > This patch fixes this issue.
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
>
> Can you please check following check-git-log.sh warnings:
>
> ./devtools/check-git-log.sh -2
> Wrong headline label:
> testpmd: fix l4 sw csum over multi segments Wrong headline case:
> "testpmd: fix l4 sw csum over multi segments": l4 --> L4 Wrong
> headline case:
> "testpmd: fix l4 sw csum over multi segments": sw --> SW
> Missing 'Fixes' tag:
> testpmd: fix l4 sw csum over multi segments
>
Will fix this in next version.
BTW, should I change the patch name to "app/testpmd: enable L4 SW csum over multi segments"? or ignore the fixline complaining?
>
>
> > ---
> > app/test-pmd/csumonly.c | 41 ++++++++++++++++++++++++++-----------
> ----
> > 1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
> > 2aeea243b6..0fbe1f1be7 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -96,12 +96,13 @@ struct simple_gre_hdr {
> > } __rte_packed;
> >
> > static uint16_t
> > -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
> > +get_udptcp_checksum(struct rte_mbuf *m, void *l3_hdr, uint16_t l4_off,
> > + uint16_t ethertype)
> > {
> > if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> > - return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > + return rte_ipv4_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
> > else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> > - return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > + return rte_ipv6_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
> > }
> >
> > /* Parse an IPv4 header to fill l3_len, l4_len, and l4_proto */ @@
> > -460,7 +461,7 @@ parse_encap_ip(void *encap_ip, struct
> testpmd_offload_info *info)
> > * depending on the testpmd command line configuration */
> > static uint64_t
> > process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info
> *info,
> > - uint64_t tx_offloads)
> > + uint64_t tx_offloads, struct rte_mbuf *m)
> > {
> > struct rte_ipv4_hdr *ipv4_hdr = l3_hdr;
> > struct rte_udp_hdr *udp_hdr;
> > @@ -468,6 +469,7 @@ process_inner_cksums(void *l3_hdr, const struct
> testpmd_offload_info *info,
> > struct rte_sctp_hdr *sctp_hdr;
> > uint64_t ol_flags = 0;
> > uint32_t max_pkt_len, tso_segsz = 0;
> > + uint16_t l4_off;
> >
> > /* ensure packet is large enough to require tso */
> > if (!info->is_tunnel) {
> > @@ -510,9 +512,15 @@ process_inner_cksums(void *l3_hdr, const struct
> testpmd_offload_info *info,
> > if (tx_offloads &
> RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
> > ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
> > } else {
> > + if (info->is_tunnel)
> > + l4_off = info->l2_len +
> > + info->outer_l3_len +
> > + info->l2_len + info->l3_len;
> > + else
> > + l4_off = info->l2_len + info->l3_len;
> > udp_hdr->dgram_cksum = 0;
> > udp_hdr->dgram_cksum =
> > - get_udptcp_checksum(l3_hdr,
> udp_hdr,
> > + get_udptcp_checksum(m, l3_hdr,
> l4_off,
> > info->ethertype);
> > }
> > }
> > @@ -527,9 +535,14 @@ process_inner_cksums(void *l3_hdr, const struct
> testpmd_offload_info *info,
> > else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
> > ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
> > } else {
> > + if (info->is_tunnel)
> > + l4_off = info->l2_len + info->outer_l3_len +
> > + info->l2_len + info->l3_len;
> > + else
> > + l4_off = info->l2_len + info->l3_len;
> > tcp_hdr->cksum = 0;
> > tcp_hdr->cksum =
> > - get_udptcp_checksum(l3_hdr, tcp_hdr,
> > + get_udptcp_checksum(m, l3_hdr, l4_off,
> > info->ethertype);
> > }
> > #ifdef RTE_LIB_GSO
> > @@ -557,7 +570,7 @@ process_inner_cksums(void *l3_hdr, const struct
> testpmd_offload_info *info,
> > /* Calculate the checksum of outer header */
> > static uint64_t
> > process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info
> *info,
> > - uint64_t tx_offloads, int tso_enabled)
> > + uint64_t tx_offloads, int tso_enabled, struct rte_mbuf *m)
> > {
> > struct rte_ipv4_hdr *ipv4_hdr = outer_l3_hdr;
> > struct rte_ipv6_hdr *ipv6_hdr = outer_l3_hdr; @@ -611,12 +624,9
> @@
> > process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info
> *info,
> > /* do not recalculate udp cksum if it was 0 */
> > if (udp_hdr->dgram_cksum != 0) {
> > udp_hdr->dgram_cksum = 0;
> > - if (info->outer_ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> > - udp_hdr->dgram_cksum =
> > - rte_ipv4_udptcp_cksum(ipv4_hdr, udp_hdr);
> > - else
> > - udp_hdr->dgram_cksum =
> > - rte_ipv6_udptcp_cksum(ipv6_hdr, udp_hdr);
> > + udp_hdr->dgram_cksum = get_udptcp_checksum(m,
> outer_l3_hdr,
> > + info->l2_len + info->outer_l3_len,
> > + info->outer_ethertype);
> > }
> >
> > return ol_flags;
> > @@ -957,7 +967,7 @@ pkt_burst_checksum_forward(struct fwd_stream
> *fs)
> >
> > /* process checksums of inner headers first */
> > tx_ol_flags |= process_inner_cksums(l3_hdr, &info,
> > - tx_offloads);
> > + tx_offloads, m);
> >
> > /* Then process outer headers if any. Note that the software
> > * checksum will be wrong if one of the inner checksums is
> @@
> > -965,7 +975,8 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> > if (info.is_tunnel == 1) {
> > tx_ol_flags |= process_outer_cksums(outer_l3_hdr,
> &info,
> > tx_offloads,
> > - !!(tx_ol_flags &
> RTE_MBUF_F_TX_TCP_SEG));
> > + !!(tx_ol_flags &
> RTE_MBUF_F_TX_TCP_SEG),
> > + m);
> > }
> >
> > /* step 3: fill the mbuf meta data (flags and header lengths)
> */
On 1/24/2022 9:43 AM, Li, Xiaoyun wrote:
> Hi
>
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Friday, January 21, 2022 15:17
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Singh, Aman Deep
>> <aman.deep.singh@intel.com>; olivier.matz@6wind.com;
>> mb@smartsharesystems.com; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; stephen@networkplumber.org;
>> Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>> Cc: dev@dpdk.org; Pai G, Sunil <sunil.pai.g@intel.com>
>> Subject: Re: [PATCH v5 2/2] testpmd: fix l4 sw csum over multi segments
>>
>> On 1/6/2022 4:03 PM, Xiaoyun Li wrote:
>>> In csum forwarding mode, software UDP/TCP csum calculation only takes
>>> the first segment into account while using the whole packet length so
>>> the calculation will read invalid memory region with multi-segments
>>> packets and will get wrong value.
>>> This patch fixes this issue.
>>>
>>> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>> Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
>>
>> Can you please check following check-git-log.sh warnings:
>>
>> ./devtools/check-git-log.sh -2
>> Wrong headline label:
>> testpmd: fix l4 sw csum over multi segments Wrong headline case:
>> "testpmd: fix l4 sw csum over multi segments": l4 --> L4 Wrong
>> headline case:
>> "testpmd: fix l4 sw csum over multi segments": sw --> SW
>> Missing 'Fixes' tag:
>> testpmd: fix l4 sw csum over multi segments
>>
>
> Will fix this in next version.
> BTW, should I change the patch name to "app/testpmd: enable L4 SW csum over multi segments"? or ignore the fixline complaining?
>
Please change the patch title, and it would be good to provide a fix patch,
thanks.
>>
>>
>>> ---
>>> app/test-pmd/csumonly.c | 41 ++++++++++++++++++++++++++-----------
>> ----
>>> 1 file changed, 26 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
>>> 2aeea243b6..0fbe1f1be7 100644
>>> --- a/app/test-pmd/csumonly.c
>>> +++ b/app/test-pmd/csumonly.c
>>> @@ -96,12 +96,13 @@ struct simple_gre_hdr {
>>> } __rte_packed;
>>>
>>> static uint16_t
>>> -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
>>> +get_udptcp_checksum(struct rte_mbuf *m, void *l3_hdr, uint16_t l4_off,
>>> + uint16_t ethertype)
>>> {
>>> if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
>>> - return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>> + return rte_ipv4_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
>>> else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
>>> - return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>> + return rte_ipv6_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
>>> }
>>>
>>> /* Parse an IPv4 header to fill l3_len, l4_len, and l4_proto */ @@
>>> -460,7 +461,7 @@ parse_encap_ip(void *encap_ip, struct
>> testpmd_offload_info *info)
>>> * depending on the testpmd command line configuration */
>>> static uint64_t
>>> process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info
>> *info,
>>> - uint64_t tx_offloads)
>>> + uint64_t tx_offloads, struct rte_mbuf *m)
>>> {
>>> struct rte_ipv4_hdr *ipv4_hdr = l3_hdr;
>>> struct rte_udp_hdr *udp_hdr;
>>> @@ -468,6 +469,7 @@ process_inner_cksums(void *l3_hdr, const struct
>> testpmd_offload_info *info,
>>> struct rte_sctp_hdr *sctp_hdr;
>>> uint64_t ol_flags = 0;
>>> uint32_t max_pkt_len, tso_segsz = 0;
>>> + uint16_t l4_off;
>>>
>>> /* ensure packet is large enough to require tso */
>>> if (!info->is_tunnel) {
>>> @@ -510,9 +512,15 @@ process_inner_cksums(void *l3_hdr, const struct
>> testpmd_offload_info *info,
>>> if (tx_offloads &
>> RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>>> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>>> } else {
>>> + if (info->is_tunnel)
>>> + l4_off = info->l2_len +
>>> + info->outer_l3_len +
>>> + info->l2_len + info->l3_len;
>>> + else
>>> + l4_off = info->l2_len + info->l3_len;
>>> udp_hdr->dgram_cksum = 0;
>>> udp_hdr->dgram_cksum =
>>> - get_udptcp_checksum(l3_hdr,
>> udp_hdr,
>>> + get_udptcp_checksum(m, l3_hdr,
>> l4_off,
>>> info->ethertype);
>>> }
>>> }
>>> @@ -527,9 +535,14 @@ process_inner_cksums(void *l3_hdr, const struct
>> testpmd_offload_info *info,
>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>>> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
>>> } else {
>>> + if (info->is_tunnel)
>>> + l4_off = info->l2_len + info->outer_l3_len +
>>> + info->l2_len + info->l3_len;
>>> + else
>>> + l4_off = info->l2_len + info->l3_len;
>>> tcp_hdr->cksum = 0;
>>> tcp_hdr->cksum =
>>> - get_udptcp_checksum(l3_hdr, tcp_hdr,
>>> + get_udptcp_checksum(m, l3_hdr, l4_off,
>>> info->ethertype);
>>> }
>>> #ifdef RTE_LIB_GSO
>>> @@ -557,7 +570,7 @@ process_inner_cksums(void *l3_hdr, const struct
>> testpmd_offload_info *info,
>>> /* Calculate the checksum of outer header */
>>> static uint64_t
>>> process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info
>> *info,
>>> - uint64_t tx_offloads, int tso_enabled)
>>> + uint64_t tx_offloads, int tso_enabled, struct rte_mbuf *m)
>>> {
>>> struct rte_ipv4_hdr *ipv4_hdr = outer_l3_hdr;
>>> struct rte_ipv6_hdr *ipv6_hdr = outer_l3_hdr; @@ -611,12 +624,9
>> @@
>>> process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info
>> *info,
>>> /* do not recalculate udp cksum if it was 0 */
>>> if (udp_hdr->dgram_cksum != 0) {
>>> udp_hdr->dgram_cksum = 0;
>>> - if (info->outer_ethertype == _htons(RTE_ETHER_TYPE_IPV4))
>>> - udp_hdr->dgram_cksum =
>>> - rte_ipv4_udptcp_cksum(ipv4_hdr, udp_hdr);
>>> - else
>>> - udp_hdr->dgram_cksum =
>>> - rte_ipv6_udptcp_cksum(ipv6_hdr, udp_hdr);
>>> + udp_hdr->dgram_cksum = get_udptcp_checksum(m,
>> outer_l3_hdr,
>>> + info->l2_len + info->outer_l3_len,
>>> + info->outer_ethertype);
>>> }
>>>
>>> return ol_flags;
>>> @@ -957,7 +967,7 @@ pkt_burst_checksum_forward(struct fwd_stream
>> *fs)
>>>
>>> /* process checksums of inner headers first */
>>> tx_ol_flags |= process_inner_cksums(l3_hdr, &info,
>>> - tx_offloads);
>>> + tx_offloads, m);
>>>
>>> /* Then process outer headers if any. Note that the software
>>> * checksum will be wrong if one of the inner checksums is
>> @@
>>> -965,7 +975,8 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>>> if (info.is_tunnel == 1) {
>>> tx_ol_flags |= process_outer_cksums(outer_l3_hdr,
>> &info,
>>> tx_offloads,
>>> - !!(tx_ol_flags &
>> RTE_MBUF_F_TX_TCP_SEG));
>>> + !!(tx_ol_flags &
>> RTE_MBUF_F_TX_TCP_SEG),
>>> + m);
>>> }
>>>
>>> /* step 3: fill the mbuf meta data (flags and header lengths)
>> */
>
On 24/01/2022 09:16, Ferruh Yigit wrote:
> On 1/21/2022 5:09 PM, Kevin Traynor wrote:
>> On 06/01/2022 16:03, Xiaoyun Li wrote:
>>> In csum forwarding mode, software UDP/TCP csum calculation only takes
>>> the first segment into account while using the whole packet length so
>>> the calculation will read invalid memory region with multi-segments
>>> packets and will get wrong value.
>>> This patch fixes this issue.
>>
>> There should be a 'Fixes:' tag and assuming you want this patch backported to LTS, 'Cc: stable@dpdk.org' too.
>>
>
> It can't be, because fix relies on new APIs added in this release (patch 1/2).
>
ok, makes sense, thanks. Do you think it worth adding a Bz for it and
listing it as a known issue in 21.11.1?
On 1/24/2022 10:30 AM, Kevin Traynor wrote:
> On 24/01/2022 09:16, Ferruh Yigit wrote:
>> On 1/21/2022 5:09 PM, Kevin Traynor wrote:
>>> On 06/01/2022 16:03, Xiaoyun Li wrote:
>>>> In csum forwarding mode, software UDP/TCP csum calculation only takes
>>>> the first segment into account while using the whole packet length so
>>>> the calculation will read invalid memory region with multi-segments
>>>> packets and will get wrong value.
>>>> This patch fixes this issue.
>>>
>>> There should be a 'Fixes:' tag and assuming you want this patch backported to LTS, 'Cc: stable@dpdk.org' too.
>>>
>>
>> It can't be, because fix relies on new APIs added in this release (patch 1/2).
>>
>
> ok, makes sense, thanks. Do you think it worth adding a Bz for it and listing it as a known issue in 21.11.1?
>
Was discussing with Xialong, it is in the gray area that if this is a fix
or new feature.
It is not the case that initial implementation says that multi segment
mbufs are supported, in that case this is not a fix but enabling multi
segment support.
So perhaps better to update commit log in this way to prevent confusion
for the fixing and fix tag..
@@ -96,12 +96,13 @@ struct simple_gre_hdr {
} __rte_packed;
static uint16_t
-get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
+get_udptcp_checksum(struct rte_mbuf *m, void *l3_hdr, uint16_t l4_off,
+ uint16_t ethertype)
{
if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
- return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+ return rte_ipv4_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
- return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+ return rte_ipv6_udptcp_cksum_mbuf(m, l3_hdr, l4_off);
}
/* Parse an IPv4 header to fill l3_len, l4_len, and l4_proto */
@@ -460,7 +461,7 @@ parse_encap_ip(void *encap_ip, struct testpmd_offload_info *info)
* depending on the testpmd command line configuration */
static uint64_t
process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
- uint64_t tx_offloads)
+ uint64_t tx_offloads, struct rte_mbuf *m)
{
struct rte_ipv4_hdr *ipv4_hdr = l3_hdr;
struct rte_udp_hdr *udp_hdr;
@@ -468,6 +469,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
struct rte_sctp_hdr *sctp_hdr;
uint64_t ol_flags = 0;
uint32_t max_pkt_len, tso_segsz = 0;
+ uint16_t l4_off;
/* ensure packet is large enough to require tso */
if (!info->is_tunnel) {
@@ -510,9 +512,15 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
} else {
+ if (info->is_tunnel)
+ l4_off = info->l2_len +
+ info->outer_l3_len +
+ info->l2_len + info->l3_len;
+ else
+ l4_off = info->l2_len + info->l3_len;
udp_hdr->dgram_cksum = 0;
udp_hdr->dgram_cksum =
- get_udptcp_checksum(l3_hdr, udp_hdr,
+ get_udptcp_checksum(m, l3_hdr, l4_off,
info->ethertype);
}
}
@@ -527,9 +535,14 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
} else {
+ if (info->is_tunnel)
+ l4_off = info->l2_len + info->outer_l3_len +
+ info->l2_len + info->l3_len;
+ else
+ l4_off = info->l2_len + info->l3_len;
tcp_hdr->cksum = 0;
tcp_hdr->cksum =
- get_udptcp_checksum(l3_hdr, tcp_hdr,
+ get_udptcp_checksum(m, l3_hdr, l4_off,
info->ethertype);
}
#ifdef RTE_LIB_GSO
@@ -557,7 +570,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
/* Calculate the checksum of outer header */
static uint64_t
process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
- uint64_t tx_offloads, int tso_enabled)
+ uint64_t tx_offloads, int tso_enabled, struct rte_mbuf *m)
{
struct rte_ipv4_hdr *ipv4_hdr = outer_l3_hdr;
struct rte_ipv6_hdr *ipv6_hdr = outer_l3_hdr;
@@ -611,12 +624,9 @@ process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
/* do not recalculate udp cksum if it was 0 */
if (udp_hdr->dgram_cksum != 0) {
udp_hdr->dgram_cksum = 0;
- if (info->outer_ethertype == _htons(RTE_ETHER_TYPE_IPV4))
- udp_hdr->dgram_cksum =
- rte_ipv4_udptcp_cksum(ipv4_hdr, udp_hdr);
- else
- udp_hdr->dgram_cksum =
- rte_ipv6_udptcp_cksum(ipv6_hdr, udp_hdr);
+ udp_hdr->dgram_cksum = get_udptcp_checksum(m, outer_l3_hdr,
+ info->l2_len + info->outer_l3_len,
+ info->outer_ethertype);
}
return ol_flags;
@@ -957,7 +967,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
/* process checksums of inner headers first */
tx_ol_flags |= process_inner_cksums(l3_hdr, &info,
- tx_offloads);
+ tx_offloads, m);
/* Then process outer headers if any. Note that the software
* checksum will be wrong if one of the inner checksums is
@@ -965,7 +975,8 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
if (info.is_tunnel == 1) {
tx_ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
tx_offloads,
- !!(tx_ol_flags & RTE_MBUF_F_TX_TCP_SEG));
+ !!(tx_ol_flags & RTE_MBUF_F_TX_TCP_SEG),
+ m);
}
/* step 3: fill the mbuf meta data (flags and header lengths) */