RE: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 17 October 2024 21.35
> 
> On Thu, 17 Oct 2024 20:03:13 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Thu, Oct 17, 2024 at 07:15:10PM +0200, Morten Brørup wrote:
> > > > +/**
> > > > + * Process the IPv4 checksum of an IPv4 header without any
> extensions.
> > > > + *
> > > > + * The checksum field does NOT have to be set by the caller, the
> field
> > > > + * is skipped by the calculation.
> > > > + *
> > > > + * @param ipv4_hdr
> > > > + *   The pointer to the contiguous IPv4 header.
> > > > + * @return
> > > > + *   The complemented checksum to set in the IP packet.
> > > > + */
> > > > +__rte_experimental
> > > > +static inline uint16_t
> > > > +rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
> > > > +{
> > > > +	const uint16_t *v16_h;
> > > > +	uint32_t ip_cksum;
> > > > +
> > > > +	/*
> > > > +	 * Compute the sum of successive 16-bit words of the IPv4
> header,
> > > > +	 * skipping the checksum field of the header.
> > > > +	 */
> > > > +	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > > > +	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
> > > > +		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> > > > +
> > > > +	/* reduce 32 bit checksum to 16 bits and complement it */
> > > > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > > > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > > > +	ip_cksum = (~ip_cksum) & 0x0000FFFF;
> > > > +	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;
> > >
> > > The zero exception does not apply to the checksum stored in the IP
> header, only to the checksum in the UDP header.
> > >
> >
> > I was wondering about that, because I didn't see it mentioned
> anywhere in
> > the RFCs I consulted, but on the other hand all the implementations
> in the
> > code seemed to have the check for zero.

Copy-pasted including the bug from the originating code.

> >
> > > > +}
> > >
> > > Besides that, for the series,
> >
> > So, just to confirm, the zero check at the end of the new
> ip_cksum_simple
> > function should be removed and we always return the computed value
> > directly?

Agree.

> 
> Depends on usage.
>   - if the computed value is zero, then 0xffff should be placed in
>     the IP header.

Not for the IP header, no.
For the UDP header, yes.

The Linux kernel doesn't do it to the IP header:
https://elixir.bootlin.com/linux/v6.12-rc3/source/net/ipv4/ip_output.c#L96
https://elixir.bootlin.com/linux/v6.12-rc3/source/lib/checksum.c#L108

>   - often code use ip checksum code to see if incoming checksum is
> good.
>     in that case zero means the checksum is valid.

I don't remember all the details, but when checking the checksum, there's something about adding 0xffff or 0x0000 yields the same result. E.g.

1 + 0xffff = 0x10000, and then 0x1 + 0x0000 = 0x0001.
1 + 0x0000 = 0x0001.

So, when checking, it doesn't matter if the header checksum field is 0xffff or 0x0000.

There are other rules for the UDP header's checksum field, because 0x0000 there has the special meaning that the UDP checksum is not present/valid. (And AFAIR, not for IPv6, because the checksum is not optional for IPv6 UDP packets.)

The above calculation is also the reason why it is safe to substitute 0x0000 by 0xffff in the UDP header; it will still yield the same result when checking the checksum.