Re: [PATCH 02/14] Add csum_icmp4() helper for calculating ICMPv4 checksums

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 17 Oct 2022 19:57:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Although tap_ip_send() is currently the only place calculating ICMPv4
> checksums, create a helper function for symmetry with ICMPv6.  For future
> flexibility it allows the ICMPv6 header and payload to be in separate
> buffers.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  checksum.c | 15 +++++++++++++++
>  checksum.h |  2 ++
>  tap.c      |  4 +---
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/checksum.c b/checksum.c
> index 0e207c8..c8b6b42 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -52,6 +52,7 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  
> +#include <linux/icmp.h>
>  #include <linux/icmpv6.h>
>  
>  /**
> @@ -107,6 +108,20 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
>  	return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
>  }
>  
> +/**
> + * csum_icmp4() - Calculate checksum for an ICMPv4 packet

"Calculate and set"? By the way, there's no such thing as ICMPv4 --
it's ICMP.

> + * @icmp4hr:	ICMPv4 header, initialized apart from checksum

...-ised, if you respin. For consistency, I would call this 'ih'.

> + * @payload:	ICMPv4 packet payload
> + * @len:	Length of @payload (not including ICMPv4 header)
> + */
> +void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)

I guess csum_icmp() is preferable. Indeed, for TCP and UDP 'tcp4' and
'udp4' make sense because those are the same protocols over IPv4 and
IPv6.

> +{
> +	/* Partial checksum for ICMPv4 header alone */
> +	uint32_t hrsum = sum_16b(icmp4hr, sizeof(*icmp4hr));

A white line would be nice here.

I would call this psum (same as in csum_icmp6()) or hdrsum, 'hr' isn't
really used for "header" elsewhere.

> +	icmp4hr->checksum = 0;
> +	icmp4hr->checksum = csum_unaligned(payload, len, hrsum);
> +}
> +
>  /**
>   * csum_icmp6() - Calculate checksum for an ICMPv6 packet
>   * @icmp6hr:	ICMPv6 header, initialized apart from checksum
> diff --git a/checksum.h b/checksum.h
> index 2c72200..ff95cf9 100644
> --- a/checksum.h
> +++ b/checksum.h
> @@ -6,11 +6,13 @@
>  #ifndef CHECKSUM_H
>  #define CHECKSUM_H
>  
> +struct icmphdr;
>  struct icmp6hdr;
>  
>  uint32_t sum_16b(const void *buf, size_t len);
>  uint16_t csum_fold(uint32_t sum);
>  uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init);
> +void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len);
>  void csum_icmp6(struct icmp6hdr *ih,
>  		const struct in6_addr *saddr,
>  		const struct in6_addr *daddr,
> diff --git a/tap.c b/tap.c
> index aafc92b..f082901 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -148,9 +148,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto,
>  			uh->check = 0;
>  		} else if (iph->protocol == IPPROTO_ICMP) {
>  			struct icmphdr *ih = (struct icmphdr *)(iph + 1);
> -
> -			ih->checksum = 0;
> -			ih->checksum = csum_unaligned(ih, len, 0);
> +			csum_icmp4(ih, ih + 1, len - sizeof(*ih));
>  		}
>  
>  		if (tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh), 1) < 0)

-- 
Stefano