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: sta...@dpdk.org
On Wed, May 27, 2020 at 05:36:59PM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-boun...@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 <guohongz...@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.
