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

[Laurent Vivier <lvivier@redhat.com>]

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).

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()?

> 
>> +{
>> +	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

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...

Thanks,
Laurent