Re: [PATCH v2 6/8] checksum: use csum_ip4_header() in udp.c and tcp.c

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 14 Feb 2024 09:56:26 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> We can find the same function to compute the IPv4 header
> checksum in tcp.c, udp.c and tap.c
> 
> Use the function defined for tap.c, csum_ip4_header(), but
> with the code used in tcp.c and udp.c as it doesn't need a fully
> initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> 
> Notes:
>     v2:
>       - use csum_ip4_header() from checksum.c
>       - use code from tcp.c and udp.c in csum_ip4_header()
>       - use "const struct iphfr *", check is not updated by the
>         function but by the caller.
> 
>  checksum.c | 16 ++++++++++++----
>  checksum.h |  2 +-
>  tap.c      |  2 +-
>  tcp.c      | 22 +---------------------
>  udp.c      | 23 +++++------------------
>  5 files changed, 20 insertions(+), 45 deletions(-)
> 
> diff --git a/checksum.c b/checksum.c
> index ac2bc49f7eb0..5613187a1c82 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -57,6 +57,7 @@
>  #include <linux/icmpv6.h>
>  
>  #include "util.h"
> +#include "ip.h"
>  
>  /* Checksums are optional for UDP over IPv4, so we usually just set
>   * them to 0.  Change this to 1 to calculate real UDP over IPv4
> @@ -115,13 +116,20 @@ uint16_t csum_fold(uint32_t sum)
>  uint16_t csum(const void *buf, size_t len, uint32_t init);
>  
>  /**
> - * csum_ip4_header() - Calculate and set IPv4 header checksum
> + * csum_ip4_header() - Calculate IPv4 header checksum
>   * @ip4h:	IPv4 header
>   */
> -void csum_ip4_header(struct iphdr *ip4h)
> +uint16_t csum_ip4_header(const struct iphdr *ip4h)
>  {
> -	ip4h->check = 0;
> -	ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0);
> +	uint32_t sum = L2_BUF_IP4_PSUM(ip4h->protocol);
> +
> +	sum += ip4h->tot_len;
> +	sum += (ip4h->saddr >> 16) & 0xffff;
> +	sum += ip4h->saddr & 0xffff;
> +	sum += (ip4h->daddr >> 16) & 0xffff;
> +	sum += ip4h->daddr & 0xffff;
> +
> +	return ~csum_fold(sum);
>  }
>  
>  /**
> diff --git a/checksum.h b/checksum.h
> index 6a20297a5826..b87ecd720df5 100644
> --- a/checksum.h
> +++ b/checksum.h
> @@ -13,7 +13,7 @@ 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_ip4_header(struct iphdr *ip4h);
> +uint16_t csum_ip4_header(const struct iphdr *ip4h);
>  void csum_udp4(struct udphdr *udp4hr,
>  	       struct in_addr saddr, struct in_addr daddr,
>  	       const void *payload, size_t len);
> diff --git a/tap.c b/tap.c
> index 3ea03f720d6d..70f36a55314f 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -160,7 +160,7 @@ static void *tap_push_ip4h(char *buf, struct in_addr src, struct in_addr dst,
>  	ip4h->protocol = proto;
>  	ip4h->saddr = src.s_addr;
>  	ip4h->daddr = dst.s_addr;
> -	csum_ip4_header(ip4h);
> +	ip4h->check = csum_ip4_header(ip4h);
>  	return ip4h + 1;
>  }
>  
> diff --git a/tcp.c b/tcp.c
> index 45ef5146729a..35e240f4ffc3 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -934,23 +934,6 @@ static void tcp_sock_set_bufsize(const struct ctx *c, int s)
>  		trace("TCP: failed to set SO_SNDBUF to %i", v);
>  }
>  
> -/**
> - * tcp_update_check_ip4() - Update IPv4 with variable parts from stored one
> - * @buf:	L2 packet buffer with final IPv4 header
> - */
> -static void tcp_update_check_ip4(struct tcp4_l2_buf_t *buf)
> -{
> -	uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_TCP);
> -
> -	sum += buf->iph.tot_len;
> -	sum += (buf->iph.saddr >> 16) & 0xffff;
> -	sum += buf->iph.saddr & 0xffff;
> -	sum += (buf->iph.daddr >> 16) & 0xffff;
> -	sum += buf->iph.daddr & 0xffff;
> -
> -	buf->iph.check = (uint16_t)~csum_fold(sum);
> -}
> -
>  /**
>   * tcp_update_check_tcp4() - Update TCP checksum from stored one
>   * @buf:	L2 packet buffer with final IPv4 header
> @@ -1393,10 +1376,7 @@ do {									\
>  		b->iph.saddr = a4->s_addr;
>  		b->iph.daddr = c->ip4.addr_seen.s_addr;
>  
> -		if (check)
> -			b->iph.check = *check;
> -		else
> -			tcp_update_check_ip4(b);
> +		b->iph.check = check ? *check : csum_ip4_header(&b->iph);
>  
>  		SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
>  
> diff --git a/udp.c b/udp.c
> index d514c864ab5b..e645c800a823 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -270,23 +270,6 @@ static void udp_invert_portmap(struct udp_port_fwd *fwd)
>  	}
>  }
>  
> -/**
> - * udp_update_check4() - Update checksum with variable parts from stored one
> - * @buf:	L2 packet buffer with final IPv4 header
> - */
> -static void udp_update_check4(struct udp4_l2_buf_t *buf)
> -{
> -	uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_UDP);
> -
> -	sum += buf->iph.tot_len;
> -	sum += (buf->iph.saddr >> 16) & 0xffff;
> -	sum += buf->iph.saddr & 0xffff;
> -	sum += (buf->iph.daddr >> 16) & 0xffff;
> -	sum += buf->iph.daddr & 0xffff;
> -
> -	buf->iph.check = (uint16_t)~csum_fold(sum);
> -}
> -
>  /**
>   * udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses
>   * @eth_d:	Ethernet destination address, NULL if unchanged
> @@ -579,6 +562,9 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>   *
>   * Return: size of tap frame with headers
>   */
> +#pragma GCC diagnostic push
> +/* ignore unaligned pointer value warning for &b->iph */
> +#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
>  static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
>  			      const struct timespec *now)
>  {
> @@ -614,13 +600,14 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
>  		b->iph.saddr = b->s_in.sin_addr.s_addr;
>  	}
>  
> -	udp_update_check4(b);
> +	b->iph.check = csum_ip4_header(&b->iph);

Similar comment as I had on v1: I don't think this is safe.

If &b->iph is, say, 0x2000, it's all fine: when csum_ip4_header() needs
to access, say, ip4h->tot_len, it will dereference 0x2000 and look at
16 bits, 2 bytes into it.

If &b->iph is 0x2001, though, csum_ip4_header() will dereference 0x2001
and, on some architectures, boom.

You need to pass b, or, if possible, to align iph to a 4-bytes boundary.
There's a reason why I implemented it like it is now.

The current version is rather inconvenient and ugly, so it's great if
you manage to improve it this way, but you shouldn't risk dereferencing
unaligned pointers... unless you know for some reason that they are
aligned, of course.

-- 
Stefano