Re: [PATCH 01/14] Add csum_icmp6() helper for calculating ICMPv6 checksums

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

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

On Tue, Oct 18, 2022 at 05:01:01AM +0200, Stefano Brivio wrote:
> On Mon, 17 Oct 2022 19:57:54 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > At least two places in passt calculate ICMPv6 checksums, ndp() and
> > tap_ip_send().  Add a helper to handle this calculation in both places.
> > For future flexibility, the new helper takes parameters for the fields in
> > the IPv6 pseudo-header, so an IPv6 header or pseudo-header doesn't need to
> > be explicitly constructed.  It also allows the ICMPv6 header and payload to
> > be in separate buffers, although we don't use this yet.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  checksum.c | 27 +++++++++++++++++++++++++++
> >  checksum.h |  7 +++++++
> >  ndp.c      |  5 +----
> >  tap.c      |  6 ++----
> >  4 files changed, 37 insertions(+), 8 deletions(-)
> > 
> > diff --git a/checksum.c b/checksum.c
> > index 56ad01e..0e207c8 100644
> > --- a/checksum.c
> > +++ b/checksum.c
> > @@ -52,6 +52,8 @@
> >  #include <stddef.h>
> >  #include <stdint.h>
> >  
> > +#include <linux/icmpv6.h>
> > +
> >  /**
> >   * sum_16b() - Calculate sum of 16-bit words
> >   * @buf:	Input buffer
> > @@ -105,6 +107,31 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
> >  	return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
> >  }
> >  
> > +/**
> > + * csum_icmp6() - Calculate checksum for an ICMPv6 packet
> 
> "Calculate and set" ...?

Done.

> > + * @icmp6hr:	ICMPv6 header, initialized apart from checksum
> > + * @saddr:	IPv6 source address
> > + * @daddr:	IPv6 destination address
> > + * @payload:	ICMP packet payload
> > + * @len:	Length of @payload (not including ICMPv6 header)
> > + */
> > +void csum_icmp6(struct icmp6hdr *icmp6hr,
> > +		const struct in6_addr *saddr,
> > +		const struct in6_addr *daddr,
> 
> I think:
> 		const struct in6_addr *saddr, const struct in6_addr *daddr,
> 
> would be easier on eyes.

Done.  Not sure why I did it that way in the first place.

> > +		const void *payload,
> > +		size_t len)
> > +{
> > +	/* Partial checksum for the pseudo-IPv6 header */
> > +	uint32_t psum = sum_16b(saddr, sizeof(*saddr)) +
> > +		sum_16b(daddr, sizeof(*daddr)) +
> > +		htons(len + sizeof(*icmp6hr)) + htons(IPPROTO_ICMPV6);
> 
> Maybe:
> 
> 	uint32_t psum = sum_16b(saddr, sizeof(*saddr)) +
> 			sum_16b(daddr, sizeof(*daddr)) +
> 			htons(len + sizeof(*icmp6hr))  + htons(IPPROTO_ICMPV6);
> 
> for me, it turns things from "sum a bunch of things" to "addresses and
> something else".

Fair enough, done.

> > +
> > +	icmp6hr->icmp6_cksum = 0;
> > +	/* Add in partial checksum for the ICMPv6 header alone */
> > +	psum += sum_16b(icmp6hr, sizeof(*icmp6hr));
> > +	icmp6hr->icmp6_cksum = csum_unaligned(payload, len, psum);
> > +}
> > +
> >  /**
> >   * csum_tcp4() - Calculate TCP checksum for IPv4 and set in place
> >   * @iph:	Packet buffer, IP header
> > diff --git a/checksum.h b/checksum.h
> > index 5418406..2c72200 100644
> > --- a/checksum.h
> > +++ b/checksum.h
> > @@ -6,9 +6,16 @@
> >  #ifndef CHECKSUM_H
> >  #define CHECKSUM_H
> >  
> > +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_icmp6(struct icmp6hdr *ih,
> > +		const struct in6_addr *saddr,
> > +		const struct in6_addr *daddr,
> > +		const void *payload,
> > +		size_t len);
> 
> It looks a bit like Haskell. ;) I would really use the horizontal space
> we have.
> 
> >  void csum_tcp4(struct iphdr *iph);
> >  uint16_t csum(const void *buf, size_t len, uint32_t init);
> >  
> > diff --git a/ndp.c b/ndp.c
> > index dec36a9..03f1d06 100644
> > --- a/ndp.c
> > +++ b/ndp.c
> > @@ -189,10 +189,7 @@ dns_done:
> >  		ip6hr->saddr = c->ip6.addr_ll;
> >  
> >  	ip6hr->payload_len = htons(sizeof(*ihr) + len);
> > -	ip6hr->hop_limit = IPPROTO_ICMPV6;
> > -	ihr->icmp6_cksum = 0;
> > -	ihr->icmp6_cksum = csum_unaligned(ip6hr, sizeof(*ip6hr) +
> > -						 sizeof(*ihr) + len, 0);
> > +	csum_icmp6(ihr, &ip6hr->saddr, &ip6hr->daddr, ihr + 1, len);
> 
> Nice to see this all going away!
> 
> >  	ip6hr->version = 6;
> >  	ip6hr->nexthdr = IPPROTO_ICMPV6;
> > diff --git a/tap.c b/tap.c
> > index 8b6d9bc..aafc92b 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -191,10 +191,8 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto,
> >  		} else if (proto == IPPROTO_ICMPV6) {
> >  			struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1);
> >  
> > -			ih->icmp6_cksum = 0;
> > -			ih->icmp6_cksum = csum_unaligned(ip6h,
> > -							 len + sizeof(*ip6h),
> > -							 0);
> > +			csum_icmp6(ih, &ip6h->saddr, &ip6h->daddr,
> > +				   ih + 1, len - sizeof(*ih));
> >  		}
> >  		ip6h->version = 6;
> >  		ip6h->nexthdr = proto;
> 

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