[dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[guohongzhi]

The function of rte_ipv4_cksum for calculating the
checksum of IPv4 header is incorrect.
This function will return checksum value like 0xffff.
This value, however, is considered an illegal checksum on some switches(like Trident3).

RFC 1624 specifies the IPv4 checksum as follows:
https://tools.ietf.org/rfc/rfc1624
Since there is guaranteed to be at least one
   non-zero field in the IP header, and the checksum field in the
   protocol header is the complement of the sum, the checksum field can
   never contain ~(+0), which is -0 (0xFFFF).  It can, however, contain
   ~(-0), which is +0 (0x0000).

---
 lib/librte_net/rte_ip.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 1ceb7b7..ece2e43 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
 {
 	uint16_t cksum;
 	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
-	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
+	return (uint16_t)~cksum;
 }
 
 /**
-- 
2.21.0.windows.1

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksumcalculating

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of guohongzhi
> Sent: Thursday, May 14, 2020 3:27 AM
> 
> The function of rte_ipv4_cksum for calculating the
> checksum of IPv4 header is incorrect.
> This function will return checksum value like 0xffff.
> This value, however, is considered an illegal checksum on some
> switches(like Trident3).
> 
> RFC 1624 specifies the IPv4 checksum as follows:
> https://tools.ietf.org/rfc/rfc1624
> Since there is guaranteed to be at least one
>    non-zero field in the IP header, and the checksum field in the
>    protocol header is the complement of the sum, the checksum field can
>    never contain ~(+0), which is -0 (0xFFFF).  It can, however, contain
>    ~(-0), which is +0 (0x0000).
> 
> ---
>  lib/librte_net/rte_ip.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 1ceb7b7..ece2e43 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
>  {
>  	uint16_t cksum;
>  	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> -	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
> +	return (uint16_t)~cksum;
>  }
> 
>  /**
> --
> 2.21.0.windows.1
> 
> 

Well spotted!

Reviewed-By: Morten Brørup <mb@smartsharesystems.com>


Would you consider writing another patch splitting rte_ipv4_udptcp_cksum() up into rte_ipv4_udp_cksum() and rte_ipv4_tcp_cksum(), so the TCP checksum will be calculated correctly?

RFC 768 for UDP specifies:

If the computed  checksum  is zero,  it is transmitted  as all ones (the equivalent  in one's complement  arithmetic).   An all zero  transmitted checksum  value means that the transmitter  generated  no checksum  (for debugging or for higher level protocols that don't care).

RFC 793 for TCP has no such special treatment for the checksum of zero, but rte_ipv4_udptcp_cksum() implements the UDP special treatment anyway.

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksumcalculating

[Olivier Matz]

Hi,

On Thu, May 14, 2020 at 02:56:41PM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of guohongzhi
> > Sent: Thursday, May 14, 2020 3:27 AM
> > 
> > The function of rte_ipv4_cksum for calculating the
> > checksum of IPv4 header is incorrect.
> > This function will return checksum value like 0xffff.
> > This value, however, is considered an illegal checksum on some
> > switches(like Trident3).
> > 
> > RFC 1624 specifies the IPv4 checksum as follows:
> > https://tools.ietf.org/rfc/rfc1624
> > Since there is guaranteed to be at least one
> >    non-zero field in the IP header, and the checksum field in the
> >    protocol header is the complement of the sum, the checksum field can
> >    never contain ~(+0), which is -0 (0xFFFF).  It can, however, contain
> >    ~(-0), which is +0 (0x0000).
> > 
> > ---
> >  lib/librte_net/rte_ip.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > index 1ceb7b7..ece2e43 100644
> > --- a/lib/librte_net/rte_ip.h
> > +++ b/lib/librte_net/rte_ip.h
> > @@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
> >  {
> >  	uint16_t cksum;
> >  	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> > -	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
> > +	return (uint16_t)~cksum;
> >  }
> > 
> >  /**
> > --
> > 2.21.0.windows.1
> > 
> > 
> 
> Well spotted!

Indeed.

> Reviewed-By: Morten Brørup <mb@smartsharesystems.com>

Fixes: 6006818cfb26 ("net: new checksum functions")
Cc: stable@dpdk.org

Acked-by: Olivier Matz <olivier.matz@6wind.com>

> Would you consider writing another patch splitting
> rte_ipv4_udptcp_cksum() up into rte_ipv4_udp_cksum() and
> rte_ipv4_tcp_cksum(), so the TCP checksum will be calculated
> correctly?
> 
> RFC 768 for UDP specifies:
> 
> If the computed checksum is zero, it is transmitted as all ones (the
> equivalent in one's complement arithmetic).  An all zero transmitted
> checksum value means that the transmitter generated no checksum (for
> debugging or for higher level protocols that don't care).
>
> RFC 793 for TCP has no such special treatment for the checksum of
> zero, but rte_ipv4_udptcp_cksum() implements the UDP special treatment
> anyway.

I agree the following test is useless in case of TCPv4 and TCPv6:

	if (cksum == 0)
                cksum =	0xffff;

For UDPv4, it is needed because 0 means "no checksum".
For UDPv6, it is needed because 0 is forbidden.

So yes, I think we could have specific csum functions for tcp and udp
checksum as Morten suggests (as soon as we keep the backward compat).

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksumcalculating

[guohongzhi (A)]

Ok, later I will write a patch to solve the problem of tcpdump checksum

-----Original Message-----
From: Morten Brørup [mailto:mb@smartsharesystems.com] 
Sent: Thursday,May 14,2020 20:57
To: guohongzhi (A) <guohongzhi1@huawei.com>; dev@dpdk.org
Cc: olivier.matz@6wind.com; konstantin.ananyev@intel.com; jiayu.hu@intel.com; ferruh.yigit@intel.com; nicolas.chautru@intel.com; cristian.dumitrescu@intel.com; Zhoujingbin (Robin, Russell Lab) <zhoujingbin@huawei.com>; chenchanghu <chenchanghu@huawei.com>; Lilijun (Jerry) <jerry.lilijun@huawei.com>; Linhaifeng <haifeng.lin@huawei.com>
Subject: RE: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksumcalculating

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of guohongzhi
> Sent: Thursday, May 14, 2020 3:27 AM
> 
> The function of rte_ipv4_cksum for calculating the checksum of IPv4 
> header is incorrect.
> This function will return checksum value like 0xffff.
> This value, however, is considered an illegal checksum on some 
> switches(like Trident3).
> 
> RFC 1624 specifies the IPv4 checksum as follows:
> https://tools.ietf.org/rfc/rfc1624
> Since there is guaranteed to be at least one
>    non-zero field in the IP header, and the checksum field in the
>    protocol header is the complement of the sum, the checksum field can
>    never contain ~(+0), which is -0 (0xFFFF).  It can, however, contain
>    ~(-0), which is +0 (0x0000).
> 
> ---
>  lib/librte_net/rte_ip.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index 
> 1ceb7b7..ece2e43 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr 
> *ipv4_hdr)  {
>  	uint16_t cksum;
>  	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> -	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
> +	return (uint16_t)~cksum;
>  }
> 
>  /**
> --
> 2.21.0.windows.1
> 
> 

Well spotted!

Reviewed-By: Morten Brørup <mb@smartsharesystems.com>


Would you consider writing another patch splitting rte_ipv4_udptcp_cksum() up into rte_ipv4_udp_cksum() and rte_ipv4_tcp_cksum(), so the TCP checksum will be calculated correctly?

RFC 768 for UDP specifies:

If the computed  checksum  is zero,  it is transmitted  as all ones (the equivalent  in one's complement  arithmetic).   An all zero  transmitted checksum  value means that the transmitter  generated  no checksum  (for debugging or for higher level protocols that don't care).

RFC 793 for TCP has no such special treatment for the checksum of zero, but rte_ipv4_udptcp_cksum() implements the UDP special treatment anyway.

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksumcalculating

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of guohongzhi (A)
> Sent: Friday, May 15, 2020 3:05 AM
> 
> Ok, later I will write a patch to solve the problem of tcpdump checksum
> 
> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday,May 14,2020 20:57
> To: guohongzhi (A) <guohongzhi1@huawei.com>; dev@dpdk.org
> Cc: olivier.matz@6wind.com; konstantin.ananyev@intel.com;
> jiayu.hu@intel.com; ferruh.yigit@intel.com; nicolas.chautru@intel.com;
> cristian.dumitrescu@intel.com; Zhoujingbin (Robin, Russell Lab)
> <zhoujingbin@huawei.com>; chenchanghu <chenchanghu@huawei.com>; Lilijun
> (Jerry) <jerry.lilijun@huawei.com>; Linhaifeng <haifeng.lin@huawei.com>
> Subject: RE: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4
> checksumcalculating
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of guohongzhi
> > Sent: Thursday, May 14, 2020 3:27 AM
> >
> > The function of rte_ipv4_cksum for calculating the checksum of IPv4
> > header is incorrect.
> > This function will return checksum value like 0xffff.
> > This value, however, is considered an illegal checksum on some
> > switches(like Trident3).
> >
> > RFC 1624 specifies the IPv4 checksum as follows:
> > https://tools.ietf.org/rfc/rfc1624
> > Since there is guaranteed to be at least one
> >    non-zero field in the IP header, and the checksum field in the
> >    protocol header is the complement of the sum, the checksum field
> can
> >    never contain ~(+0), which is -0 (0xFFFF).  It can, however,
> contain
> >    ~(-0), which is +0 (0x0000).
> >
> > ---
> >  lib/librte_net/rte_ip.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index
> > 1ceb7b7..ece2e43 100644
> > --- a/lib/librte_net/rte_ip.h
> > +++ b/lib/librte_net/rte_ip.h
> > @@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr
> > *ipv4_hdr)  {
> >  	uint16_t cksum;
> >  	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> > -	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
> > +	return (uint16_t)~cksum;
> >  }
> >
> >  /**
> > --
> > 2.21.0.windows.1
> >
> >
> 
> Well spotted!
> 
> Reviewed-By: Morten Brørup <mb@smartsharesystems.com>
> 

While you are at it, you could also fix a Big Endian bug in __rte_raw_cksum():

	/* if length is in odd bytes */
	if (len == 1)
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+		sum += *((const uint8_t *)u16_buf) << 8;
+#else
		sum += *((const uint8_t *)u16_buf);
+#endif

	return sum;

> 
> Would you consider writing another patch splitting
> rte_ipv4_udptcp_cksum() up into rte_ipv4_udp_cksum() and
> rte_ipv4_tcp_cksum(), so the TCP checksum will be calculated correctly?
> 
> RFC 768 for UDP specifies:
> 
> If the computed  checksum  is zero,  it is transmitted  as all ones
> (the equivalent  in one's complement  arithmetic).   An all zero
> transmitted checksum  value means that the transmitter  generated  no
> checksum  (for debugging or for higher level protocols that don't
> care).
> 
> RFC 793 for TCP has no such special treatment for the checksum of zero,
> but rte_ipv4_udptcp_cksum() implements the UDP special treatment
> anyway.

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[Thomas Monjalon]

14/05/2020 03:27, guohongzhi:
> The function of rte_ipv4_cksum for calculating the
> checksum of IPv4 header is incorrect.
> This function will return checksum value like 0xffff.
> This value, however, is considered an illegal checksum on some switches(like Trident3).
> 
> RFC 1624 specifies the IPv4 checksum as follows:
> https://tools.ietf.org/rfc/rfc1624
> Since there is guaranteed to be at least one
>    non-zero field in the IP header, and the checksum field in the
>    protocol header is the complement of the sum, the checksum field can
>    never contain ~(+0), which is -0 (0xFFFF).  It can, however, contain
>    ~(-0), which is +0 (0x0000).
> 
> ---
>  lib/librte_net/rte_ip.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Suggested title:
	net: fix IPv4 checksum

Please send a v2 with your full name and add a Signed-off-by line.
You can check the contributing guidelines:
	http://core.dpdk.org/contribute/#send

You need to add these lines from previous reviews:

Fixes: 6006818cfb26 ("net: new checksum functions")
Cc: stable@dpdk.org

Reviewed-By: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>

[dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[guohongzhi]

0xffff is invalid for IPv4 checksum(RFC1624)

Fixes: 6006818cfb26 ("net: new checksum functions")

Reviewed-By: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Signed-off-by: guohongzhi <guohongzhi1@huawei.com>
---
 lib/librte_net/rte_ip.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 1ceb7b7..ece2e43 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
 {
 	uint16_t cksum;
 	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
-	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
+	return (uint16_t)~cksum;
 }
 
 /**
-- 
2.21.0.windows.1

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[Morten Brørup]

guohongzhi,

You must use your real name in the Signed-off-by line.

The signoff must be a real name and not an alias or nickname. For further details, please refer to: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1


Med venlig hilsen / kind regards
- Morten Brørup

> -----Original Message-----
> From: guohongzhi [mailto:guohongzhi1@huawei.com]
> Sent: Tuesday, May 26, 2020 11:17 AM
> To: dev@dpdk.org
> Cc: olivier.matz@6wind.com; Morten Brørup;
> konstantin.ananyev@intel.com; jiayu.hu@intel.com;
> ferruh.yigit@intel.com; nicolas.chautru@intel.com;
> cristian.dumitrescu@intel.com; zhoujingbin@huawei.com;
> chenchanghu@huawei.com; jerry.lilijun@huawei.com;
> haifeng.lin@huawei.com; guohongzhi1@huawei.com
> Subject: [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating
> 
> 0xffff is invalid for IPv4 checksum(RFC1624)
> 
> Fixes: 6006818cfb26 ("net: new checksum functions")
> 
> Reviewed-By: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Signed-off-by: guohongzhi <guohongzhi1@huawei.com>
> ---
>  lib/librte_net/rte_ip.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 1ceb7b7..ece2e43 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
>  {
>  	uint16_t cksum;
>  	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> -	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
> +	return (uint16_t)~cksum;
>  }
> 
>  /**
> --
> 2.21.0.windows.1
> 
> 

[dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[guohongzhi]

From: Hongzhi Guo <guohongzhi1@huawei.com>

0xffff is invalid for IPv4 checksum(RFC1624)

Fixes: 6006818cfb26 ("net: new checksum functions")
Cc: stable@dpdk.org

Reviewed-By: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
---
 lib/librte_net/rte_ip.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 1ceb7b7..ece2e43 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
 {
 	uint16_t cksum;
 	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
-	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
+	return (uint16_t)~cksum;
 }
 
 /**
-- 
2.21.0.windows.1

Re: [dpdk-dev] [PATCH] lib/librte_net: fix bug for ipv4 checksum calculating

[Thomas Monjalon]

26/05/2020 12:08, guohongzhi:
> From: Hongzhi Guo <guohongzhi1@huawei.com>
> 
> 0xffff is invalid for IPv4 checksum(RFC1624)
> 
> Fixes: 6006818cfb26 ("net: new checksum functions")
> Cc: stable@dpdk.org
> 
> Reviewed-By: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>

The Signed-off should be in chronological order, before reviews.

As suggested in previous patch, the title should be
	net: fix IPv4 checksum

I am doing above modifications while merging.

More advices about submitting patches:
	- use -v2, -v3, etc when sending new versions
	- add a changelog below the line ---
	- use --in-reply-to to keep all versions threaded


> @@ -267,7 +267,7 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
>  {
>  	uint16_t cksum;
>  	cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> -	return (cksum == 0xffff) ? cksum : (uint16_t)~cksum;
> +	return (uint16_t)~cksum;
>  }

Applied, thanks