patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently
@ 2020-05-27 14:41 guohongzhi
  2020-05-27 15:02 ` [dpdk-stable] [dpdk-dev] " Stephen Hemminger
  0 siblings, 1 reply; 5+ messages in thread
From: guohongzhi @ 2020-05-27 14:41 UTC (permalink / raw)
  To: dev
  Cc: stable, olivier.matz, mb, konstantin.ananyev, jiayu.hu,
	ferruh.yigit, nicolas.chautru, cristian.dumitrescu, zhoujingbin,
	chenchanghu, jerry.lilijun, haifeng.lin, guohongzhi1

From: Hongzhi Guo <guohongzhi1@huawei.com>

RFC 768 for UDP specifies:
If the computed  checksum  is zero,  it is transmitted  as all ones.
An all zero transmitted checksum  value means that the transmitter
generated  no checksum.

RFC 793 for TCP has no such special treatment for the checksum of zero.

Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
---
 lib/librte_net/rte_ip.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 1ceb7b7..8bef313 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -324,8 +324,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
  * @param l4_hdr
  *   The pointer to the beginning of the L4 header.
  * @return
- *   The complemented checksum to set in the IP packet
- *   or 0 on error
+ *   The complemented checksum to set in the IP packet.
  */
 static inline uint16_t
 rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
@@ -344,7 +343,10 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
 
 	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
 	cksum = (~cksum) & 0xffff;
-	if (cksum == 0)
+	/* For Udp, if the computed checksum is zero,
+	 * it is transmitted as all ones.RFC768
+	 */
+	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
 		cksum = 0xffff;
 
 	return (uint16_t)cksum;
@@ -436,7 +438,10 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr)
 
 	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
 	cksum = (~cksum) & 0xffff;
-	if (cksum == 0)
+	/* For Udp, if the computed checksum is zero,
+	 * it is transmitted as all ones.RFC768
+	 */
+	if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP)
 		cksum = 0xffff;
 
 	return (uint16_t)cksum;
-- 
2.21.0.windows.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently
  2020-05-27 14:41 [dpdk-stable] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently guohongzhi
@ 2020-05-27 15:02 ` " Stephen Hemminger
  2020-05-27 15:36   ` Morten Brørup
  0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2020-05-27 15:02 UTC (permalink / raw)
  To: guohongzhi
  Cc: dev, stable, olivier.matz, mb, konstantin.ananyev, jiayu.hu,
	ferruh.yigit, nicolas.chautru, cristian.dumitrescu, zhoujingbin,
	chenchanghu, jerry.lilijun, haifeng.lin

On Wed, 27 May 2020 22:41:27 +0800
guohongzhi <guohongzhi1@huawei.com> wrote:

> +	/* For Udp, if the computed checksum is zero,
> +	 * it is transmitted as all ones.RFC768
> +	 */
> +	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
>  		cksum = 0xffff;
>  


The comment should be reformatted to be clearer.

Maybe:
	/*
	 * Per RFC768:
	 * If the computed  checksum  is zero,it is transmitted  as all ones
	 * (the equivalent  in one's complement  arithmetic).
	 */

There is no special case required here, it is true for TCP as well.
In TCP it appears 0 is allowed but not generally used. Most implementations
use common checksum for both TCP and UDP

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently
  2020-05-27 15:02 ` [dpdk-stable] [dpdk-dev] " Stephen Hemminger
@ 2020-05-27 15:36   ` Morten Brørup
  2020-07-06  7:58     ` Olivier Matz
  0 siblings, 1 reply; 5+ messages in thread
From: Morten Brørup @ 2020-05-27 15:36 UTC (permalink / raw)
  To: Stephen Hemminger, guohongzhi
  Cc: dev, stable, olivier.matz, konstantin.ananyev, jiayu.hu,
	ferruh.yigit, nicolas.chautru, cristian.dumitrescu, zhoujingbin,
	chenchanghu, jerry.lilijun, haifeng.lin

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, May 27, 2020 5:03 PM
> 
> On Wed, 27 May 2020 22:41:27 +0800
> guohongzhi <guohongzhi1@huawei.com> wrote:
> 
> > +	/* For Udp, if the computed checksum is zero,
> > +	 * it is transmitted as all ones.RFC768
> > +	 */
> > +	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
> >  		cksum = 0xffff;
> >
> 
> 
> The comment should be reformatted to be clearer.
> 
> Maybe:
> 	/*
> 	 * Per RFC768:
> 	 * If the computed  checksum  is zero,it is transmitted  as all
> ones
> 	 * (the equivalent  in one's complement  arithmetic).
> 	 */
> 

But without the double spaces. :-)

> There is no special case required here, it is true for TCP as well.

I disagree. I researched this topic when Hongzhi Guo initially submitted the patches, and have only seen the special exception mentioned in RFC 768 (describing UDP), where a transmitted value of 0x0000 means that the checksum has not been generated. None of the RFCs regarding Internet Checksum or TCP checksum mentions this special case.

> In TCP it appears 0 is allowed but not generally used. Most
> implementations
> use common checksum for both TCP and UDP

Then most implementations are wrong.

Jon Postel famously wrote: "Be liberal in what you accept, and conservative in what you send."

This function is in the transmission path, so we should calculate it correctly.

In the receive path, when checking the checksum as described in RFC 1071, any of the two values (0x0000 or 0xFFFF) in the Checksum field will yield the correct result. Which is why most implementations can get away with doing it wrong.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently
  2020-05-27 15:36   ` Morten Brørup
@ 2020-07-06  7:58     ` Olivier Matz
  0 siblings, 0 replies; 5+ messages in thread
