Re: [PATCH 06/24] ip: move duplicate IPv4 checksum function to ip.h

[Stefano Brivio <sbrivio@redhat.com>]

On Fri,  2 Feb 2024 15:11:33 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> We can find the same function to compute the IPv4 header
> checksum in tcp.c and udp.c
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  ip.h  | 14 ++++++++++++++
>  tcp.c | 23 ++---------------------
>  udp.c | 19 +------------------
>  3 files changed, 17 insertions(+), 39 deletions(-)
> 
> diff --git a/ip.h b/ip.h
> index b2e08bc049f3..ff7902c45a95 100644
> --- a/ip.h
> +++ b/ip.h
> @@ -9,6 +9,8 @@
>  #include <netinet/ip.h>
>  #include <netinet/ip6.h>
>  
> +#include "checksum.h"
> +
>  #define IN4_IS_ADDR_UNSPECIFIED(a) \
>  	((a)->s_addr == htonl_constant(INADDR_ANY))
>  #define IN4_IS_ADDR_BROADCAST(a) \
> @@ -83,4 +85,16 @@ struct ipv6_opt_hdr {
>  
>  char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
>  		 size_t *dlen);
> +static inline uint16_t ipv4_hdr_checksum(struct iphdr *iph, int proto)

A function comment would be nice. A couple of doubts:

- why is this an inline in ip.h, instead of a function in checksum.c?
  That would be more natural, I think

- this would be the first Layer-4 protocol number passed as int: we use
  uint8_t elsewhere. Now, socket(2) and similar all take an int, but
  using uint8_t internally keeps large arrays such as tap4_l4 a bit
  smaller.

  The only value defined in Linux UAPI exceeding eight bits is
  IPPROTO_MPTCP, 262, because that's never on the wire (the TCP
  protocol number is used instead). And we won't meet that either.

  In practice, it doesn't matter what we use here, but still uint8_t
  would be consistent.

> +{
> +	uint32_t sum = L2_BUF_IP4_PSUM(proto);
> +
> +	sum += iph->tot_len;
> +	sum += (iph->saddr >> 16) & 0xffff;
> +	sum += iph->saddr & 0xffff;
> +	sum += (iph->daddr >> 16) & 0xffff;
> +	sum += iph->daddr & 0xffff;
> +
> +	return ~csum_fold(sum);
> +}
>  #endif /* IP_H */
> diff --git a/tcp.c b/tcp.c
> index 4c9c5fb51c60..293ab12d8c21 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,8 @@ 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 :
> +				       ipv4_hdr_checksum(&b->iph, IPPROTO_TCP);
>  
>  		SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
>  
> diff --git a/udp.c b/udp.c
> index d514c864ab5b..6f867df81c05 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
> @@ -614,7 +597,7 @@ 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 = ipv4_hdr_checksum(&b->iph, IPPROTO_UDP);
>  	b->uh.source = b->s_in.sin_port;
>  	b->uh.dest = htons(dstport);
>  	b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));

-- 
Stefano