[PATCH 0/8] Small cleanups related to addresses and binding

[PATCH 0/8] Small cleanups related to addresses and binding

[David Gibson]

Here's another batch of cleanups for minor warts I noticed while
working on flow table things.

David Gibson (8):
  tcp: Fix address type for tcp_sock_init_af()
  treewide: Use IN4ADDR_LOOPBACK_INIT more widely
  treewide: Add IN4ADDR_ANY_INIT macro
  util: Use htonl_constant() in more places
  util: Improve sockaddr initialisation in sock_l4()
  icmp: Avoid unnecessary handling of unspecified bind address
  treewide: Avoid in_addr_t
  util: Make sock_l4() treat empty string ifname like NULL

 icmp.c       | 27 ++++++---------------------
 tcp.c        |  4 ++--
 tcp_splice.c |  2 +-
 udp.c        | 14 ++++++--------
 util.c       | 12 ++++--------
 util.h       |  7 +++++--
 6 files changed, 24 insertions(+), 42 deletions(-)

-- 
2.43.0

[PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[David Gibson]

This takes a struct in_addr * (i.e. an IPv4 address), although it's
explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
which it calls use a void * for the address, which can be either an in_addr
or an in6_addr.

We get away with this, because we don't do anything with the pointer other
than transfer it from the caller to sock_l4(), but it's misleading.  And
quite possibly technically UB, because C is like that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c
index f506cfd..bda95b2 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
  * Return: fd for the new listening socket, negative error code on failure
  */
 static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
-			    const struct in_addr *addr, const char *ifname)
+			    const void *addr, const char *ifname)
 {
 	union tcp_listen_epoll_ref tref = {
 		.port = port + c->tcp.fwd_in.delta[port],
-- 
@@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
  * Return: fd for the new listening socket, negative error code on failure
  */
 static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
-			    const struct in_addr *addr, const char *ifname)
+			    const void *addr, const char *ifname)
 {
 	union tcp_listen_epoll_ref tref = {
 		.port = port + c->tcp.fwd_in.delta[port],
-- 
2.43.0

[PATCH 2/8] treewide: Use IN4ADDR_LOOPBACK_INIT more widely

[David Gibson]

We already define IN4ADDR_LOOPBACK_INIT to initialise a struct in_addr to
the loopback address without delving into its internals.  However there are
some places we don't use it, and explicitly look at the internal structure
of struct in_addr, which we generally want to avoid.  Use the define more
widely to avoid that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c        | 2 +-
 tcp_splice.c | 2 +-
 udp.c        | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tcp.c b/tcp.c
index bda95b2..466b0ca 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2973,7 +2973,7 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
 		.port = port + c->tcp.fwd_out.delta[port],
 		.pif = PIF_SPLICE,
 	};
-	struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
+	struct in_addr loopback = IN4ADDR_LOOPBACK_INIT;
 	int s;
 
 	ASSERT(c->mode == MODE_PASTA);
diff --git a/tcp_splice.c b/tcp_splice.c
index 69ea79d..1655f8e 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -347,7 +347,7 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
 	struct sockaddr_in addr4 = {
 		.sin_family = AF_INET,
 		.sin_port = htons(port),
-		.sin_addr = { .s_addr = htonl(INADDR_LOOPBACK) },
+		.sin_addr = IN4ADDR_LOOPBACK_INIT,
 	};
 	const struct sockaddr *sa;
 	socklen_t sl;
diff --git a/udp.c b/udp.c
index 1f8c306..489a843 100644
--- a/udp.c
+++ b/udp.c
@@ -429,7 +429,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
 		struct sockaddr_in addr4 = {
 			.sin_family = AF_INET,
 			.sin_port = htons(src),
-			.sin_addr = { .s_addr = htonl(INADDR_LOOPBACK) },
+			.sin_addr = IN4ADDR_LOOPBACK_INIT,
 		};
 		if (bind(s, (struct sockaddr *)&addr4, sizeof(addr4)))
 			goto fail;
@@ -1012,7 +1012,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			udp_tap_map[V4][uref.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
 		} else {
-			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
+			struct in_addr loopback = IN4ADDR_LOOPBACK_INIT;
 
 			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
 					 ifname, port, uref.u32);
-- 
@@ -429,7 +429,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
 		struct sockaddr_in addr4 = {
 			.sin_family = AF_INET,
 			.sin_port = htons(src),
-			.sin_addr = { .s_addr = htonl(INADDR_LOOPBACK) },
+			.sin_addr = IN4ADDR_LOOPBACK_INIT,
 		};
 		if (bind(s, (struct sockaddr *)&addr4, sizeof(addr4)))
 			goto fail;
@@ -1012,7 +1012,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			udp_tap_map[V4][uref.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
 		} else {
-			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
+			struct in_addr loopback = IN4ADDR_LOOPBACK_INIT;
 
 			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
 					 ifname, port, uref.u32);
-- 
2.43.0

[PATCH 3/8] treewide: Add IN4ADDR_ANY_INIT macro

[David Gibson]

We already define IN4ADDR_LOOPBACK_INIT to initialise a struct in_addr to
the loopback address, make a similar one for the unspecified / any address.
This avoids messying things with the internal structure of struct in_addr
where we don't care about it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 icmp.c | 2 +-
 util.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/icmp.c b/icmp.c
index a1de8ae..2a15d25 100644
--- a/icmp.c
+++ b/icmp.c
@@ -169,7 +169,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 	if (af == AF_INET) {
 		struct sockaddr_in sa = {
 			.sin_family = AF_INET,
-			.sin_addr = { .s_addr = htonl(INADDR_ANY) },
+			.sin_addr = IN4ADDR_ANY_INIT,
 		};
 		union icmp_epoll_ref iref;
 		struct icmphdr *ih;
diff --git a/util.h b/util.h
index 53bb54b..6b11951 100644
--- a/util.h
+++ b/util.h
@@ -122,6 +122,9 @@
 	(((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
 #define IN4ADDR_LOOPBACK_INIT \
 	{ .s_addr	= htonl_constant(INADDR_LOOPBACK) }
+#define IN4ADDR_ANY_INIT \
+	{ .s_addr	= htonl_constant(INADDR_ANY) }
+
 
 #define NS_FN_STACK_SIZE	(RLIMIT_STACK_VAL * 1024 / 8)
 int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
-- 
@@ -122,6 +122,9 @@
 	(((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
 #define IN4ADDR_LOOPBACK_INIT \
 	{ .s_addr	= htonl_constant(INADDR_LOOPBACK) }
+#define IN4ADDR_ANY_INIT \
+	{ .s_addr	= htonl_constant(INADDR_ANY) }
+
 
 #define NS_FN_STACK_SIZE	(RLIMIT_STACK_VAL * 1024 / 8)
 int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
-- 
2.43.0

[PATCH 4/8] util: Use htonl_constant() in more places

[David Gibson]

We might as well when we're passing a known constant value, giving the
compiler the best chance to optimise things away.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util.h b/util.h
index 6b11951..b4ad32d 100644
--- a/util.h
+++ b/util.h
@@ -111,9 +111,9 @@
 #endif
 
 #define IN4_IS_ADDR_UNSPECIFIED(a) \
-	((a)->s_addr == htonl(INADDR_ANY))
+	((a)->s_addr == htonl_constant(INADDR_ANY))
 #define IN4_IS_ADDR_BROADCAST(a) \
-	((a)->s_addr == htonl(INADDR_BROADCAST))
+	((a)->s_addr == htonl_constant(INADDR_BROADCAST))
 #define IN4_IS_ADDR_LOOPBACK(a) \
 	(ntohl((a)->s_addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET)
 #define IN4_IS_ADDR_MULTICAST(a) \
-- 
@@ -111,9 +111,9 @@
 #endif
 
 #define IN4_IS_ADDR_UNSPECIFIED(a) \
-	((a)->s_addr == htonl(INADDR_ANY))
+	((a)->s_addr == htonl_constant(INADDR_ANY))
 #define IN4_IS_ADDR_BROADCAST(a) \
-	((a)->s_addr == htonl(INADDR_BROADCAST))
+	((a)->s_addr == htonl_constant(INADDR_BROADCAST))
 #define IN4_IS_ADDR_LOOPBACK(a) \
 	(ntohl((a)->s_addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET)
 #define IN4_IS_ADDR_MULTICAST(a) \
-- 
2.43.0

[PATCH 5/8] util: Improve sockaddr initialisation in sock_l4()

[David Gibson]

Currently we initialise the address field of the sockaddrs we construct
to the any/unspecified address, but not in a very clear way: we use
explicit 0 values, which is only interpretable if you know the order of
fields in the sockaddr structures.  Use explicit field names, and explicit
initialiser macros for the address.

Because we initialise to this default value, we don't need to explicitly
set the any/unspecified address later on if the caller didn't pass an
overriding bind address.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/util.c b/util.c
index d465e48..0152ae6 100644
--- a/util.c
+++ b/util.c
@@ -105,12 +105,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	struct sockaddr_in addr4 = {
 		.sin_family = AF_INET,
 		.sin_port = htons(port),
-		{ 0 }, { 0 },
+		.sin_addr = IN4ADDR_ANY_INIT,
 	};
 	struct sockaddr_in6 addr6 = {
 		.sin6_family = AF_INET6,
 		.sin6_port = htons(port),
-		0, IN6ADDR_ANY_INIT, 0,
+		.sin6_addr = IN6ADDR_ANY_INIT,
 	};
 	const struct sockaddr *sa;
 	bool dual_stack = false;
@@ -162,8 +162,6 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	if (af == AF_INET) {
 		if (bind_addr)
 			addr4.sin_addr.s_addr = *(in_addr_t *)bind_addr;
-		else
-			addr4.sin_addr.s_addr = htonl(INADDR_ANY);
 
 		sa = (const struct sockaddr *)&addr4;
 		sl = sizeof(addr4);
@@ -174,8 +172,6 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 			if (!memcmp(bind_addr, &c->ip6.addr_ll,
 			    sizeof(c->ip6.addr_ll)))
 				addr6.sin6_scope_id = c->ifi6;
-		} else {
-			addr6.sin6_addr = in6addr_any;
 		}
 
 		sa = (const struct sockaddr *)&addr6;
-- 
@@ -105,12 +105,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	struct sockaddr_in addr4 = {
 		.sin_family = AF_INET,
 		.sin_port = htons(port),
-		{ 0 }, { 0 },
+		.sin_addr = IN4ADDR_ANY_INIT,
 	};
 	struct sockaddr_in6 addr6 = {
 		.sin6_family = AF_INET6,
 		.sin6_port = htons(port),
-		0, IN6ADDR_ANY_INIT, 0,
+		.sin6_addr = IN6ADDR_ANY_INIT,
 	};
 	const struct sockaddr *sa;
 	bool dual_stack = false;
@@ -162,8 +162,6 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	if (af == AF_INET) {
 		if (bind_addr)
 			addr4.sin_addr.s_addr = *(in_addr_t *)bind_addr;
-		else
-			addr4.sin_addr.s_addr = htonl(INADDR_ANY);
 
 		sa = (const struct sockaddr *)&addr4;
 		sl = sizeof(addr4);
@@ -174,8 +172,6 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 			if (!memcmp(bind_addr, &c->ip6.addr_ll,
 			    sizeof(c->ip6.addr_ll)))
 				addr6.sin6_scope_id = c->ifi6;
-		} else {
-			addr6.sin6_addr = in6addr_any;
 		}
 
 		sa = (const struct sockaddr *)&addr6;
-- 
2.43.0

[PATCH 6/8] icmp: Avoid unnecessary handling of unspecified bind address

[David Gibson]

We go to some trouble, if the configured output address is unspecified, to
pass NULL to sock_l4().  But while passing NULL is one way to get sock_l4()
not to specify a bind address, passing the "any" address explicitly works
too.  Use this to simplify icmp_tap_handler().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 icmp.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/icmp.c b/icmp.c
index 2a15d25..d982fda 100644
--- a/icmp.c
+++ b/icmp.c
@@ -187,16 +187,12 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		iref.id = id = ntohs(ih->un.echo.id);
 
 		if ((s = icmp_id_map[V4][id].sock) <= 0) {
-			const struct in_addr *bind_addr = NULL;
 			const char *bind_if;
 
 			bind_if = *c->ip4.ifname_out ? c->ip4.ifname_out : NULL;
 
-			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out))
-				bind_addr = &c->ip4.addr_out;
-
-			s = sock_l4(c, AF_INET, IPPROTO_ICMP, bind_addr,
-				    bind_if, id, iref.u32);
+			s = sock_l4(c, AF_INET, IPPROTO_ICMP,
+				    &c->ip4.addr_out, bind_if, id, iref.u32);
 			if (s < 0)
 				goto fail_sock;
 			if (s > FD_REF_MAX) {
@@ -241,16 +237,12 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 
 		iref.id = id = ntohs(ih->icmp6_identifier);
 		if ((s = icmp_id_map[V6][id].sock) <= 0) {
-			const struct in6_addr *bind_addr = NULL;
 			const char *bind_if;
 
 			bind_if = *c->ip6.ifname_out ? c->ip6.ifname_out : NULL;
 
-			if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out))
-				bind_addr = &c->ip6.addr_out;
-
-			s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6, bind_addr,
-				    bind_if, id, iref.u32);
+			s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6,
+				    &c->ip6.addr_out, bind_if, id, iref.u32);
 			if (s < 0)
 				goto fail_sock;
 			if (s > FD_REF_MAX) {
-- 
@@ -187,16 +187,12 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		iref.id = id = ntohs(ih->un.echo.id);
 
 		if ((s = icmp_id_map[V4][id].sock) <= 0) {
-			const struct in_addr *bind_addr = NULL;
 			const char *bind_if;
 
 			bind_if = *c->ip4.ifname_out ? c->ip4.ifname_out : NULL;
 
-			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out))
-				bind_addr = &c->ip4.addr_out;
-
-			s = sock_l4(c, AF_INET, IPPROTO_ICMP, bind_addr,
-				    bind_if, id, iref.u32);
+			s = sock_l4(c, AF_INET, IPPROTO_ICMP,
+				    &c->ip4.addr_out, bind_if, id, iref.u32);
 			if (s < 0)
 				goto fail_sock;
 			if (s > FD_REF_MAX) {
@@ -241,16 +237,12 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 
 		iref.id = id = ntohs(ih->icmp6_identifier);
 		if ((s = icmp_id_map[V6][id].sock) <= 0) {
-			const struct in6_addr *bind_addr = NULL;
 			const char *bind_if;
 
 			bind_if = *c->ip6.ifname_out ? c->ip6.ifname_out : NULL;
 
-			if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out))
-				bind_addr = &c->ip6.addr_out;
-
-			s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6, bind_addr,
-				    bind_if, id, iref.u32);
+			s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6,
+				    &c->ip6.addr_out, bind_if, id, iref.u32);
 			if (s < 0)
 				goto fail_sock;
 			if (s > FD_REF_MAX) {
-- 
2.43.0

[PATCH 7/8] treewide: Avoid in_addr_t

[David Gibson]

IPv4 addresses can be stored in an in_addr_t or a struct in_addr.  The
former is just a type alias to a 32-bit integer, so doesn't really give us
any type checking.  Therefore we generally prefer the structure, since we
mostly want to treat IP address as opaque objects.  Fix a few places where
we still use in_addr_t, but can just as easily use struct in_addr.

Note there are still some uses of in_addr_t in conf.c, but those are
justified: since they're doing prefix calculations, they actually need to
look at the internals of the address as an integer.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c  | 4 ++--
 util.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/udp.c b/udp.c
index 489a843..f6e8b37 100644
--- a/udp.c
+++ b/udp.c
@@ -866,8 +866,8 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 		debug("UDP from tap src=%hu dst=%hu, s=%d",
 		      src, dst, udp_tap_map[V4][src].sock);
 		if ((s = udp_tap_map[V4][src].sock) < 0) {
+			struct in_addr bind_addr = IN4ADDR_ANY_INIT;
 			union udp_epoll_ref uref = { .port = src };
-			in_addr_t bind_addr = { 0 };
 			const char *bind_if = NULL;
 
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr) &&
@@ -876,7 +876,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 
 			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) &&
 			    !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
-				bind_addr = c->ip4.addr_out.s_addr;
+				bind_addr = c->ip4.addr_out;
 
 			s = sock_l4(c, AF_INET, IPPROTO_UDP, &bind_addr,
 				    bind_if, src, uref.u32);
diff --git a/util.c b/util.c
index 0152ae6..4de7b96 100644
--- a/util.c
+++ b/util.c
@@ -161,7 +161,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 
 	if (af == AF_INET) {
 		if (bind_addr)
-			addr4.sin_addr.s_addr = *(in_addr_t *)bind_addr;
+			addr4.sin_addr = *(struct in_addr *)bind_addr;
 
 		sa = (const struct sockaddr *)&addr4;
 		sl = sizeof(addr4);
-- 
@@ -161,7 +161,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 
 	if (af == AF_INET) {
 		if (bind_addr)
-			addr4.sin_addr.s_addr = *(in_addr_t *)bind_addr;
+			addr4.sin_addr = *(struct in_addr *)bind_addr;
 
 		sa = (const struct sockaddr *)&addr4;
 		sl = sizeof(addr4);
-- 
2.43.0

[PATCH 8/8] util: Make sock_l4() treat empty string ifname like NULL

[David Gibson]

sock_l4() takes NULL for ifname if you don't want to bind the socket to a
particular interface.  However, for a number of the callers, it's more
natural to use an empty string for that case.  Change sock_l4() to accept
either NULL or an empty string equivalently, and simplify some callers
using that change.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 icmp.c | 15 ++++-----------
 udp.c  |  6 ++----
 util.c |  2 +-
 3 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/icmp.c b/icmp.c
index d982fda..c82efd0 100644
--- a/icmp.c
+++ b/icmp.c
@@ -187,12 +187,8 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		iref.id = id = ntohs(ih->un.echo.id);
 
 		if ((s = icmp_id_map[V4][id].sock) <= 0) {
-			const char *bind_if;
-
-			bind_if = *c->ip4.ifname_out ? c->ip4.ifname_out : NULL;
-
-			s = sock_l4(c, AF_INET, IPPROTO_ICMP,
-				    &c->ip4.addr_out, bind_if, id, iref.u32);
+			s = sock_l4(c, AF_INET, IPPROTO_ICMP, &c->ip4.addr_out,
+				    c->ip4.ifname_out, id, iref.u32);
 			if (s < 0)
 				goto fail_sock;
 			if (s > FD_REF_MAX) {
@@ -237,12 +233,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 
 		iref.id = id = ntohs(ih->icmp6_identifier);
 		if ((s = icmp_id_map[V6][id].sock) <= 0) {
-			const char *bind_if;
-
-			bind_if = *c->ip6.ifname_out ? c->ip6.ifname_out : NULL;
-
 			s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6,
-				    &c->ip6.addr_out, bind_if, id, iref.u32);
+				    &c->ip6.addr_out,
+				    c->ip6.ifname_out, id, iref.u32);
 			if (s < 0)
 				goto fail_sock;
 			if (s > FD_REF_MAX) {
diff --git a/udp.c b/udp.c
index f6e8b37..7057977 100644
--- a/udp.c
+++ b/udp.c
@@ -870,8 +870,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			union udp_epoll_ref uref = { .port = src };
 			const char *bind_if = NULL;
 
-			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr) &&
-			    *c->ip6.ifname_out)
+			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr))
 				bind_if = c->ip6.ifname_out;
 
 			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) &&
@@ -919,8 +918,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			union udp_epoll_ref uref = { .v6 = 1, .port = src };
 			const char *bind_if = NULL;
 
-			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) &&
-			    *c->ip6.ifname_out)
+			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr))
 				bind_if = c->ip6.ifname_out;
 
 			if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out) &&