From: Olivier Matz @ 2020-07-06  7:58 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Stephen Hemminger, guohongzhi, dev, stable, konstantin.ananyev,
	jiayu.hu, ferruh.yigit, nicolas.chautru, cristian.dumitrescu,
	zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin

Hi,

Here is a suggestion for the title:

  net: fix uneeded replacement of 0 by ffff for TCP checksum

The commit log looks good to me, but you can add:

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

On Wed, May 27, 2020 at 05:36:59PM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Wednesday, May 27, 2020 5:03 PM
> > 
> > On Wed, 27 May 2020 22:41:27 +0800
> > guohongzhi <guohongzhi1@huawei.com> wrote:
> >
> > > --- a/lib/librte_net/rte_ip.h
> > > +++ b/lib/librte_net/rte_ip.h
> > > @@ -324,8 +324,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
> > >   * @param l4_hdr
> > >   *   The pointer to the beginning of the L4 header.
> > >   * @return
> > > - *   The complemented checksum to set in the IP packet
> > > - *   or 0 on error
> > > + *   The complemented checksum to set in the IP packet.
> > >   */
> > >  static inline uint16_t
> > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)

It still returns 0 when ipv4_hdr->total_length < sizeof(struct rte_ipv4_hdr).
I suggest to keep this case in the API comment, maybe like this:

  The complemented checksum to set in the IP packet
  or 0 if the IP length is invalid in the header.


> > > +	/* For Udp, if the computed checksum is zero,
> > > +	 * it is transmitted as all ones.RFC768
> > > +	 */
> > > +	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
> > >  		cksum = 0xffff;
> > >
> > 
> > 
> > The comment should be reformatted to be clearer.
> > 
> > Maybe:
> > 	/*
> > 	 * Per RFC768:
> > 	 * If the computed  checksum  is zero,it is transmitted  as all
> > ones
> > 	 * (the equivalent  in one's complement  arithmetic).
> > 	 */
> > 
> 
> But without the double spaces. :-)

I agree, Stephen's wording looks better, can you please use it?

> > There is no special case required here, it is true for TCP as well.
> 
> I disagree. I researched this topic when Hongzhi Guo initially submitted the patches, and have only seen the special exception mentioned in RFC 768 (describing UDP), where a transmitted value of 0x0000 means that the checksum has not been generated. None of the RFCs regarding Internet Checksum or TCP checksum mentions this special case.
> 
> > In TCP it appears 0 is allowed but not generally used. Most
> > implementations
> > use common checksum for both TCP and UDP
> 
> Then most implementations are wrong.
> 
> Jon Postel famously wrote: "Be liberal in what you accept, and conservative in what you send."
> 
> This function is in the transmission path, so we should calculate it correctly.
> 
> In the receive path, when checking the checksum as described in RFC 1071, any of the two values (0x0000 or 0xFFFF) in the Checksum field will yield the correct result. Which is why most implementations can get away with doing it wrong.

I agree with Morten, it looks more strict like this.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [dpdk-stable] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently
@ 2020-05-27 14:40 " guohongzhi
  0 siblings, 0 replies; 5+ messages in thread
From: guohongzhi @ 2020-05-27 14:40 UTC (permalink / raw)
  To: dev
  Cc: stable, olivier.matz, mb, konstantin.ananyev, jiayu.hu,
	ferruh.yigit, nicolas.chautru, cristian.dumitrescu, zhoujingbin,
	chenchanghu, jerry.lilijun, haifeng.lin, guohongzhi1

RFC 768 for UDP specifies:
If the computed  checksum  is zero,  it is transmitted  as all ones.
An all zero transmitted checksum  value means that the transmitter
generated  no checksum.

RFC 793 for TCP has no such special treatment for the checksum of zero.

Signed-off-by: guohongzhi <guohongzhi1@huawei.com>
---
 lib/librte_net/rte_ip.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 1ceb7b7..8bef313 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -324,8 +324,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
  * @param l4_hdr
  *   The pointer to the beginning of the L4 header.
  * @return
- *   The complemented checksum to set in the IP packet
- *   or 0 on error
+ *   The complemented checksum to set in the IP packet.
  */
 static inline uint16_t
 rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
@@ -344,7 +343,10 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
 
 	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
 	cksum = (~cksum) & 0xffff;
-	if (cksum == 0)
+	/* For Udp, if the computed checksum is zero,
+	 * it is transmitted as all ones.RFC768
+	 */
+	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
 		cksum = 0xffff;
 
 	return (uint16_t)cksum;
@@ -436,7 +438,10 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr)
 
 	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
 	cksum = (~cksum) & 0xffff;
-	if (cksum == 0)
+	/* For Udp, if the computed checksum is zero,
+	 * it is transmitted as all ones.RFC768
+	 */
+	if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP)
 		cksum = 0xffff;
 
 	return (uint16_t)cksum;
-- 
2.21.0.windows.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 14:41 [dpdk-stable] [PATCH] bugfix: udptcp_checksum should tread tcp and udp differently guohongzhi
2020-05-27 15:02 ` [dpdk-stable] [dpdk-dev] " Stephen Hemminger
2020-05-27 15:36   ` Morten Brørup
2020-07-06  7:58     ` Olivier Matz
  -- strict thread matches above, loose matches on Subject: below --
2020-05-27 14:40 [dpdk-stable] " guohongzhi

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox