[dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Hongzhi Guo]

Per RFC768:
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.

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

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

Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
---
v2:
* Fixed commit tile
* Fixed the API comment
---
---
 lib/librte_net/rte_ip.h | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 292f63fd7..d03c77120 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
  *   The pointer to the beginning of the L4 header.
  * @return
  *   The complemented checksum to set in the IP packet
- *   or 0 on error
+ *   or 0 if the IP length is invalid in the header.
  */
 static inline uint16_t
 rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
@@ -344,7 +344,13 @@ 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)
+	/*
+	 *Per RFC768:
+	 *If the computed checksum is zero for udp,
+	 *it is transmitted as all ones.
+	 *(the equivalent in one's complement arithmetic).
+	 */
+	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
 		cksum = 0xffff;
 
 	return (uint16_t)cksum;
@@ -438,7 +444,13 @@ 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)
+	/*
+	 *Per RFC768:
+	 *If the computed checksum is zero for udp,
+	 *it is transmitted as all ones.
+	 *(the equivalent in one's complement arithmetic).
+	 */
+	if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP)
 		cksum = 0xffff;
 
 	return (uint16_t)cksum;
-- 
2.21.0.windows.1

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Olivier Matz]

On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> Per RFC768:
> 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.
> 
> RFC793 for TCP has no such special treatment for the checksum of zero.
> 
> Fixes: 6006818cfb26 ("net: new checksum functions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> ---
> v2:
> * Fixed commit tile
> * Fixed the API comment
> ---
> ---
>  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 292f63fd7..d03c77120 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
>   *   The pointer to the beginning of the L4 header.
>   * @return
>   *   The complemented checksum to set in the IP packet
> - *   or 0 on error
> + *   or 0 if the IP length is invalid in the header.
>   */
>  static inline uint16_t
>  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
> @@ -344,7 +344,13 @@ 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)
> +	/*
> +	 *Per RFC768:
> +	 *If the computed checksum is zero for udp,
> +	 *it is transmitted as all ones.
> +	 *(the equivalent in one's complement arithmetic).
> +	 */

There should be a space after the '*', and maybe it could be on
less lines.

Thomas, maybe you can do it when applying?


> +	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
>  		cksum = 0xffff;
>  
>  	return (uint16_t)cksum;
> @@ -438,7 +444,13 @@ 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)
> +	/*
> +	 *Per RFC768:
> +	 *If the computed checksum is zero for udp,
> +	 *it is transmitted as all ones.
> +	 *(the equivalent in one's complement arithmetic).
> +	 */

Same here

> +	if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP)
>  		cksum = 0xffff;
>  
>  	return (uint16_t)cksum;
> -- 
> 2.21.0.windows.1
> 
> 

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

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Morten Brørup]

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, July 10, 2020 2:41 PM
> 
> On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > Per RFC768:
> > 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.
> >
> > RFC793 for TCP has no such special treatment for the checksum of
> zero.
> >
> > Fixes: 6006818cfb26 ("net: new checksum functions")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > ---
> > v2:
> > * Fixed commit tile
> > * Fixed the API comment
> > ---
> > ---
> >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > index 292f63fd7..d03c77120 100644
> > --- a/lib/librte_net/rte_ip.h
> > +++ b/lib/librte_net/rte_ip.h
> > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr
> *ipv4_hdr, uint64_t ol_flags)
> >   *   The pointer to the beginning of the L4 header.
> >   * @return
> >   *   The complemented checksum to set in the IP packet
> > - *   or 0 on error
> > + *   or 0 if the IP length is invalid in the header.
> >   */
> >  static inline uint16_t
> >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const
> void *l4_hdr)

0 is a valid return value, so I suggest omitting it from the return value description:

  * @return
- *   The complemented checksum to set in the IP packet
- *   or 0 on error
+ *   The complemented checksum to set in the IP packet.

The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only there to protect against invalid input; it prevents l4_len from becoming negative.

For the same reason, unlikely() should be added to this comparison.

Otherwise,

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Olivier Matz]

On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Friday, July 10, 2020 2:41 PM
> > 
> > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > Per RFC768:
> > > 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.
> > >
> > > RFC793 for TCP has no such special treatment for the checksum of
> > zero.
> > >
> > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > ---
> > > v2:
> > > * Fixed commit tile
> > > * Fixed the API comment
> > > ---
> > > ---
> > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > > index 292f63fd7..d03c77120 100644
> > > --- a/lib/librte_net/rte_ip.h
> > > +++ b/lib/librte_net/rte_ip.h
> > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr
> > *ipv4_hdr, uint64_t ol_flags)
> > >   *   The pointer to the beginning of the L4 header.
> > >   * @return
> > >   *   The complemented checksum to set in the IP packet
> > > - *   or 0 on error
> > > + *   or 0 if the IP length is invalid in the header.
> > >   */
> > >  static inline uint16_t
> > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const
> > void *l4_hdr)
> 
> 0 is a valid return value, so I suggest omitting it from the return value description:
> 
>   * @return
> - *   The complemented checksum to set in the IP packet
> - *   or 0 on error
> + *   The complemented checksum to set in the IP packet.
> 
> The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only there to protect against invalid input; it prevents l4_len from becoming negative.

I don't get why "0 if the IP length is invalid in the header" should
be removed from the comment: 0 is both a valid return value and
the value returned on invalid packet.

> For the same reason, unlikely() should be added to this comparison.

Maybe yes, but that's another story I think.

> Otherwise,
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Morten Brørup]

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, July 10, 2020 3:16 PM
> 
> On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Friday, July 10, 2020 2:41 PM
> > >
> > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > Per RFC768:
> > > > 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.
> > > >
> > > > RFC793 for TCP has no such special treatment for the checksum of
> > > zero.
> > > >
> > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > > ---
> > > > v2:
> > > > * Fixed commit tile
> > > > * Fixed the API comment
> > > > ---
> > > > ---
> > > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > > > index 292f63fd7..d03c77120 100644
> > > > --- a/lib/librte_net/rte_ip.h
> > > > +++ b/lib/librte_net/rte_ip.h
> > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr
> > > *ipv4_hdr, uint64_t ol_flags)
> > > >   *   The pointer to the beginning of the L4 header.
> > > >   * @return
> > > >   *   The complemented checksum to set in the IP packet
> > > > - *   or 0 on error
> > > > + *   or 0 if the IP length is invalid in the header.
> > > >   */
> > > >  static inline uint16_t
> > > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const
> > > void *l4_hdr)
> >
> > 0 is a valid return value, so I suggest omitting it from the return
> value description:
> >
> >   * @return
> > - *   The complemented checksum to set in the IP packet
> > - *   or 0 on error
> > + *   The complemented checksum to set in the IP packet.
> >
> > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only
> there to protect against invalid input; it prevents l4_len from
> becoming negative.
> 
> I don't get why "0 if the IP length is invalid in the header" should
> be removed from the comment: 0 is both a valid return value and
> the value returned on invalid packet.

To avoid confusion. We do not want people to add error handling for a return value of 0.

0 is not a special value or an error, so it does not deserve explicit mentioning.

If we want to mention the return value for garbage input, we should not use the wording "or 0", because this suggests that 0 is not a normal return value.

> 
> > For the same reason, unlikely() should be added to this comparison.
> 
> Maybe yes, but that's another story I think.

Agree. I was just mentioning it so it can be done when modifying the function anyway.

> 
> > Otherwise,
> >
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Olivier Matz]

On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote:
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Friday, July 10, 2020 3:16 PM
> > 
> > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > Sent: Friday, July 10, 2020 2:41 PM
> > > >
> > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > > Per RFC768:
> > > > > 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.
> > > > >
> > > > > RFC793 for TCP has no such special treatment for the checksum of
> > > > zero.
> > > > >
> > > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > > > ---
> > > > > v2:
> > > > > * Fixed commit tile
> > > > > * Fixed the API comment
> > > > > ---
> > > > > ---
> > > > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > > > > index 292f63fd7..d03c77120 100644
> > > > > --- a/lib/librte_net/rte_ip.h
> > > > > +++ b/lib/librte_net/rte_ip.h
> > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr
> > > > *ipv4_hdr, uint64_t ol_flags)
> > > > >   *   The pointer to the beginning of the L4 header.
> > > > >   * @return
> > > > >   *   The complemented checksum to set in the IP packet
> > > > > - *   or 0 on error
> > > > > + *   or 0 if the IP length is invalid in the header.
> > > > >   */
> > > > >  static inline uint16_t
> > > > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const
> > > > void *l4_hdr)
> > >
> > > 0 is a valid return value, so I suggest omitting it from the return
> > value description:
> > >
> > >   * @return
> > > - *   The complemented checksum to set in the IP packet
> > > - *   or 0 on error
> > > + *   The complemented checksum to set in the IP packet.
> > >
> > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only
> > there to protect against invalid input; it prevents l4_len from
> > becoming negative.
> > 
> > I don't get why "0 if the IP length is invalid in the header" should
> > be removed from the comment: 0 is both a valid return value and
> > the value returned on invalid packet.
> 
> To avoid confusion. We do not want people to add error handling for a return value of 0.
> 
> 0 is not a special value or an error, so it does not deserve explicit mentioning.
> 
> If we want to mention the return value for garbage input, we should not use the wording "or 0", because this suggests that 0 is not a normal return value.

Ok, got it.

So maybe this?

 The complemented checksum to set in the IP packet. If
 the IP length is invalid in the header, it returns 0.


> 
> > 
> > > For the same reason, unlikely() should be added to this comparison.
> > 
> > Maybe yes, but that's another story I think.
> 
> Agree. I was just mentioning it so it can be done when modifying the function anyway.
> 
> > 
> > > Otherwise,
> > >
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >
> 

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Friday, July 10, 2020 3:41 PM
> 
> On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote:
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Friday, July 10, 2020 3:16 PM
> > >
> > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > Sent: Friday, July 10, 2020 2:41 PM
> > > > >
> > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > > > Per RFC768:
> > > > > > 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.
> > > > > >
> > > > > > RFC793 for TCP has no such special treatment for the checksum
> of
> > > > > zero.
> > > > > >
> > > > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > > > > ---
> > > > > > v2:
> > > > > > * Fixed commit tile
> > > > > > * Fixed the API comment
> > > > > > ---
> > > > > > ---
> > > > > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > > > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/librte_net/rte_ip.h
> b/lib/librte_net/rte_ip.h
> > > > > > index 292f63fd7..d03c77120 100644
> > > > > > --- a/lib/librte_net/rte_ip.h
> > > > > > +++ b/lib/librte_net/rte_ip.h
> > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct
> rte_ipv4_hdr
> > > > > *ipv4_hdr, uint64_t ol_flags)
> > > > > >   *   The pointer to the beginning of the L4 header.
> > > > > >   * @return
> > > > > >   *   The complemented checksum to set in the IP packet
> > > > > > - *   or 0 on error
> > > > > > + *   or 0 if the IP length is invalid in the header.
> > > > > >   */
> > > > > >  static inline uint16_t
> > > > > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr,
> const
> > > > > void *l4_hdr)
> > > >
> > > > 0 is a valid return value, so I suggest omitting it from the
> return
> > > value description:
> > > >
> > > >   * @return
> > > > - *   The complemented checksum to set in the IP packet
> > > > - *   or 0 on error
> > > > + *   The complemented checksum to set in the IP packet.
> > > >
> > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is
> only
> > > there to protect against invalid input; it prevents l4_len from
> > > becoming negative.
> > >
> > > I don't get why "0 if the IP length is invalid in the header"
> should
> > > be removed from the comment: 0 is both a valid return value and
> > > the value returned on invalid packet.
> >
> > To avoid confusion. We do not want people to add error handling for a
> return value of 0.
> >
> > 0 is not a special value or an error, so it does not deserve explicit
> mentioning.
> >
> > If we want to mention the return value for garbage input, we should
> not use the wording "or 0", because this suggests that 0 is not a
> normal return value.
> 
> Ok, got it.
> 
> So maybe this?
> 
>  The complemented checksum to set in the IP packet. If
>  the IP length is invalid in the header, it returns 0.
> 
It still mentions 0 as a special value, increasing the risk of the defensive user adding "error handling" for a return value of 0.

How about this?

 The complemented checksum to set in the IP packet. If
 the IP length is invalid in the header, the return value
 is undefined.

> 
> >
> > >
> > > > For the same reason, unlikely() should be added to this
> comparison.
> > >
> > > Maybe yes, but that's another story I think.
> >
> > Agree. I was just mentioning it so it can be done when modifying the
> function anyway.
> >
> > >
> > > > Otherwise,
> > > >
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > >
> >

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Olivier Matz]