diff --git a/util.c b/util.c
index 4de7b96..9c7012c 100644
--- a/util.c
+++ b/util.c
@@ -187,7 +187,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)))
 		debug("Failed to set SO_REUSEADDR on socket %i", fd);
 
-	if (ifname) {
+	if (ifname && *ifname) {
 		/* Supported since kernel version 5.7, commit c427bfec18f2
 		 * ("net: core: enable SO_BINDTODEVICE for non-root users"). If
 		 * it's unsupported, don't bind the socket at all, because the
-- 
@@ -187,7 +187,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)))
 		debug("Failed to set SO_REUSEADDR on socket %i", fd);
 
-	if (ifname) {
+	if (ifname && *ifname) {
 		/* Supported since kernel version 5.7, commit c427bfec18f2
 		 * ("net: core: enable SO_BINDTODEVICE for non-root users"). If
 		 * it's unsupported, don't bind the socket at all, because the
-- 
2.43.0

Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[Stefano Brivio]

On Fri,  8 Dec 2023 01:31:33 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This takes a struct in_addr * (i.e. an IPv4 address), although it's
> explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
> which it calls use a void * for the address, which can be either an in_addr
> or an in6_addr.
> 
> We get away with this, because we don't do anything with the pointer other
> than transfer it from the caller to sock_l4(), but it's misleading.  And
> quite possibly technically UB, because C is like that.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcp.c b/tcp.c
> index f506cfd..bda95b2 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
>   * Return: fd for the new listening socket, negative error code on failure
>   */
>  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> -			    const struct in_addr *addr, const char *ifname)
> +			    const void *addr, const char *ifname)

