[dpdk-stable] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[dpdk-stable] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Xiaoyun Li]

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.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
---
 app/test-pmd/csumonly.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 090797318a..5df3be0a6f 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -18,7 +18,7 @@
 #include <rte_log.h>
 #include <rte_debug.h>
 #include <rte_cycles.h>
-#include <rte_memory.h>
+#include <rte_malloc.h>
 #include <rte_memcpy.h>
 #include <rte_launch.h>
 #include <rte_eal.h>
@@ -56,6 +56,11 @@
 #define GRE_SUPPORTED_FIELDS	(GRE_CHECKSUM_PRESENT | GRE_KEY_PRESENT |\
 				 GRE_SEQUENCE_PRESENT)
 
+/* When UDP or TCP or outer UDP csum offload is off, sw l4 csum is needed */
+#define UDP_TCP_CSUM            (DEV_TX_OFFLOAD_UDP_CKSUM |\
+				 DEV_TX_OFFLOAD_TCP_CKSUM |\
+				 DEV_TX_OFFLOAD_OUTER_UDP_CKSUM)
+
 /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
 #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
 #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
@@ -602,12 +607,8 @@ 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(outer_l3_hdr,
+					udp_hdr, info->outer_ethertype);
 	}
 
 	return ol_flags;
@@ -802,6 +803,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 	struct rte_mbuf *m, *p;
 	struct rte_ether_hdr *eth_hdr;
 	void *l3_hdr = NULL, *outer_l3_hdr = NULL; /* can be IPv4 or IPv6 */
+	uint8_t *l3_buf = NULL;
 	void **gro_ctx;
 	uint16_t gro_pkts_num;
 	uint8_t gro_enable;
@@ -877,7 +879,19 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
 				&eth_hdr->src_addr);
 		parse_ethernet(eth_hdr, &info);
-		l3_hdr = (char *)eth_hdr + info.l2_len;
+		/* When sw csum is needed, multi-segs needs a buf to contain
+		 * the whole packet for later UDP/TCP csum calculation.
+		 */
+		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
+		    !(tx_offloads & UDP_TCP_CSUM)) {
+			l3_buf = rte_zmalloc("csum l3_buf",
+					     info.pkt_len - info.l2_len,
+					     RTE_CACHE_LINE_SIZE);
+			rte_pktmbuf_read(m, info.l2_len,
+					 info.pkt_len - info.l2_len, l3_buf);
+			l3_hdr = l3_buf;
+		} else
+			l3_hdr = (char *)eth_hdr + info.l2_len;
 
 		/* check if it's a supported tunnel */
 		if (txp->parse_tunnel) {
@@ -1051,6 +1065,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 			printf("tx: flags=%s", buf);
 			printf("\n");
 		}
+		rte_free(l3_buf);
 	}
 
 	if (unlikely(gro_enable)) {
-- 
2.25.1

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[David Marchand]

Hello,

On Fri, Oct 15, 2021 at 7:27 AM Xiaoyun Li <xiaoyun.li@intel.com> 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.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> ---
>  app/test-pmd/csumonly.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 090797318a..5df3be0a6f 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -18,7 +18,7 @@
>  #include <rte_log.h>
>  #include <rte_debug.h>
>  #include <rte_cycles.h>
> -#include <rte_memory.h>
> +#include <rte_malloc.h>

This include caught my eye.


>  #include <rte_memcpy.h>
>  #include <rte_launch.h>
>  #include <rte_eal.h>
> @@ -56,6 +56,11 @@
>  #define GRE_SUPPORTED_FIELDS   (GRE_CHECKSUM_PRESENT | GRE_KEY_PRESENT |\
>                                  GRE_SEQUENCE_PRESENT)
>
> +/* When UDP or TCP or outer UDP csum offload is off, sw l4 csum is needed */
> +#define UDP_TCP_CSUM            (DEV_TX_OFFLOAD_UDP_CKSUM |\
> +                                DEV_TX_OFFLOAD_TCP_CKSUM |\
> +                                DEV_TX_OFFLOAD_OUTER_UDP_CKSUM)
> +
>  /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
>  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>  #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
> @@ -602,12 +607,8 @@ 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(outer_l3_hdr,
> +                                       udp_hdr, info->outer_ethertype);
>         }
>
>         return ol_flags;
> @@ -802,6 +803,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>         struct rte_mbuf *m, *p;
>         struct rte_ether_hdr *eth_hdr;
>         void *l3_hdr = NULL, *outer_l3_hdr = NULL; /* can be IPv4 or IPv6 */
> +       uint8_t *l3_buf = NULL;
>         void **gro_ctx;
>         uint16_t gro_pkts_num;
>         uint8_t gro_enable;
> @@ -877,7 +879,19 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>                 rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
>                                 &eth_hdr->src_addr);
>                 parse_ethernet(eth_hdr, &info);
> -               l3_hdr = (char *)eth_hdr + info.l2_len;
> +               /* When sw csum is needed, multi-segs needs a buf to contain
> +                * the whole packet for later UDP/TCP csum calculation.
> +                */
> +               if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> +                   !(tx_offloads & UDP_TCP_CSUM)) {
> +                       l3_buf = rte_zmalloc("csum l3_buf",
> +                                            info.pkt_len - info.l2_len,
> +                                            RTE_CACHE_LINE_SIZE);

Rather than call a dyn allocation in datapath, can't we have a static
buffer on the stack?


> +                       rte_pktmbuf_read(m, info.l2_len,
> +                                        info.pkt_len - info.l2_len, l3_buf);
> +                       l3_hdr = l3_buf;
> +               } else
> +                       l3_hdr = (char *)eth_hdr + info.l2_len;
>
>                 /* check if it's a supported tunnel */
>                 if (txp->parse_tunnel) {
> @@ -1051,6 +1065,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>                         printf("tx: flags=%s", buf);
>                         printf("\n");
>                 }
> +               rte_free(l3_buf);
>         }
>
>         if (unlikely(gro_enable)) {
> --
> 2.25.1
>


-- 
David Marchand

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, October 15, 2021 16:10
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev <dev@dpdk.org>; dpdk stable
> <stable@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi
> segments
> 
> Hello,
> 
> On Fri, Oct 15, 2021 at 7:27 AM Xiaoyun Li <xiaoyun.li@intel.com> 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.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > ---
> >  app/test-pmd/csumonly.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
> > 090797318a..5df3be0a6f 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -18,7 +18,7 @@
> >  #include <rte_log.h>
> >  #include <rte_debug.h>
> >  #include <rte_cycles.h>
> > -#include <rte_memory.h>
> > +#include <rte_malloc.h>
> 
> This include caught my eye.
> 
> 
> >  #include <rte_memcpy.h>
> >  #include <rte_launch.h>
> >  #include <rte_eal.h>
> > @@ -56,6 +56,11 @@
> >  #define GRE_SUPPORTED_FIELDS   (GRE_CHECKSUM_PRESENT |
> GRE_KEY_PRESENT |\
> >                                  GRE_SEQUENCE_PRESENT)
> >
> > +/* When UDP or TCP or outer UDP csum offload is off, sw l4 csum is needed
> */
> > +#define UDP_TCP_CSUM            (DEV_TX_OFFLOAD_UDP_CKSUM |\
> > +                                DEV_TX_OFFLOAD_TCP_CKSUM |\
> > +                                DEV_TX_OFFLOAD_OUTER_UDP_CKSUM)
> > +
> >  /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
> > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN  #define _htons(x)
> > ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8))) @@
> > -602,12 +607,8 @@ 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(outer_l3_hdr,
> > +                                       udp_hdr,
> > + info->outer_ethertype);
> >         }
> >
> >         return ol_flags;
> > @@ -802,6 +803,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> >         struct rte_mbuf *m, *p;
> >         struct rte_ether_hdr *eth_hdr;
> >         void *l3_hdr = NULL, *outer_l3_hdr = NULL; /* can be IPv4 or
> > IPv6 */
> > +       uint8_t *l3_buf = NULL;
> >         void **gro_ctx;
> >         uint16_t gro_pkts_num;
> >         uint8_t gro_enable;
> > @@ -877,7 +879,19 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> >                 rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
> >                                 &eth_hdr->src_addr);
> >                 parse_ethernet(eth_hdr, &info);
> > -               l3_hdr = (char *)eth_hdr + info.l2_len;
> > +               /* When sw csum is needed, multi-segs needs a buf to contain
> > +                * the whole packet for later UDP/TCP csum calculation.
> > +                */
> > +               if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> > +                   !(tx_offloads & UDP_TCP_CSUM)) {
> > +                       l3_buf = rte_zmalloc("csum l3_buf",
> > +                                            info.pkt_len - info.l2_len,
> > +                                            RTE_CACHE_LINE_SIZE);
> 
> Rather than call a dyn allocation in datapath, can't we have a static buffer on
> the stack?

I wanted to do that. But the issue only happens when it's a large packet. Each hw has its own limitation on max packet size but it grows fast.
I'm not sure how large array should I use. 64K? Since total length in IP hdr is 16 bit.

BRs
Xiaoyun

> 
> 
> > +                       rte_pktmbuf_read(m, info.l2_len,
> > +                                        info.pkt_len - info.l2_len, l3_buf);
> > +                       l3_hdr = l3_buf;
> > +               } else
> > +                       l3_hdr = (char *)eth_hdr + info.l2_len;
> >
> >                 /* check if it's a supported tunnel */
> >                 if (txp->parse_tunnel) { @@ -1051,6 +1065,7 @@
> > pkt_burst_checksum_forward(struct fwd_stream *fs)
> >                         printf("tx: flags=%s", buf);
> >                         printf("\n");
> >                 }
> > +               rte_free(l3_buf);
> >         }
> >
> >         if (unlikely(gro_enable)) {
> > --
> > 2.25.1
> >
> 
> 
> --
> David Marchand

[dpdk-stable] [PATCH v2] app/testpmd: fix l4 sw csum over multi segments

[Xiaoyun Li]

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.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
---
v2:
 * Use static stack memory instead of dynamic allocating in datapath.
---
 app/test-pmd/csumonly.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 090797318a..9f78ac74e1 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -56,6 +56,11 @@
 #define GRE_SUPPORTED_FIELDS	(GRE_CHECKSUM_PRESENT | GRE_KEY_PRESENT |\
 				 GRE_SEQUENCE_PRESENT)
 