On Fri, Jul 10, 2020 at 03:56:11PM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Sent: Friday, July 10, 2020 3:41 PM
> > 
> > On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote:
> > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > Sent: Friday, July 10, 2020 3:16 PM
> > > >
> > > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > > Sent: Friday, July 10, 2020 2:41 PM
> > > > > >
> > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > > > > Per RFC768:
> > > > > > > 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.
> > > > > > >
> > > > > > > RFC793 for TCP has no such special treatment for the checksum
> > of
> > > > > > zero.
> > > > > > >
> > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > > > > Cc: stable@dpdk.org
> > > > > > >
> > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > > > > > ---
> > > > > > > v2:
> > > > > > > * Fixed commit tile
> > > > > > > * Fixed the API comment
> > > > > > > ---
> > > > > > > ---
> > > > > > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > > > > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/librte_net/rte_ip.h
> > b/lib/librte_net/rte_ip.h
> > > > > > > index 292f63fd7..d03c77120 100644
> > > > > > > --- a/lib/librte_net/rte_ip.h
> > > > > > > +++ b/lib/librte_net/rte_ip.h
> > > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct
> > rte_ipv4_hdr
> > > > > > *ipv4_hdr, uint64_t ol_flags)
> > > > > > >   *   The pointer to the beginning of the L4 header.
> > > > > > >   * @return
> > > > > > >   *   The complemented checksum to set in the IP packet
> > > > > > > - *   or 0 on error
> > > > > > > + *   or 0 if the IP length is invalid in the header.
> > > > > > >   */
> > > > > > >  static inline uint16_t
> > > > > > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr,
> > const
> > > > > > void *l4_hdr)
> > > > >
> > > > > 0 is a valid return value, so I suggest omitting it from the
> > return
> > > > value description:
> > > > >
> > > > >   * @return
> > > > > - *   The complemented checksum to set in the IP packet
> > > > > - *   or 0 on error
> > > > > + *   The complemented checksum to set in the IP packet.
> > > > >
> > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is
> > only
> > > > there to protect against invalid input; it prevents l4_len from
> > > > becoming negative.
> > > >
> > > > I don't get why "0 if the IP length is invalid in the header"
> > should
> > > > be removed from the comment: 0 is both a valid return value and
> > > > the value returned on invalid packet.
> > >
> > > To avoid confusion. We do not want people to add error handling for a
> > return value of 0.
> > >
> > > 0 is not a special value or an error, so it does not deserve explicit
> > mentioning.
> > >
> > > If we want to mention the return value for garbage input, we should
> > not use the wording "or 0", because this suggests that 0 is not a
> > normal return value.
> > 
> > Ok, got it.
> > 
> > So maybe this?
> > 
> >  The complemented checksum to set in the IP packet. If
> >  the IP length is invalid in the header, it returns 0.
> > 
> It still mentions 0 as a special value, increasing the risk of the defensive user adding "error handling" for a return value of 0.
> 
> How about this?
> 
>  The complemented checksum to set in the IP packet. If
>  the IP length is invalid in the header, the return value
>  is undefined.

After reading again your arguments, I think I prefer your first
proposal, which was also Hongzhi's initial submission:

   - *   The complemented checksum to set in the IP packet
   - *   or 0 on error
   + *   The complemented checksum to set in the IP packet.

Thomas, do you want to to resubmit with this change?

> > 
> > >
> > > >
> > > > > For the same reason, unlikely() should be added to this
> > comparison.
> > > >
> > > > Maybe yes, but that's another story I think.
> > >
> > > Agree. I was just mentioning it so it can be done when modifying the
> > function anyway.
> > >
> > > >
> > > > > Otherwise,
> > > > >
> > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > >
> > >
> 

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Olivier Matz]