This is obviously correct.

However, after a lot of thinking: (gcc) optimisations based on
Type-Based Alias Analysis, which we don't disable on this path, could,
now, happily defer filling 'addr' with inet_pton() in conf_ports() to a
point *after* the tcp_sock_init() call.

Without this patch, at least 32 bits must be updated before the call.

It might sound like a joke because... it actually is. But look at what
we had to do for the functions in checksum.c. We pass const void *buf,
and anything that buf points to can be updated (with TBAA) after the
call.

I don't see any conceptual difference between this case and those
functions.

Anyway, that won't reasonably happen here, and in any case this would
have been broken for IPv6, so I'll go ahead and apply this.

But, eventually, I think we should switch all these usages to union
inany_addr *.

-- 
Stefano

Re: [PATCH 5/8] util: Improve sockaddr initialisation in sock_l4()

[Stefano Brivio]

On Fri,  8 Dec 2023 01:31:37 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we initialise the address field of the sockaddrs we construct
> to the any/unspecified address, but not in a very clear way: we use
> explicit 0 values, which is only interpretable if you know the order of
> fields in the sockaddr structures.  Use explicit field names, and explicit
> initialiser macros for the address.
> 
> Because we initialise to this default value, we don't need to explicitly
> set the any/unspecified address later on if the caller didn't pass an
> overriding bind address.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  util.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/util.c b/util.c
> index d465e48..0152ae6 100644
> --- a/util.c
> +++ b/util.c
> @@ -105,12 +105,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  	struct sockaddr_in addr4 = {
>  		.sin_family = AF_INET,
>  		.sin_port = htons(port),
> -		{ 0 }, { 0 },
> +		.sin_addr = IN4ADDR_ANY_INIT,

The second { 0 } was meant to initialise .sin_zero, and:

>  	};
>  	struct sockaddr_in6 addr6 = {
>  		.sin6_family = AF_INET6,
>  		.sin6_port = htons(port),
> -		0, IN6ADDR_ANY_INIT, 0,
> +		.sin6_addr = IN6ADDR_ANY_INIT,

these zeroes were for sin6_flowinfo and sin6_scope_id.

Not needed because we want zeroes anyway (or the "same as objects that
have static storage duration"), but see commit eed6933e6c29 ("udp:
Explicitly initialise sin6_scope_id and sin_zero in sockaddr_in{,6}"):
gcc versions 7 to 9 used to complain. And gcc 9 isn't *that* old.

But then, you might ask, why didn't I just use names for all the
initialisers? Well, there was some issue with sockaddr_in6 or
sockaddr_in not having a field defined in some header (kernel or a C
library). I have a vague memory it was about sin6_scope_id, but I can't
find any note in the git history or anywhere else. :(

Now, the latest addition to sockaddr_in6 was sin6_scope_id (kernel:
2006, glibc: 2000), so that's fine. Also sockaddr_in looks okay,
including sin_zero, with "#define sin_zero __pad" in the kernel version.

So... my preferred option would be to leave this untouched: the
initialisers you replaced are anyway all zeroes. And, after RFC 2553
(24 years ago), the order of those fields never changed.

If it really bothers you, let's at least initialise everything
explicitly by name, because we know that gcc 9 complains, and if we hit
another version of sockaddr_in6 without a named sin6_scope_id, we'll
find out, eventually.

The rest of the series looks good to me and I just applied it (minus
_this part_ of this patch).

-- 
Stefano

Re: [PATCH 0/8] Small cleanups related to addresses and binding

[Stefano Brivio]

On Fri,  8 Dec 2023 01:31:32 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Here's another batch of cleanups for minor warts I noticed while
> working on flow table things.
> 
> David Gibson (8):
>   tcp: Fix address type for tcp_sock_init_af()
>   treewide: Use IN4ADDR_LOOPBACK_INIT more widely
>   treewide: Add IN4ADDR_ANY_INIT macro
>   util: Use htonl_constant() in more places
>   util: Improve sockaddr initialisation in sock_l4()
>   icmp: Avoid unnecessary handling of unspecified bind address
>   treewide: Avoid in_addr_t
>   util: Make sock_l4() treat empty string ifname like NULL

Applied, minus first two hunks of 5/8.

-- 
Stefano

Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[David Gibson]

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

On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote:
> On Fri,  8 Dec 2023 01:31:33 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This takes a struct in_addr * (i.e. an IPv4 address), although it's
> > explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
> > which it calls use a void * for the address, which can be either an in_addr
> > or an in6_addr.
> > 
> > We get away with this, because we don't do anything with the pointer other
> > than transfer it from the caller to sock_l4(), but it's misleading.  And
> > quite possibly technically UB, because C is like that.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index f506cfd..bda95b2 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
> >   * Return: fd for the new listening socket, negative error code on failure
> >   */
> >  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> > -			    const struct in_addr *addr, const char *ifname)
> > +			    const void *addr, const char *ifname)
> 
> This is obviously correct.
> 
> However, after a lot of thinking: (gcc) optimisations based on
> Type-Based Alias Analysis, which we don't disable on this path, could,
> now, happily defer filling 'addr' with inet_pton() in conf_ports() to a
> point *after* the tcp_sock_init() call.

Hrm... possibly.  The fact that the addr variable in conf_ports() is a
char array, not a struct in*_addr might save us.  I think replacing it
with a union of an in_addr and in6_addr would also be ok.

> Without this patch, at least 32 bits must be updated before the call.

I'm not sure that's correct.  If the compiler is allowed to assume
that a char[] and a void * aren't aliased (even if they clearly are),
then I'd expect it to also be allowed to assume that a char[] and a
struct in_addr * aren't aliased.

> It might sound like a joke because... it actually is. But look at what
> we had to do for the functions in checksum.c. We pass const void *buf,
> and anything that buf points to can be updated (with TBAA) after the
> call.
> 
> I don't see any conceptual difference between this case and those
> functions.
> 
> Anyway, that won't reasonably happen here, and in any case this would
> have been broken for IPv6, so I'll go ahead and apply this.
> 
> But, eventually, I think we should switch all these usages to union
> inany_addr *.

So, we may be able to use union inany_addr in some places, but that's
not the same thing as this: inany_addr carries IPv4 addresses as
mapped IPv6 addresses, it's not switched on a separate af parameter.
We could, of course, define a new type as a simple union of in_addr
and in6_addr.

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

Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[Stefano Brivio]

On Thu, 28 Dec 2023 13:42:25 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote:
> > On Fri,  8 Dec 2023 01:31:33 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > This takes a struct in_addr * (i.e. an IPv4 address), although it's
> > > explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
> > > which it calls use a void * for the address, which can be either an in_addr
> > > or an in6_addr.
> > > 
> > > We get away with this, because we don't do anything with the pointer other
> > > than transfer it from the caller to sock_l4(), but it's misleading.  And
> > > quite possibly technically UB, because C is like that.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  tcp.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tcp.c b/tcp.c
> > > index f506cfd..bda95b2 100644
> > > --- a/tcp.c
> > > +++ b/tcp.c
> > > @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
> > >   * Return: fd for the new listening socket, negative error code on failure
> > >   */
> > >  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> > > -			    const struct in_addr *addr, const char *ifname)
> > > +			    const void *addr, const char *ifname)  
> > 
> > This is obviously correct.
> > 
> > However, after a lot of thinking: (gcc) optimisations based on
> > Type-Based Alias Analysis, which we don't disable on this path, could,
> > now, happily defer filling 'addr' with inet_pton() in conf_ports() to a
> > point *after* the tcp_sock_init() call.  
> 
> Hrm... possibly.  The fact that the addr variable in conf_ports() is a
> char array, not a struct in*_addr might save us.

