Re: [PATCH 07/10] tcp: Remove ugly address union from struct tcp_conn

8 Nov 2022

On Mon, Nov 07, 2022 at 07:08:38PM +0100, Stefano Brivio wrote:
...
  On Fri,  4 Nov 2022 19:43:30 +1100
 David Gibson &lt;david(a)gibson.dropbear.id.au&gt; wrote:

  We now only extract the v4 part of the
tcp_conn::a union in one place.
 Make this neater by using a helper to extract the IPv4 address from the
 mapped address in that place.

 Signed-off-by: David Gibson &lt;david(a)gibson.dropbear.id.au&gt;
 ---
  tcp.c  | 40 +++++++++++++++-------------------------
  util.c | 13 +++++++++++++
  util.h |  1 +
  3 files changed, 29 insertions(+), 25 deletions(-)

 diff --git a/tcp.c b/tcp.c
 index 3816a1c..6634abb 100644
 --- a/tcp.c
 +++ b/tcp.c
 @@ -416,10 +416,7 @@ struct tcp6_l2_head {	/* For MSS6 macro: keep in sync with
tcp6_l2_buf_t */
   * @ws_to_tap:		Window scaling factor advertised to tap/guest
   * @sndbuf:		Sending buffer in kernel, rounded to 2 ^ SNDBUF_BITS
   * @seq_dup_ack_approx:	Last duplicate ACK number sent to tap
 - * @a.a6:		IPv6 remote address, can be IPv4-mapped
 - * @a.a4.zero:		Zero prefix for IPv4-mapped, see RFC 6890, Table 20
 - * @a.a4.one:		Ones prefix for IPv4-mapped
 - * @a.a4.a:		IPv4 address
 + * @addr:		IPv6 remote address, can be IPv4-mapped
   * @tap_port:		Guest-facing tap port
   * @sock_port:		Remote, socket-facing port
   * @wnd_from_tap:	Last window size from tap, unscaled (as received)
 @@ -489,15 +486,8 @@ struct tcp_conn {
  	uint8_t		seq_dup_ack_approx;

 -	union {
 -		struct in6_addr a6;
 -		struct {
 -			uint8_t zero[10];
 -			uint8_t one[2];
 -			struct in_addr a;
 -		} a4;
 -	} a;
 -#define CONN_V4(conn)		IN6_IS_ADDR_V4MAPPED(&conn->a.a6)
 +	struct in6_addr addr;
 +#define CONN_V4(conn)		IN6_IS_ADDR_V4MAPPED(&conn->addr)
  #define CONN_V6(conn)		(!CONN_V4(conn))

  	in_port_t	tap_port;
 @@ -960,7 +950,7 @@ static int tcp_rtt_dst_low(const struct tcp_conn *conn)
  	int i;

  	for (i = 0; i < LOW_RTT_TABLE_SIZE; i++)
 -		if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, low_rtt_dst + i))
 +		if (IN6_ARE_ADDR_EQUAL(&conn->addr, low_rtt_dst + i))
  			return 1;

  	return 0;
 @@ -982,7 +972,7 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn,
  		return;

  	for (i = 0; i < LOW_RTT_TABLE_SIZE; i++) {
 -		if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, low_rtt_dst + i))
 +		if (IN6_ARE_ADDR_EQUAL(&conn->addr, low_rtt_dst + i))
  			return;
  		if (hole == -1 && IN6_IS_ADDR_UNSPECIFIED(low_rtt_dst + i))
  			hole = i;
 @@ -994,10 +984,10 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn,
  	if (hole == -1)
  		return;

 -	memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6));
 +	memcpy(low_rtt_dst + hole++, &conn->addr, sizeof(conn->addr));
  	if (hole == LOW_RTT_TABLE_SIZE)
  		hole = 0;
 -	memcpy(low_rtt_dst + hole, &in6addr_any, sizeof(conn->a.a6));
 +	memcpy(low_rtt_dst + hole, &in6addr_any, sizeof(conn->addr));
  #else
  	(void)conn;
  	(void)tinfo;
 @@ -1285,7 +1275,7 @@ static int tcp_hash_match(const struct tcp_conn *conn,
  			  const struct in6_addr *addr,
  			  in_port_t tap_port, in_port_t sock_port)
  {
 -	if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, addr)		&&
 +	if (IN6_ARE_ADDR_EQUAL(&conn->addr, addr)		&&
  	    conn->tap_port == tap_port && conn->sock_port == sock_port)
  		return 1;

 @@ -1330,7 +1320,7 @@ static void tcp_hash_insert(const struct ctx *c, struct tcp_conn
*conn)
  {
  	int b;

 -	b = tcp_hash(c, &conn->a.a6, conn->tap_port, conn->sock_port);
 +	b = tcp_hash(c, &conn->addr, conn->tap_port, conn->sock_port);
  	conn->next_index = tc_hash[b] ? tc_hash[b] - tc : -1;
  	tc_hash[b] = conn;
  	conn->hash_bucket = b;
 @@ -1660,7 +1650,7 @@ do {									\
  		ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);

  		b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
 -		b->ip6h.saddr = conn->a.a6;
 +		b->ip6h.saddr = conn->addr;
  		if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
  			b->ip6h.daddr = c->ip6.addr_ll_seen;
  		else
 @@ -1684,7 +1674,7 @@ do {									\

  		ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
  		b->iph.tot_len = htons(ip_len);
 -		b->iph.saddr = conn->a.a4.a.s_addr;
 +		b->iph.saddr = decode_ip4mapped_ip6(&conn->addr).s_addr;
  		b->iph.daddr = c->ip4.addr_seen.s_addr;

  		if (check)
 @@ -2202,12 +2192,12 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void
*addr,
  		sa = (struct sockaddr *)&addr4;
  		sl = sizeof(addr4);

 -		encode_ip4mapped_ip6(&conn->a.a6, addr);
 +		encode_ip4mapped_ip6(&conn->addr, addr);
  	} else {
  		sa = (struct sockaddr *)&addr6;
  		sl = sizeof(addr6);

 -		memcpy(&conn->a.a6,      addr, sizeof(conn->a.a6));
 +		memcpy(&conn->addr,      addr, sizeof(conn->addr));
  	}

  	conn->sock_port = ntohs(th->dest);
 @@ -2869,7 +2859,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
  			memcpy(&sa6.sin6_addr, src, sizeof(*src));
  		}

 -		memcpy(&conn->a.a6, &sa6.sin6_addr, sizeof(conn->a.a6));
 +		memcpy(&conn->addr, &sa6.sin6_addr, sizeof(conn->addr));

  		conn->sock_port = ntohs(sa6.sin6_port);
  		conn->tap_port = ref.r.p.tcp.tcp.index;
 @@ -2888,7 +2878,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
  		    IN4_ARE_ADDR_EQUAL(&sa4.sin_addr, &c->ip4.addr_seen))
  			sa4.sin_addr = c->ip4.gw;

 -		encode_ip4mapped_ip6(&conn->a.a6, &sa4.sin_addr);
 +		encode_ip4mapped_ip6(&conn->addr, &sa4.sin_addr);

  		conn->sock_port = ntohs(sa4.sin_port);
  		conn->tap_port = ref.r.p.tcp.tcp.index;
 diff --git a/util.c b/util.c
 index 257d0b6..ec7f57f 100644
 --- a/util.c
 +++ b/util.c
 @@ -502,3 +502,16 @@ void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4)
  	memset(&a->one, 0xff, sizeof(a->one));
  	memcpy(&a->a4, ip4, sizeof(a->a4));
  }
 +
 +/**
 + * decode_ip4mapped_ip6() - Decode an IPv4-mapped IPv6 address
 + * @ip6:	IPv6 address
 + *
 + * Returns:	IPv4 address  
 Return: IPv4 address 
Fixed.  I note there are a few other instances of this mistake in the
code, most by fault by the looks of it.  I'll try to fix those others
at some point.

...
   + */
 +struct in_addr decode_ip4mapped_ip6(const struct in6_addr *ip6)
 +{
 +	const struct ip4mapped_ip6 *a = (const struct ip4mapped_ip6 *)ip6;  
 ...if you use struct ip4_mapped_ip6 directly in struct conn, you could
 drop this cast, and perhaps this helper too, but I'm not sure if it has
 other implications. 
True.  As I said, I'm considering a slightly different approach now.

...

  +
 +	return a->a4;
 +}
 diff --git a/util.h b/util.h
 index f7d6a6f..103211d 100644
 --- a/util.h
 +++ b/util.h
 @@ -210,5 +210,6 @@ int __daemon(int pidfile_fd, int devnull_fd);
  int fls(unsigned long x);
  int write_file(const char *path, const char *buf);
  void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4);
 +struct in_addr decode_ip4mapped_ip6(const struct in6_addr *ip6);

  #endif /* UTIL_H */ 

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

2024

2023

2022

2021

Re: [PATCH 07/10] tcp: Remove ugly address union from struct tcp_conn