On Fri, Jul 10, 2020 at 04:40:59PM +0200, Olivier Matz wrote:
> On Fri, Jul 10, 2020 at 03:56:11PM +0200, Morten Brørup wrote:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > > Sent: Friday, July 10, 2020 3:41 PM
> > > 
> > > On Fri, Jul 10, 2020 at 03:29:36PM +0200, Morten Brørup wrote:
> > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > Sent: Friday, July 10, 2020 3:16 PM
> > > > >
> > > > > On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > > > Sent: Friday, July 10, 2020 2:41 PM
> > > > > > >
> > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > > > > > Per RFC768:
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > RFC793 for TCP has no such special treatment for the checksum
> > > of
> > > > > > > zero.
> > > > > > > >
> > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > > > > > Cc: stable@dpdk.org
> > > > > > > >
> > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > > > > > > ---
> > > > > > > > v2:
> > > > > > > > * Fixed commit tile
> > > > > > > > * Fixed the API comment
> > > > > > > > ---
> > > > > > > > ---
> > > > > > > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > > > > > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/librte_net/rte_ip.h
> > > b/lib/librte_net/rte_ip.h
> > > > > > > > index 292f63fd7..d03c77120 100644
> > > > > > > > --- a/lib/librte_net/rte_ip.h
> > > > > > > > +++ b/lib/librte_net/rte_ip.h
> > > > > > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct
> > > rte_ipv4_hdr
> > > > > > > *ipv4_hdr, uint64_t ol_flags)
> > > > > > > >   *   The pointer to the beginning of the L4 header.
> > > > > > > >   * @return
> > > > > > > >   *   The complemented checksum to set in the IP packet
> > > > > > > > - *   or 0 on error
> > > > > > > > + *   or 0 if the IP length is invalid in the header.
> > > > > > > >   */
> > > > > > > >  static inline uint16_t
> > > > > > > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr,
> > > const
> > > > > > > void *l4_hdr)
> > > > > >
> > > > > > 0 is a valid return value, so I suggest omitting it from the
> > > return
> > > > > value description:
> > > > > >
> > > > > >   * @return
> > > > > > - *   The complemented checksum to set in the IP packet
> > > > > > - *   or 0 on error
> > > > > > + *   The complemented checksum to set in the IP packet.
> > > > > >
> > > > > > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is
> > > only
> > > > > there to protect against invalid input; it prevents l4_len from
> > > > > becoming negative.
> > > > >
> > > > > I don't get why "0 if the IP length is invalid in the header"
> > > should
> > > > > be removed from the comment: 0 is both a valid return value and
> > > > > the value returned on invalid packet.
> > > >
> > > > To avoid confusion. We do not want people to add error handling for a
> > > return value of 0.
> > > >
> > > > 0 is not a special value or an error, so it does not deserve explicit
> > > mentioning.
> > > >
> > > > If we want to mention the return value for garbage input, we should
> > > not use the wording "or 0", because this suggests that 0 is not a
> > > normal return value.
> > > 
> > > Ok, got it.
> > > 
> > > So maybe this?
> > > 
> > >  The complemented checksum to set in the IP packet. If
> > >  the IP length is invalid in the header, it returns 0.
> > > 
> > It still mentions 0 as a special value, increasing the risk of the defensive user adding "error handling" for a return value of 0.
> > 
> > How about this?
> > 
> >  The complemented checksum to set in the IP packet. If
> >  the IP length is invalid in the header, the return value
> >  is undefined.
> 
> After reading again your arguments, I think I prefer your first
> proposal, which was also Hongzhi's initial submission:
> 
>    - *   The complemented checksum to set in the IP packet
>    - *   or 0 on error
>    + *   The complemented checksum to set in the IP packet.
> 
> Thomas, do you want to to resubmit with this change?

Sorry, I meant "do you want me to resubmit?"

> 
> > > 
> > > >
> > > > >
> > > > > > For the same reason, unlikely() should be added to this
> > > comparison.
> > > > >
> > > > > Maybe yes, but that's another story I think.
> > > >
> > > > Agree. I was just mentioning it so it can be done when modifying the
> > > function anyway.
> > > >
> > > > >
> > > > > > Otherwise,
> > > > > >
> > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > >
> > > >
> > 

Re: [dpdk-dev] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

[Thomas Monjalon]

10/07/2020 16:40, Olivier Matz:
> > > > > > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > > > > > Per RFC768:
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > RFC793 for TCP has no such special treatment for the checksum
> > > of
> > > > > > > zero.
> > > > > > > >
> > > > > > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > > > > > Cc: stable@dpdk.org
> > > > > > > >
> > > > > > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
[...]
> After reading again your arguments, I think I prefer your first
> proposal, which was also Hongzhi's initial submission:
> 
>    - *   The complemented checksum to set in the IP packet
>    - *   or 0 on error
>    + *   The complemented checksum to set in the IP packet.
> 
> Thomas, do you want to to resubmit with this change?

Applied with all changes in comments.