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

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

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

On Mon, Nov 07, 2022 at 07:08:38PM +0100, Stefano Brivio wrote:
> On Fri,  4 Nov 2022 19:43:30 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > ---
> >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]