+/* When UDP or TCP or outer UDP csum offload is off, sw l4 csum is needed */
+#define UDP_TCP_CSUM            (DEV_TX_OFFLOAD_UDP_CKSUM |\
+				 DEV_TX_OFFLOAD_TCP_CKSUM |\
+				 DEV_TX_OFFLOAD_OUTER_UDP_CKSUM)
+
 /* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
 #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
 #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
@@ -602,12 +607,8 @@ 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(outer_l3_hdr,
+					udp_hdr, info->outer_ethertype);
 	}
 
 	return ol_flags;
@@ -877,7 +878,17 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
 				&eth_hdr->src_addr);
 		parse_ethernet(eth_hdr, &info);
-		l3_hdr = (char *)eth_hdr + info.l2_len;
+		/* When sw csum is needed, multi-segs needs a buf to contain
+		 * the whole packet for later UDP/TCP csum calculation.
+		 */
+		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
+		    !(tx_offloads & UDP_TCP_CSUM)) {
+			char l3_buf[RTE_IPV4_MAX_PKT_LEN + 1];
+			rte_pktmbuf_read(m, info.l2_len,
+					 info.pkt_len - info.l2_len, l3_buf);
+			l3_hdr = l3_buf;
+		} else
+			l3_hdr = (char *)eth_hdr + info.l2_len;
 
 		/* check if it's a supported tunnel */
 		if (txp->parse_tunnel) {
-- 
2.25.1

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Stephen Hemminger]

On Fri, 15 Oct 2021 13:13:06 +0800
Xiaoyun Li <xiaoyun.li@intel.com> wrote:

> +		/* When sw csum is needed, multi-segs needs a buf to contain
> +		 * the whole packet for later UDP/TCP csum calculation.
> +		 */
> +		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> +		    !(tx_offloads & UDP_TCP_CSUM)) {
> +			l3_buf = rte_zmalloc("csum l3_buf",
> +					     info.pkt_len - info.l2_len,
> +					     RTE_CACHE_LINE_SIZE);
> +			rte_pktmbuf_read(m, info.l2_len,
> +					 info.pkt_len - info.l2_len, l3_buf);
> +			l3_hdr = l3_buf;
> +		} else
> +			l3_hdr = (char *)eth_hdr + info.l2_len;
>  

Rather than copying whole packet, make the code handle checksum streaming.

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, October 18, 2021 11:00
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi
> segments
> 
> On Fri, 15 Oct 2021 13:13:06 +0800
> Xiaoyun Li <xiaoyun.li@intel.com> wrote:
> 
> > +		/* When sw csum is needed, multi-segs needs a buf to contain
> > +		 * the whole packet for later UDP/TCP csum calculation.
> > +		 */
> > +		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> > +		    !(tx_offloads & UDP_TCP_CSUM)) {
> > +			l3_buf = rte_zmalloc("csum l3_buf",
> > +					     info.pkt_len - info.l2_len,
> > +					     RTE_CACHE_LINE_SIZE);
> > +			rte_pktmbuf_read(m, info.l2_len,
> > +					 info.pkt_len - info.l2_len, l3_buf);
> > +			l3_hdr = l3_buf;
> > +		} else
> > +			l3_hdr = (char *)eth_hdr + info.l2_len;
> >
> 
> Rather than copying whole packet, make the code handle checksum streaming.

