Re: [PATCH v3 7/9] checksum: introduce functions to compute the header part checksum for TCP/UDP

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6650 bytes --]

On Wed, Feb 28, 2024 at 02:26:18PM +0100, Laurent Vivier wrote:
> On 2/19/24 04:08, David Gibson wrote:
> > On Sat, Feb 17, 2024 at 04:07:23PM +0100, Laurent Vivier wrote:
> > > The TCP and UDP checksums are computed using the data in the TCP/UDP
> > > payload but also some informations in the IP header (protocol,
> > > length, source and destination addresses).
> > > 
> > > We add two functions, proto_ipv4_header_psum() and
> > > proto_ipv6_header_psum(), to compute the checksum of the IP
> > > header part.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > > 
> > > Notes:
> > >      v3:
> > >        - function parameters provide tot_len, saddr, daddr and protocol
> > >          rather than an iphdr/ipv6hdr
> > >      v2:
> > >        - move new function to checksum.c
> > >        - use _psum rather than _checksum in the name
> > >        - replace csum_udp4() and csum_udp6() by the new function
> > > 
> > >   checksum.c | 67 ++++++++++++++++++++++++++++++++++++++++++------------
> > >   checksum.h |  4 ++++
> > >   tcp.c      | 44 ++++++++++++++++-------------------
> > >   udp.c      | 10 ++++----
> > >   4 files changed, 81 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/checksum.c b/checksum.c
> > > index 511b296a9a80..55bf1340a257 100644
> > > --- a/checksum.c
> > > +++ b/checksum.c
> > > @@ -134,6 +134,30 @@ uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
> > >   	return ~csum_fold(sum);
> > >   }
> > > +/**
> > > + * proto_ipv4_header_psum() - Calculates the partial checksum of an
> > > + * 			      IPv4 header for UDP or TCP
> > > + * @tot_len:	Payload length
> > > + * @proto:	Protocol number
> > > + * @saddr:	Source address
> > > + * @daddr:	Destination address
> > > + * @proto:	proto Protocol number
> > > + * Returns:	Partial checksum of the IPv4 header
> > > + */
> > > +uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol,
> > > +				uint32_t saddr, uint32_t daddr)
> > > +{
> > > +	uint32_t psum = htons(protocol);
> > > +
> > > +	psum += (saddr >> 16) & 0xffff;
> > > +	psum += saddr & 0xffff;
> > > +	psum += (daddr >> 16) & 0xffff;
> > > +	psum += daddr & 0xffff;
> > > +	psum += htons(ntohs(tot_len) - 20);
> > > +
> > > +	return psum;
> > > +}
> > > +
> > >   /**
> > >    * csum_udp4() - Calculate and set checksum for a UDP over IPv4 packet
> > >    * @udp4hr:	UDP header, initialised apart from checksum
> > > @@ -150,14 +174,10 @@ void csum_udp4(struct udphdr *udp4hr,
> > >   	udp4hr->check = 0;
> > >   	if (UDP4_REAL_CHECKSUMS) {
> > > -		/* UNTESTED: if we did want real UDPv4 checksums, this
> > > -		 * is roughly what we'd need */
> > > -		uint32_t psum = csum_fold(saddr.s_addr)
> > > -			+ csum_fold(daddr.s_addr)
> > > -			+ htons(len + sizeof(*udp4hr))
> > > -			+ htons(IPPROTO_UDP);
> > > -		/* Add in partial checksum for the UDP header alone */
> > > -		psum += sum_16b(udp4hr, sizeof(*udp4hr));
> > > +		uint32_t psum = proto_ipv4_header_psum(len, IPPROTO_UDP,
> > > +						       saddr.s_addr,
> > > +						       daddr.s_addr);
> > > +		psum = csum_unfolded(udp4hr, sizeof(struct udphdr), psum);
> > >   		udp4hr->check = csum(payload, len, psum);
> > >   	}
> > >   }
> > > @@ -180,6 +200,26 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)
> > >   	icmp4hr->checksum = csum(payload, len, psum);
> > >   }
> > > +/**
> > > + * proto_ipv6_header_psum() - Calculates the partial checksum of an
> > > + * 			      IPv6 header for UDP or TCP
> > > + * @payload_len:	Payload length
> > > + * @proto:		Protocol number
> > > + * @saddr:		Source address
> > > + * @daddr:		Destination address
> > > + * Returns:	Partial checksum of the IPv6 header
> > > + */
> > > +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol,
> > > +				struct in6_addr saddr, struct in6_addr daddr)
> > 
> > Hrm, this is passing 2 16-byte IPv6 addresses by value, which might
> > not be what we want.
> 
> The idea here is to avoid the pointer alignment problem (&ip6h->saddr and
> &ip6h->daddr can be misaligned).

Ah, right.  That's a neat idea, but I'm not sure it really helps: I
think it will just move the misaligned access from inside the function
to the call site, where we try to marshal the parameter from something
unaligned.

> Is it a better solution to copy the content of ip6h->saddr and ip6h->daddr
> to some local variables and then provide the pointers of the local variables
> to proto_ipv6_header_psum()?

Honestly, I'm not sure.

> > > +{
> > > +	uint32_t sum = htons(protocol) + payload_len;
> > 
> > Uh.. doesn't that need to be htons(payload_len + sizeof(struct
> > ipv6hdr)) rather than simply payload_len?
> > 
> 
> payload_len is:
> 
> - b->ip6h.payload_len (from udp_update_hdr6())
> - ip6h->payload_len (from tcp_update_check_tcp6())
> 
> and in ip6h payload_len is:
> 
> - htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh)) (from udp_update_hdr6())
> - htons(plen + sizeof(struct tcphdr)) (from tcp_fill_ipv6_header())
> 
> So this is correct... but

Ah, right.  Not sure why I thought the ip6h length needed to be
included.

As a rule htons(x) + y is always suspect, because you generally can't
do math on values that aren't host endian.  We get away with it in
these csum functions because the way they're folded means the answers
end up the same - as long as we're consistent about it, anyway.

> csum_udp6() uses "len" from tap_udp6_send(), so there is a bug here.
> 
> but there is also a problem with proto_ipv4_header_psum() that need host
> endianness and tcp_update_check_tcp4() provides network endianness...
> 
> The first idea was to use the value from ip6h payload_len as it is already computed.
> But mixing network endianness and host endianness appears to be a bad idea...

Right.  As a rule I really dislike putting non-host-endian values in a
plain u16 local or parameter, because it's really easy to think it's
just a number, rather than a funny encoding of a number.  Likewise, I
think it's a lot easier to keep track of things if every field of a
struct has a strict endianness, which we never change in place, even
temporarily. (Ideally, in fact, I'd prefer to see non-host-endian
values always in an encapsulating type that won't let you do math on
them, but that's not always practical in C).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]