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
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(a)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],
--
2.43.0
On Fri, 8 Dec 2023 01:31:33 +1100 David Gibson <david(a)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(a)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
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(a)gibson.dropbear.id.au> wrote: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.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(a)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.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
On Thu, 28 Dec 2023 13:42:25 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote: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.On Fri, 8 Dec 2023 01:31:33 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Hrm... possibly. The fact that the addr variable in conf_ports() is a char array, not a struct in*_addr might save us.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(a)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.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.Ouch, right, they aren't (again... sarcastically speaking).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.I really meant *a pointer* to union inany_addr, that is: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....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
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(a)gibson.dropbear.id.au> wrote: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/~dgibsonOn Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote: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.On Fri, 8 Dec 2023 01:31:33 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Hrm... possibly. The fact that the addr variable in conf_ports() is a char array, not a struct in*_addr might save us.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(a)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.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.Ouch, right, they aren't (again... sarcastically speaking).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.I really meant *a pointer* to union inany_addr, that is: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....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.
On Sun, 7 Jan 2024 16:35:56 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Thu, Dec 28, 2023 at 11:11:19AM +0100, Stefano Brivio wrote:Right, I just made that up. I was suggesting a new name for this hypothetical union. -- StefanoOn Thu, 28 Dec 2023 13:42:25 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Uh, I haven't heard of either of those.On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote: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.On Fri, 8 Dec 2023 01:31:33 +1100 David Gibson <david(a)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(a)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.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.Ouch, right, they aren't (again... sarcastically speaking).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.I really meant *a pointer* to union inany_addr, that is: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....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.
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(a)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);
--
2.43.0
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(a)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,
--
2.43.0
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(a)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) \ -- 2.43.0
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(a)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;
--
2.43.0
On Fri, 8 Dec 2023 01:31:37 +1100 David Gibson <david(a)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(a)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
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(a)gibson.dropbear.id.au> wrote: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/~dgibsonCurrently 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(a)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).
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(a)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) {
--
2.43.0
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(a)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);
--
2.43.0
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(a)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
--
2.43.0
On Fri, 8 Dec 2023 01:31:32 +1100 David Gibson <david(a)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 NULLApplied, minus first two hunks of 5/8. -- Stefano