Copying is the easiest way to do this.

The problem of handling checksum streaming is that in the first segment, l2 and l3 hdr len is 14 bytes when checksum takes 4 bytes each time.
If the datalen of the first segment is 4 bytes aligned (usual case), for the second segment and the following segments, they may need to add a special 2 bytes 0x0 at the start.
Also, mbuf is not passed down to process_inner/outer_chksum so the change will be a lot.

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Li, Xiaoyun]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
> Sent: Monday, October 18, 2021 11:17
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi
> segments
> 
> Hi
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Monday, October 18, 2021 11:00
> > To: Li, Xiaoyun <xiaoyun.li@intel.com>
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org;
> > stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi
> > segments
> >
> > On Fri, 15 Oct 2021 13:13:06 +0800
> > Xiaoyun Li <xiaoyun.li@intel.com> wrote:
> >
> > > +		/* When sw csum is needed, multi-segs needs a buf to contain
> > > +		 * the whole packet for later UDP/TCP csum calculation.
> > > +		 */
> > > +		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> > > +		    !(tx_offloads & UDP_TCP_CSUM)) {
> > > +			l3_buf = rte_zmalloc("csum l3_buf",
> > > +					     info.pkt_len - info.l2_len,
> > > +					     RTE_CACHE_LINE_SIZE);
> > > +			rte_pktmbuf_read(m, info.l2_len,
> > > +					 info.pkt_len - info.l2_len, l3_buf);
> > > +			l3_hdr = l3_buf;
> > > +		} else
> > > +			l3_hdr = (char *)eth_hdr + info.l2_len;
> > >
> >
> > Rather than copying whole packet, make the code handle checksum streaming.
> 
> Copying is the easiest way to do this.
> 
> The problem of handling checksum streaming is that in the first segment, l2 and
> l3 hdr len is 14 bytes when checksum takes 4 bytes each time.
> If the datalen of the first segment is 4 bytes aligned (usual case), for the second
> segment and the following segments, they may need to add a special 2 bytes
> 0x0 at the start.
> Also, mbuf is not passed down to process_inner/outer_chksum so the change
> will be a lot.

Also, rte_ipv4/6_udptcp_cksum can't be directly called. Because it only takes ip_hdr and whole packet buffer as input.

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Ananyev, Konstantin]

> > > +		/* When sw csum is needed, multi-segs needs a buf to contain
> > > +		 * the whole packet for later UDP/TCP csum calculation.
> > > +		 */
> > > +		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> > > +		    !(tx_offloads & UDP_TCP_CSUM)) {
> > > +			l3_buf = rte_zmalloc("csum l3_buf",
> > > +					     info.pkt_len - info.l2_len,
> > > +					     RTE_CACHE_LINE_SIZE);
> > > +			rte_pktmbuf_read(m, info.l2_len,
> > > +					 info.pkt_len - info.l2_len, l3_buf);
> > > +			l3_hdr = l3_buf;
> > > +		} else
> > > +			l3_hdr = (char *)eth_hdr + info.l2_len;
> > >
> >
> > Rather than copying whole packet, make the code handle checksum streaming.
> 
> Copying is the easiest way to do this.
> 
> The problem of handling checksum streaming is that in the first segment, l2 and l3 hdr len is 14 bytes when checksum takes 4 bytes each
> time.
> If the datalen of the first segment is 4 bytes aligned (usual case), for the second segment and the following segments, they may need to add
> a special 2 bytes 0x0 at the start.

Didn't understand that one...
Why you suddenly need to pad non-first segments with zeroes?
Why simply rte_raw_cksum() can't be used for multi-seg case?

> Also, mbuf is not passed down to process_inner/outer_chksum so the change will be a lot.

I also think that copying whole packet just to calculate a checksum - way too much overhead. 

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi segments