Hmm, look at the commit message for a48c5c2abf8a ("treewide: Disable
gcc strict aliasing rules as needed, drop workarounds"): that didn't help
with the checksum functions, because yes, at some point I had char *, but
then I used those as different types.

I guess struct in_addr / struct in6_addr as we have in sock_l4() might be
equivalent to that.

> I think replacing it
> with a union of an in_addr and in6_addr would also be ok.

That should work, yes, and that's what I originally wanted to suggest,
before remembering about union inany_addr... but that doesn't fit, see
below.

> > Without this patch, at least 32 bits must be updated before the call.  
> 
> I'm not sure that's correct.  If the compiler is allowed to assume
> that a char[] and a void * aren't aliased (even if they clearly are),
> then I'd expect it to also be allowed to assume that a char[] and a
> struct in_addr * aren't aliased.

Ouch, right, they aren't (again... sarcastically speaking).

> > It might sound like a joke because... it actually is. But look at what
> > we had to do for the functions in checksum.c. We pass const void *buf,
> > and anything that buf points to can be updated (with TBAA) after the
> > call.
> > 
> > I don't see any conceptual difference between this case and those
> > functions.
> > 
> > Anyway, that won't reasonably happen here, and in any case this would
> > have been broken for IPv6, so I'll go ahead and apply this.
> > 
> > But, eventually, I think we should switch all these usages to union
> > inany_addr *.  
> 
> So, we may be able to use union inany_addr in some places, but that's
> not the same thing as this: inany_addr carries IPv4 addresses as
> mapped IPv6 addresses, it's not switched on a separate af parameter.

I really meant *a pointer* to union inany_addr, that is:

> We could, of course, define a new type as a simple union of in_addr
> and in6_addr.

...abusing it instead of using a separate union. On the other hand,
given where 'a4' is in there, it's not necessarily the same for
(strict) aliasing considerations.

Is "union in10_addr" fashionable enough? We could use A [16], but it's
inconvenient to type, and difficult to pronounce.

-- 
Stefano

Re: [PATCH 5/8] util: Improve sockaddr initialisation in sock_l4()

[David Gibson]

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

On Wed, Dec 27, 2023 at 09:25:15PM +0100, Stefano Brivio wrote:
> On Fri,  8 Dec 2023 01:31:37 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently we initialise the address field of the sockaddrs we construct
> > to the any/unspecified address, but not in a very clear way: we use
> > explicit 0 values, which is only interpretable if you know the order of
> > fields in the sockaddr structures.  Use explicit field names, and explicit
> > initialiser macros for the address.
> > 
> > Because we initialise to this default value, we don't need to explicitly
> > set the any/unspecified address later on if the caller didn't pass an
> > overriding bind address.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  util.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/util.c b/util.c
> > index d465e48..0152ae6 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -105,12 +105,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
> >  	struct sockaddr_in addr4 = {
> >  		.sin_family = AF_INET,
> >  		.sin_port = htons(port),
> > -		{ 0 }, { 0 },
> > +		.sin_addr = IN4ADDR_ANY_INIT,
> 
> The second { 0 } was meant to initialise .sin_zero, and:
> 
> >  	};
> >  	struct sockaddr_in6 addr6 = {
> >  		.sin6_family = AF_INET6,
> >  		.sin6_port = htons(port),
> > -		0, IN6ADDR_ANY_INIT, 0,
> > +		.sin6_addr = IN6ADDR_ANY_INIT,
> 
> these zeroes were for sin6_flowinfo and sin6_scope_id.
> 
> Not needed because we want zeroes anyway (or the "same as objects that
> have static storage duration"), but see commit eed6933e6c29 ("udp:
> Explicitly initialise sin6_scope_id and sin_zero in sockaddr_in{,6}"):
> gcc versions 7 to 9 used to complain. And gcc 9 isn't *that* old.
> 
> But then, you might ask, why didn't I just use names for all the
> initialisers? Well, there was some issue with sockaddr_in6 or
> sockaddr_in not having a field defined in some header (kernel or a C
> library). I have a vague memory it was about sin6_scope_id, but I can't
> find any note in the git history or anywhere else. :(
> 
> Now, the latest addition to sockaddr_in6 was sin6_scope_id (kernel:
> 2006, glibc: 2000), so that's fine. Also sockaddr_in looks okay,
> including sin_zero, with "#define sin_zero __pad" in the kernel version.
> 
> So... my preferred option would be to leave this untouched: the
> initialisers you replaced are anyway all zeroes. And, after RFC 2553
> (24 years ago), the order of those fields never changed.
> 
> If it really bothers you, let's at least initialise everything
> explicitly by name, because we know that gcc 9 complains, and if we hit
> another version of sockaddr_in6 without a named sin6_scope_id, we'll
> find out, eventually.
> 
> The rest of the series looks good to me and I just applied it (minus
> _this part_ of this patch).

Eh, ok.  I'll worry about this bit if I run across it again.

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

Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[David Gibson]

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

On Thu, Dec 28, 2023 at 11:11:19AM +0100, Stefano Brivio wrote:
> On Thu, 28 Dec 2023 13:42:25 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote:
> > > On Fri,  8 Dec 2023 01:31:33 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > This takes a struct in_addr * (i.e. an IPv4 address), although it's
> > > > explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
> > > > which it calls use a void * for the address, which can be either an in_addr
> > > > or an in6_addr.
> > > > 
> > > > We get away with this, because we don't do anything with the pointer other
> > > > than transfer it from the caller to sock_l4(), but it's misleading.  And
> > > > quite possibly technically UB, because C is like that.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  tcp.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tcp.c b/tcp.c
> > > > index f506cfd..bda95b2 100644
> > > > --- a/tcp.c
> > > > +++ b/tcp.c
> > > > @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
> > > >   * Return: fd for the new listening socket, negative error code on failure
> > > >   */
> > > >  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> > > > -			    const struct in_addr *addr, const char *ifname)
> > > > +			    const void *addr, const char *ifname)  
> > > 
> > > This is obviously correct.
> > > 
> > > However, after a lot of thinking: (gcc) optimisations based on
> > > Type-Based Alias Analysis, which we don't disable on this path, could,
> > > now, happily defer filling 'addr' with inet_pton() in conf_ports() to a
> > > point *after* the tcp_sock_init() call.  
> > 
> > Hrm... possibly.  The fact that the addr variable in conf_ports() is a
> > char array, not a struct in*_addr might save us.
> 
> Hmm, look at the commit message for a48c5c2abf8a ("treewide: Disable
> gcc strict aliasing rules as needed, drop workarounds"): that didn't help
> with the checksum functions, because yes, at some point I had char *, but
> then I used those as different types.
> 
> I guess struct in_addr / struct in6_addr as we have in sock_l4() might be
> equivalent to that.
> 
> > I think replacing it
> > with a union of an in_addr and in6_addr would also be ok.
> 
> That should work, yes, and that's what I originally wanted to suggest,
> before remembering about union inany_addr... but that doesn't fit, see
> below.
> 
> > > Without this patch, at least 32 bits must be updated before the call.  
> > 
> > I'm not sure that's correct.  If the compiler is allowed to assume
> > that a char[] and a void * aren't aliased (even if they clearly are),
> > then I'd expect it to also be allowed to assume that a char[] and a
> > struct in_addr * aren't aliased.
> 
> Ouch, right, they aren't (again... sarcastically speaking).
> 
> > > It might sound like a joke because... it actually is. But look at what
> > > we had to do for the functions in checksum.c. We pass const void *buf,
> > > and anything that buf points to can be updated (with TBAA) after the
> > > call.
> > > 
> > > I don't see any conceptual difference between this case and those
> > > functions.
> > > 
> > > Anyway, that won't reasonably happen here, and in any case this would
> > > have been broken for IPv6, so I'll go ahead and apply this.
> > > 
> > > But, eventually, I think we should switch all these usages to union
> > > inany_addr *.  
> > 
> > So, we may be able to use union inany_addr in some places, but that's
> > not the same thing as this: inany_addr carries IPv4 addresses as
> > mapped IPv6 addresses, it's not switched on a separate af parameter.
> 
> I really meant *a pointer* to union inany_addr, that is:
> 
> > We could, of course, define a new type as a simple union of in_addr
> > and in6_addr.
> 
> ...abusing it instead of using a separate union. On the other hand,
> given where 'a4' is in there, it's not necessarily the same for
> (strict) aliasing considerations.
> 
> Is "union in10_addr" fashionable enough? We could use A [16], but it's
> inconvenient to type, and difficult to pronounce.

Uh, I haven't heard of either of those.

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

Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[Stefano Brivio]

On Sun, 7 Jan 2024 16:35:56 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Dec 28, 2023 at 11:11:19AM +0100, Stefano Brivio wrote:
> > On Thu, 28 Dec 2023 13:42:25 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote:  
> > > > On Fri,  8 Dec 2023 01:31:33 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > This takes a struct in_addr * (i.e. an IPv4 address), although it's
> > > > > explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
> > > > > which it calls use a void * for the address, which can be either an in_addr
> > > > > or an in6_addr.
> > > > > 
> > > > > We get away with this, because we don't do anything with the pointer other
> > > > > than transfer it from the caller to sock_l4(), but it's misleading.  And
> > > > > quite possibly technically UB, because C is like that.
> > > > > 
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > ---
> > > > >  tcp.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tcp.c b/tcp.c
> > > > > index f506cfd..bda95b2 100644
> > > > > --- a/tcp.c
> > > > > +++ b/tcp.c
> > > > > @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
> > > > >   * Return: fd for the new listening socket, negative error code on failure
> > > > >   */
> > > > >  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> > > > > -			    const struct in_addr *addr, const char *ifname)
> > > > > +			    const void *addr, const char *ifname)    
> > > > 
> > > > This is obviously correct.
> > > > 
> > > > However, after a lot of thinking: (gcc) optimisations based on
> > > > Type-Based Alias Analysis, which we don't disable on this path, could,
> > > > now, happily defer filling 'addr' with inet_pton() in conf_ports() to a
> > > > point *after* the tcp_sock_init() call.    
> > > 
> > > Hrm... possibly.  The fact that the addr variable in conf_ports() is a
> > > char array, not a struct in*_addr might save us.  
> > 
> > Hmm, look at the commit message for a48c5c2abf8a ("treewide: Disable
> > gcc strict aliasing rules as needed, drop workarounds"): that didn't help
> > with the checksum functions, because yes, at some point I had char *, but
> > then I used those as different types.
> > 
> > I guess struct in_addr / struct in6_addr as we have in sock_l4() might be
> > equivalent to that.
> >   
> > > I think replacing it
> > > with a union of an in_addr and in6_addr would also be ok.  
> > 
> > That should work, yes, and that's what I originally wanted to suggest,
> > before remembering about union inany_addr... but that doesn't fit, see
> > below.
> >   
> > > > Without this patch, at least 32 bits must be updated before the call.    
> > > 
> > > I'm not sure that's correct.  If the compiler is allowed to assume
> > > that a char[] and a void * aren't aliased (even if they clearly are),
> > > then I'd expect it to also be allowed to assume that a char[] and a
> > > struct in_addr * aren't aliased.  
> > 
> > Ouch, right, they aren't (again... sarcastically speaking).
> >   
> > > > It might sound like a joke because... it actually is. But look at what
> > > > we had to do for the functions in checksum.c. We pass const void *buf,
> > > > and anything that buf points to can be updated (with TBAA) after the
> > > > call.
> > > > 
> > > > I don't see any conceptual difference between this case and those
> > > > functions.
> > > > 
> > > > Anyway, that won't reasonably happen here, and in any case this would
> > > > have been broken for IPv6, so I'll go ahead and apply this.
> > > > 
> > > > But, eventually, I think we should switch all these usages to union
> > > > inany_addr *.    
> > > 
> > > So, we may be able to use union inany_addr in some places, but that's
> > > not the same thing as this: inany_addr carries IPv4 addresses as
> > > mapped IPv6 addresses, it's not switched on a separate af parameter.  
> > 
> > I really meant *a pointer* to union inany_addr, that is:
> >   
> > > We could, of course, define a new type as a simple union of in_addr
> > > and in6_addr.  
> > 
> > ...abusing it instead of using a separate union. On the other hand,
> > given where 'a4' is in there, it's not necessarily the same for
> > (strict) aliasing considerations.
> > 
> > Is "union in10_addr" fashionable enough? We could use A [16], but it's
> > inconvenient to type, and difficult to pronounce.  
> 
> Uh, I haven't heard of either of those.

Right, I just made that up. I was suggesting a new name for this
hypothetical union.

-- 
Stefano