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

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

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

On Tue, Oct 18, 2022 at 05:01:51AM +0200, Stefano Brivio wrote:
> 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"?

Done.

> By the way, there's no such thing as ICMPv4 --
> it's ICMP.

Technically, yes, but I kind of wanted to make it clear at a glance
that these are IPv4 specific functions.  I'd also like to avoid the
implication that v4 is the "normal" sort.  I've changed from "ICMPv4"
to "ICMP" in the comments, but I've left the '4's in the various names

> > + * @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.

See above.

> > +{
> > +	/* Partial checksum for ICMPv4 header alone */
> > +	uint32_t hrsum = sum_16b(icmp4hr, sizeof(*icmp4hr));
> 
> A white line would be nice here.

Done.
> 
> I would call this psum (same as in csum_icmp6())

Changed to 'psum'.

> or hdrsum, 'hr' isn't
> really used for "header" elsewhere.

Well.. except as a suffix, 'ihr' etc.

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

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