[Li, Xiaoyun]

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, October 18, 2021 18:16
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix l4 sw csum over multi
> segments
> 
> 
> > > > +		/* When sw csum is needed, multi-segs needs a buf to contain
> > > > +		 * the whole packet for later UDP/TCP csum calculation.
> > > > +		 */
> > > > +		if (m->nb_segs > 1 && !(tx_ol_flags & PKT_TX_TCP_SEG) &&
> > > > +		    !(tx_offloads & UDP_TCP_CSUM)) {
> > > > +			l3_buf = rte_zmalloc("csum l3_buf",
> > > > +					     info.pkt_len - info.l2_len,
> > > > +					     RTE_CACHE_LINE_SIZE);
> > > > +			rte_pktmbuf_read(m, info.l2_len,
> > > > +					 info.pkt_len - info.l2_len, l3_buf);
> > > > +			l3_hdr = l3_buf;
> > > > +		} else
> > > > +			l3_hdr = (char *)eth_hdr + info.l2_len;
> > > >
> > >
> > > Rather than copying whole packet, make the code handle checksum
> streaming.
> >
> > Copying is the easiest way to do this.
> >
> > The problem of handling checksum streaming is that in the first
> > segment, l2 and l3 hdr len is 14 bytes when checksum takes 4 bytes each time.
> > If the datalen of the first segment is 4 bytes aligned (usual case),
> > for the second segment and the following segments, they may need to add a
> special 2 bytes 0x0 at the start.
> 
> Didn't understand that one...
> Why you suddenly need to pad non-first segments with zeroes?
> Why simply rte_raw_cksum() can't be used for multi-seg case?

Normal udp/tcp packets:
The first segment: eth hdr + ip hdr + udp/tcp packet (The total length of this is mbuf data len so like 2048, 4 bytes aligned)
The second segment: continue udp/tcp packet

Now, udp/tcp checksum is calculated. It will take the whole udp/tcp packet. 4 bytes + 4 bytes + 4 bytes...
Then
1st segment: udp/tcp packet (size = 2048 - 14 = 2034, not 4 bytes aligned, 2 bytes left, if use rte_raw_cksum(), the last 2 bytes will be combined with 2 bytes zeros)
2nd segment: continue udp/tcp packet (size = data_len)

For 2nd segment, if don't add 2 bytes zeros first, the checksum value will be wrong.
Because it should be for example 0x1234 (0x12 is left in 1st, 0x34 is in 2nd), 0x1200+0x0034 is correct but 0x1200+0x3400 is not correct.

That's why I think all of the following segments needs zero padding first.

And above is only the usual case of normal tcp/udp packets. The issue also exists for tunnel packets which will calculate outer udp and inner udp/tcp checksum.

> 
> > Also, mbuf is not passed down to process_inner/outer_chksum so the change
> will be a lot.
> 
> I also think that copying whole packet just to calculate a checksum - way too
> much overhead.

Yes. I agree. But it only happens when users don't enable checksum offload, don't enable TSO and the packet crosses multi-segments.

[dpdk-stable] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

[Xiaoyun Li]

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.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
---
v3:
 * Use rte_raw_cksum() for multi-segs case instead of copying the whole
 * packet.
v2:
 * Use static stack memory instead of dynamic allocating in datapath
---
 app/test-pmd/csumonly.c | 68 ++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 15 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 090797318a..f3e60eb3c3 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -91,12 +91,41 @@ struct simple_gre_hdr {
 } __rte_packed;
 
 static uint16_t
-get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
+get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t l4_off,
+		    uint16_t ethertype)
 {
+	uint16_t off = l4_off;
+	uint32_t cksum = 0;
+	char *buf;
+
+	while (m != NULL) {
+		buf = rte_pktmbuf_mtod_offset(m, char *, off);
+		cksum += rte_raw_cksum(buf, m->data_len - off);
+		off = 0;
+		m = m->next;
+	}
 	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
-		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
 	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
-		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
+
+	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
+	cksum = (~cksum) & 0xffff;
+
+	/*
+	 * Per RFC 768:If the computed checksum is zero for UDP,
+	 * it is transmitted as all ones
+	 * (the equivalent in one's complement arithmetic).
+	 */
+	if (cksum == 0 && ethertype == _htons(RTE_ETHER_TYPE_IPV4) &&
+	    ((struct rte_ipv4_hdr *)l3_hdr)->next_proto_id == IPPROTO_UDP)
+		cksum = 0xffff;
+
+	if (cksum == 0 && ethertype == _htons(RTE_ETHER_TYPE_IPV6) &&
+	    ((struct rte_ipv6_hdr *)l3_hdr)->proto == IPPROTO_UDP)
+		cksum = 0xffff;
+
+	return (uint16_t)cksum;
 }
 
 /* Parse an IPv4 header to fill l3_len, l4_len, and l4_proto */
@@ -455,7 +484,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;
@@ -463,6 +492,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) {
@@ -505,9 +535,15 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 			if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
 				ol_flags |= PKT_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(l3_hdr, m, l4_off,
 						info->ethertype);
 			}
 		}
@@ -520,9 +556,14 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
 			ol_flags |= PKT_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(l3_hdr, m, l4_off,
 					info->ethertype);
 		}
 		if (info->gso_enable)
@@ -548,7 +589,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;
@@ -602,12 +643,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(outer_l3_hdr,
+					m, info->l2_len + info->outer_l3_len,
+					info->outer_ethertype);
 	}
 
 	return ol_flags;
@@ -942,7 +980,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
@@ -950,7 +988,7 @@ 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 & PKT_TX_TCP_SEG));
+					!!(tx_ol_flags & PKT_TX_TCP_SEG), m);
 		}
 
 		/* step 3: fill the mbuf meta data (flags and header lengths) */
-- 
2.25.1

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

[Ferruh Yigit]

On 10/20/2021 11:12 AM, 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.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> ---
> v3:
>   * Use rte_raw_cksum() for multi-segs case instead of copying the whole
>   * packet.
> v2:
>   * Use static stack memory instead of dynamic allocating in datapath
> ---
>   app/test-pmd/csumonly.c | 68 ++++++++++++++++++++++++++++++++---------
>   1 file changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 090797318a..f3e60eb3c3 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -91,12 +91,41 @@ struct simple_gre_hdr {
>   } __rte_packed;
>   
>   static uint16_t
> -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
> +get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t l4_off,
> +		    uint16_t ethertype)
>   {
> +	uint16_t off = l4_off;
> +	uint32_t cksum = 0;
> +	char *buf;
> +
> +	while (m != NULL) {
> +		buf = rte_pktmbuf_mtod_offset(m, char *, off);
> +		cksum += rte_raw_cksum(buf, m->data_len - off);
> +		off = 0;
> +		m = m->next;
> +	}
>   	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> -		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> +		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
>   	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> -		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> +		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
> +

Hi Xiaoyun,

I can see 'rte_ipv[46]_udptcp_cksum()' is not taking multi segment mbuf
into account, so this fix is required,
but instead of implementing this logic into testpmd, what do you think
to have APIs to support multi segment mbufs?
This way other applications also benefit from it and we don't need to
maintain ip4/6 checksum related code in testpmd.

btw, how are you testing this?

Re: [dpdk-stable] [dpdk-dev] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Wednesday, 27 October 2021 12.49
> 
> On 10/20/2021 11:12 AM, 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.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > ---
> > v3:
> >   * Use rte_raw_cksum() for multi-segs case instead of copying the
> whole
> >   * packet.
> > v2:
> >   * Use static stack memory instead of dynamic allocating in datapath
> > ---
> >   app/test-pmd/csumonly.c | 68 ++++++++++++++++++++++++++++++++------
> ---
> >   1 file changed, 53 insertions(+), 15 deletions(-)
> >
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > index 090797318a..f3e60eb3c3 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -91,12 +91,41 @@ struct simple_gre_hdr {
> >   } __rte_packed;
> >
> >   static uint16_t
> > -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
> > +get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t
> l4_off,
> > +		    uint16_t ethertype)
> >   {
> > +	uint16_t off = l4_off;
> > +	uint32_t cksum = 0;
> > +	char *buf;
> > +
> > +	while (m != NULL) {
> > +		buf = rte_pktmbuf_mtod_offset(m, char *, off);
> > +		cksum += rte_raw_cksum(buf, m->data_len - off);
> > +		off = 0;
> > +		m = m->next;
> > +	}
> >   	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> > -		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > +		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
> >   	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> > -		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > +		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
> > +
> 
> Hi Xiaoyun,
> 
> I can see 'rte_ipv[46]_udptcp_cksum()' is not taking multi segment mbuf
> into account, so this fix is required,
> but instead of implementing this logic into testpmd, what do you think
> to have APIs to support multi segment mbufs?
> This way other applications also benefit from it and we don't need to
> maintain ip4/6 checksum related code in testpmd.

+1

Also, there is no need to implement the multi-segment raw checksum loop in test-pmd.

You can use the multi-segment raw checksum function in the net library instead:
http://code.dpdk.org/dpdk/latest/source/lib/net/rte_ip.h#L224

> 
> btw, how are you testing this?

Re: [dpdk-stable] [dpdk-dev] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

[Olivier Matz]

On Wed, Oct 27, 2021 at 01:29:52PM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > Sent: Wednesday, 27 October 2021 12.49
> > 
> > On 10/20/2021 11:12 AM, 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.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > > ---
> > > v3:
> > >   * Use rte_raw_cksum() for multi-segs case instead of copying the
> > whole
> > >   * packet.
> > > v2:
> > >   * Use static stack memory instead of dynamic allocating in datapath
> > > ---
> > >   app/test-pmd/csumonly.c | 68 ++++++++++++++++++++++++++++++++------
> > ---
> > >   1 file changed, 53 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > > index 090797318a..f3e60eb3c3 100644
> > > --- a/app/test-pmd/csumonly.c
> > > +++ b/app/test-pmd/csumonly.c
> > > @@ -91,12 +91,41 @@ struct simple_gre_hdr {
> > >   } __rte_packed;
> > >
> > >   static uint16_t
> > > -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
> > > +get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t
> > l4_off,
> > > +		    uint16_t ethertype)
> > >   {
> > > +	uint16_t off = l4_off;
> > > +	uint32_t cksum = 0;
> > > +	char *buf;
> > > +
> > > +	while (m != NULL) {
> > > +		buf = rte_pktmbuf_mtod_offset(m, char *, off);
> > > +		cksum += rte_raw_cksum(buf, m->data_len - off);
> > > +		off = 0;
> > > +		m = m->next;
> > > +	}
> > >   	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> > > -		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > > +		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
> > >   	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> > > -		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > > +		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
> > > +
> > 
> > Hi Xiaoyun,
> > 
> > I can see 'rte_ipv[46]_udptcp_cksum()' is not taking multi segment mbuf
> > into account, so this fix is required,
> > but instead of implementing this logic into testpmd, what do you think
> > to have APIs to support multi segment mbufs?
> > This way other applications also benefit from it and we don't need to
> > maintain ip4/6 checksum related code in testpmd.
> 
> +1
> 
> Also, there is no need to implement the multi-segment raw checksum loop in test-pmd.
> 
> You can use the multi-segment raw checksum function in the net library instead:
> http://code.dpdk.org/dpdk/latest/source/lib/net/rte_ip.h#L224

+1

We can have mbuf variants of udptcp checksum functions:

rte_ipv4_udptcp_cksum()
rte_ipv4_udptcp_cksum_verify()
rte_ipv6_udptcp_cksum()
rte_ipv6_udptcp_cksum_verify()

Adding a "_mbuf" suffix would be consistent with rte_raw_cksum_mbuf().


Olivier

RE: [dpdk-dev] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Friday, October 29, 2021 09:29
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org; dev@dpdk.org; stable@dpdk.org;
> Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3] app/testpmd: fix l4 sw csum over multi
> segments
> 
> On Wed, Oct 27, 2021 at 01:29:52PM +0200, Morten Brørup wrote:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > > Sent: Wednesday, 27 October 2021 12.49
> > >
> > > On 10/20/2021 11:12 AM, 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.
> > > >
> > > > Fixes: af75078fece3 ("first public release")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > > > ---
> > > > v3:
> > > >   * Use rte_raw_cksum() for multi-segs case instead of copying the
> > > whole
> > > >   * packet.
> > > > v2:
> > > >   * Use static stack memory instead of dynamic allocating in
> > > > datapath
> > > > ---
> > > >   app/test-pmd/csumonly.c | 68
> > > > ++++++++++++++++++++++++++++++++------
> > > ---
> > > >   1 file changed, 53 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > > > index 090797318a..f3e60eb3c3 100644
> > > > --- a/app/test-pmd/csumonly.c
> > > > +++ b/app/test-pmd/csumonly.c
> > > > @@ -91,12 +91,41 @@ struct simple_gre_hdr {
> > > >   } __rte_packed;
> > > >
> > > >   static uint16_t
> > > > -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t
> > > > ethertype)
> > > > +get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t
> > > l4_off,
> > > > +		    uint16_t ethertype)
> > > >   {
> > > > +	uint16_t off = l4_off;
> > > > +	uint32_t cksum = 0;
> > > > +	char *buf;
> > > > +
> > > > +	while (m != NULL) {
> > > > +		buf = rte_pktmbuf_mtod_offset(m, char *, off);
> > > > +		cksum += rte_raw_cksum(buf, m->data_len - off);
> > > > +		off = 0;
> > > > +		m = m->next;
> > > > +	}
> > > >   	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> > > > -		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > > > +		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
> > > >   	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> > > > -		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > > > +		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
> > > > +
> > >
> > > Hi Xiaoyun,
> > >
> > > I can see 'rte_ipv[46]_udptcp_cksum()' is not taking multi segment
> > > mbuf into account, so this fix is required, but instead of
> > > implementing this logic into testpmd, what do you think to have APIs
> > > to support multi segment mbufs?
> > > This way other applications also benefit from it and we don't need
> > > to maintain ip4/6 checksum related code in testpmd.
> >
> > +1
> >
> > Also, there is no need to implement the multi-segment raw checksum loop
> in test-pmd.
> >
> > You can use the multi-segment raw checksum function in the net library
> instead:
> > http://code.dpdk.org/dpdk/latest/source/lib/net/rte_ip.h#L224
> 
> +1
> 
> We can have mbuf variants of udptcp checksum functions:
> 
> rte_ipv4_udptcp_cksum()
> rte_ipv4_udptcp_cksum_verify()
> rte_ipv6_udptcp_cksum()
> rte_ipv6_udptcp_cksum_verify()
> 
> Adding a "_mbuf" suffix would be consistent with rte_raw_cksum_mbuf().

Thanks for the suggestion. Since it's API change, I'll send these in 22.03 release with release note.
V4 will come soon.

> 
> 
> Olivier
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

[dpdk-stable] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

[Xiaoyun Li]

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.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
---
v3:
 * Use rte_raw_cksum() for multi-segs case instead of copying the whole
 * packet.
v2:
 * Use static stack memory instead of dynamic allocating in datapath
---
 app/test-pmd/csumonly.c | 68 ++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 15 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 090797318a..f3e60eb3c3 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -91,12 +91,41 @@ struct simple_gre_hdr {
 } __rte_packed;
 
 static uint16_t
-get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t ethertype)
+get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t l4_off,
+		    uint16_t ethertype)
 {
+	uint16_t off = l4_off;
+	uint32_t cksum = 0;
+	char *buf;
+
+	while (m != NULL) {
+		buf = rte_pktmbuf_mtod_offset(m, char *, off);
+		cksum += rte_raw_cksum(buf, m->data_len - off);
+		off = 0;
+		m = m->next;
+	}
 	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
-		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
 	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
-		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
+
+	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
+	cksum = (~cksum) & 0xffff;
+
+	/*
+	 * Per RFC 768:If the computed checksum is zero for UDP,
+	 * it is transmitted as all ones
+	 * (the equivalent in one's complement arithmetic).
+	 */
+	if (cksum == 0 && ethertype == _htons(RTE_ETHER_TYPE_IPV4) &&
+	    ((struct rte_ipv4_hdr *)l3_hdr)->next_proto_id == IPPROTO_UDP)
+		cksum = 0xffff;
+
+	if (cksum == 0 && ethertype == _htons(RTE_ETHER_TYPE_IPV6) &&
+	    ((struct rte_ipv6_hdr *)l3_hdr)->proto == IPPROTO_UDP)
+		cksum = 0xffff;
+
+	return (uint16_t)cksum;
 }
 
 /* Parse an IPv4 header to fill l3_len, l4_len, and l4_proto */
@@ -455,7 +484,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;
@@ -463,6 +492,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) {
@@ -505,9 +535,15 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 			if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
 				ol_flags |= PKT_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(l3_hdr, m, l4_off,
 						info->ethertype);
 			}
 		}
@@ -520,9 +556,14 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
 			ol_flags |= PKT_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(l3_hdr, m, l4_off,
 					info->ethertype);
 		}
 		if (info->gso_enable)
@@ -548,7 +589,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;
@@ -602,12 +643,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(outer_l3_hdr,
+					m, info->l2_len + info->outer_l3_len,
+					info->outer_ethertype);
 	}
 
 	return ol_flags;
@@ -942,7 +980,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
@@ -950,7 +988,7 @@ 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 & PKT_TX_TCP_SEG));
+					!!(tx_ol_flags & PKT_TX_TCP_SEG), m);
 		}
 
 		/* step 3: fill the mbuf meta data (flags and header lengths) */
-- 
2.25.1