Surprisingly little logic is shared between the path for creating a listen()ing socket in the guest namespace versus in the host namespace. Improve this, by extending tcp_sock_init_one() to take a pif parameter indicating where it should open the socket. This allows tcp_ns_sock_init[46]() to be removed entirely. We generalise tcp_sock_init() in the same way, although we don't fully use it yet, due to some subtle differences in how we bind for -t versus -T. Signed-off-by: David Gibson --- conf.c | 2 +- tcp.c | 100 +++++++++++++++++++-------------------------------------- tcp.h | 5 +-- 3 files changed, 37 insertions(+), 70 deletions(-) diff --git a/conf.c b/conf.c index 66b9e634..26f1bcc0 100644 --- a/conf.c +++ b/conf.c @@ -169,7 +169,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, fwd->delta[i] = to - first; if (optname == 't') - ret = tcp_sock_init(c, addr, ifname, i); + ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') ret = udp_sock_init(c, 0, addr, ifname, i); else diff --git a/tcp.c b/tcp.c index b8a98523..428bac7b 100644 --- a/tcp.c +++ b/tcp.c @@ -2530,29 +2530,42 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, /** * tcp_sock_init_one() - Initialise listening socket for address and port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL for dual stack any * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: fd for the new listening socket, negative error code on failure + * + * If pif == PIF_SPLICE, the caller must have already entered the guest ns. */ -static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr, - const char *ifname, in_port_t port) +static int tcp_sock_init_one(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port) { union tcp_listen_epoll_ref tref = { .port = port, - .pif = PIF_HOST, + .pif = pif, }; + const struct fwd_ports *fwd; int s; - s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, PIF_HOST, addr, - ifname, port, tref.u32); + if (pif == PIF_HOST) + fwd = &c->tcp.fwd_in; + else + fwd = &c->tcp.fwd_out; + + s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, pif, addr, ifname, + port, tref.u32); + + if (fwd->mode == FWD_AUTO) { + int (*socks)[IP_VERSIONS] = pif == PIF_SPLICE ? + tcp_sock_ns : tcp_sock_init_ext; - if (c->tcp.fwd_in.mode == FWD_AUTO) { if (!addr || inany_v4(addr)) - tcp_sock_init_ext[port][V4] = s < 0 ? -1 : s; + socks[port][V4] = s < 0 ? -1 : s; if (!addr || !inany_v4(addr)) - tcp_sock_init_ext[port][V6] = s < 0 ? -1 : s; + socks[port][V6] = s < 0 ? -1 : s; } if (s < 0) @@ -2564,14 +2577,16 @@ static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr, /** * tcp_sock_init() - Create listening sockets for a given host ("inbound") port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL if not configured * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: 0 on (partial) success, negative error code on (complete) failure */ -int tcp_sock_init(const struct ctx *c, const union inany_addr *addr, - const char *ifname, in_port_t port) +int tcp_sock_init(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port) { int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; @@ -2579,72 +2594,23 @@ int tcp_sock_init(const struct ctx *c, const union inany_addr *addr, if (!addr && c->ifi4 && c->ifi6) /* Attempt to get a dual stack socket */ - if (tcp_sock_init_one(c, NULL, ifname, port) >= 0) + if (tcp_sock_init_one(c, pif, NULL, ifname, port) >= 0) return 0; /* Otherwise create a socket per IP version */ if ((!addr || inany_v4(addr)) && c->ifi4) - r4 = tcp_sock_init_one(c, addr ? addr : &inany_any4, - ifname, port); + r4 = tcp_sock_init_one(c, pif, + addr ? addr : &inany_any4, ifname, port); if ((!addr || !inany_v4(addr)) && c->ifi6) - r6 = tcp_sock_init_one(c, addr ? addr : &inany_any6, - ifname, port); + r6 = tcp_sock_init_one(c, pif, + addr ? addr : &inany_any6, ifname, port); if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6)) return 0; return r4 < 0 ? r4 : r6; } - -/** - * tcp_ns_sock_init4() - Init socket to listen for outbound IPv4 connections - * @c: Execution context - * @port: Port, host order - */ -static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port) -{ - union tcp_listen_epoll_ref tref = { - .port = port, - .pif = PIF_SPLICE, - }; - int s; - - ASSERT(c->mode == MODE_PASTA); - - s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, PIF_SPLICE, &inany_loopback4, - NULL, port, tref.u32); - if (s < 0) - s = -1; - - if (c->tcp.fwd_out.mode == FWD_AUTO) - tcp_sock_ns[port][V4] = s; -} - -/** - * tcp_ns_sock_init6() - Init socket to listen for outbound IPv6 connections - * @c: Execution context - * @port: Port, host order - */ -static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port) -{ - union tcp_listen_epoll_ref tref = { - .port = port, - .pif = PIF_SPLICE, - }; - int s; - - ASSERT(c->mode == MODE_PASTA); - - s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, PIF_SPLICE, &inany_loopback6, - NULL, port, tref.u32); - if (s < 0) - s = -1; - - if (c->tcp.fwd_out.mode == FWD_AUTO) - tcp_sock_ns[port][V6] = s; -} - /** * tcp_ns_sock_init() - Init socket to listen for spliced outbound connections * @c: Execution context @@ -2655,9 +2621,9 @@ static void tcp_ns_sock_init(const struct ctx *c, in_port_t port) ASSERT(!c->no_tcp); if (c->ifi4) - tcp_ns_sock_init4(c, port); + tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback4, NULL, port); if (c->ifi6) - tcp_ns_sock_init6(c, port); + tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback6, NULL, port); } /** @@ -2855,7 +2821,7 @@ static void tcp_port_rebind(struct ctx *c, bool outbound) if (outbound) tcp_ns_sock_init(c, port); else - tcp_sock_init(c, NULL, NULL, port); + tcp_sock_init(c, PIF_HOST, NULL, NULL, port); } } } diff --git a/tcp.h b/tcp.h index 00823867..de6b9f93 100644 --- a/tcp.h +++ b/tcp.h @@ -18,8 +18,9 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, uint32_t flow_lbl, const struct pool *p, int idx, const struct timespec *now); -int tcp_sock_init(const struct ctx *c, const union inany_addr *addr, - const char *ifname, in_port_t port); +int tcp_sock_init(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port); int tcp_init(struct ctx *c); void tcp_port_rebind_all(struct ctx *c); void tcp_timer(const struct ctx *c, const struct timespec *now); -- 2.51.1

sock_l4_sa() has a somewhat confusing 'v6only' option controlling whether to set the IPV6_V6ONLY socket option. Usually it's set when the given address is IPv6, but not when we want to create a dual stack listening socket. The latter only makes sense when the address is :: however. Clarify this by only keeping the v6only option in an internal helper sock_l4_(). External users will call either sock_l4() which always creates a socket bound to a specific IP version, or sock_l4_dualstack() which creates a dual stack socket, but takes only a port not an address. We drop the '_sa' suffix while we're at it - it exists because this used to be an internal version with a sock_l4() wrapper. The wrapper no longer exists so the '_sa' is no longer useful. Signed-off-by: David Gibson --- flow.c | 6 ++---- pif.c | 12 ++++-------- util.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- util.h | 6 ++++-- 4 files changed, 52 insertions(+), 17 deletions(-) diff --git a/flow.c b/flow.c index 11e3f283..8d72965c 100644 --- a/flow.c +++ b/flow.c @@ -183,8 +183,7 @@ static int flowside_sock_splice(void *arg) ns_enter(a->c); - a->fd = sock_l4_sa(a->c, a->type, a->sa, NULL, - a->sa->sa_family == AF_INET6); + a->fd = sock_l4(a->c, a->type, a->sa, NULL); a->err = errno; return 0; @@ -219,8 +218,7 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, else if (sa.sa_family == AF_INET6) ifname = c->ip6.ifname_out; - return sock_l4_sa(c, type, &sa, ifname, - sa.sa_family == AF_INET6); + return sock_l4(c, type, &sa, ifname); case PIF_SPLICE: { struct flowside_sock_args args = { diff --git a/pif.c b/pif.c index e6a50806..85904f35 100644 --- a/pif.c +++ b/pif.c @@ -76,11 +76,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, const union inany_addr *addr, const char *ifname, in_port_t port, uint32_t data) { - union sockaddr_inany sa = { - .sa6.sin6_family = AF_INET6, - .sa6.sin6_addr = in6addr_any, - .sa6.sin6_port = htons(port), - }; union epoll_ref ref; int ret; @@ -93,11 +88,12 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, } if (!addr) { - ref.fd = sock_l4_sa(c, type, &sa, ifname, false); + ref.fd = sock_l4_dualstack(c, type, port, ifname); } else { + union sockaddr_inany sa; + pif_sockaddr(c, &sa, pif, addr, port); - ref.fd = sock_l4_sa(c, type, &sa, ifname, - sa.sa_family == AF_INET6); + ref.fd = sock_l4(c, type, &sa, ifname); } if (ref.fd < 0) diff --git a/util.c b/util.c index 7df50c69..1108a1da 100644 --- a/util.c +++ b/util.c @@ -40,7 +40,7 @@ #endif /** - * sock_l4_sa() - Create and bind socket to socket address, add to epoll list + * sock_l4_() - Create and bind socket to socket address * @c: Execution context * @type: epoll type * @sa: Socket address to bind to @@ -49,8 +49,9 @@ * * Return: newly created socket, negative error code on failure */ -int sock_l4_sa(const struct ctx *c, enum epoll_type type, - const union sockaddr_inany *sa, const char *ifname, bool v6only) +static int sock_l4_(const struct ctx *c, enum epoll_type type, + const union sockaddr_inany *sa, const char *ifname, + bool v6only) { sa_family_t af = sa->sa_family; bool freebind = false; @@ -167,6 +168,44 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type, return fd; } +/** + * sock_l4() - Create and bind socket to given address + * @c: Execution context + * @type: epoll type + * @sa: Socket address to bind to + * @ifname: Interface for binding, NULL for any + * + * Return: newly created socket, negative error code on failure + */ +int sock_l4(const struct ctx *c, enum epoll_type type, + const union sockaddr_inany *sa, const char *ifname) +{ + return sock_l4_(c, type, sa, ifname, sa->sa_family == AF_INET6); +} + +/** + * sock_l4_dualstack() - Create a dual stack socket bound with wildcard address + * @c: Execution context + * @type: epoll type + * @port Port to bind to (:: and 0.0.0.0) + * @ifname: Interface for binding, NULL for any + * + * Return: newly created socket, negative error code on failure + * + * A dual stack socket is effectively bound to both :: and 0.0.0.0. + */ +int sock_l4_dualstack(const struct ctx *c, enum epoll_type type, + in_port_t port, const char *ifname) +{ + union sockaddr_inany sa = { + .sa6.sin6_family = AF_INET6, + .sa6.sin6_addr = in6addr_any, + .sa6.sin6_port = htons(port), + }; + + return sock_l4_(c, type, &sa, ifname, 0); +} + /** * sock_unix() - Create and bind AF_UNIX socket * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) diff --git a/util.h b/util.h index f8294199..d9fbfe5d 100644 --- a/util.h +++ b/util.h @@ -208,8 +208,10 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, struct ctx; union sockaddr_inany; -int sock_l4_sa(const struct ctx *c, enum epoll_type type, - const union sockaddr_inany *sa, const char *ifname, bool v6only); +int sock_l4(const struct ctx *c, enum epoll_type type, + const union sockaddr_inany *sa, const char *ifname); +int sock_l4_dualstack(const struct ctx *c, enum epoll_type type, + in_port_t port, const char *ifname); int sock_unix(char *sock_path); void sock_probe_mem(struct ctx *c); long timespec_diff_ms(const struct timespec *a, const struct timespec *b); -- 2.51.1

udp_sock_init() takes an 'ns' parameter determining if it creates a socket in the guest namespace or host namespace. Alter it to take a pif parameter instead, like tcp_sock_init(), and use that change to slightly reduce code duplication between the HOST and SPLICE cases. Signed-off-by: David Gibson --- conf.c | 2 +- udp.c | 63 ++++++++++++++++++++++++++++++---------------------------- udp.h | 5 +++-- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/conf.c b/conf.c index 26f1bcc0..08cb50aa 100644 --- a/conf.c +++ b/conf.c @@ -171,7 +171,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, if (optname == 't') ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') - ret = udp_sock_init(c, 0, addr, ifname, i); + ret = udp_sock_init(c, PIF_HOST, addr, ifname, i); else /* No way to check in advance for -T and -U */ ret = 0; diff --git a/udp.c b/udp.c index d414ca32..3b43f136 100644 --- a/udp.c +++ b/udp.c @@ -1104,64 +1104,66 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, /** * udp_sock_init() - Initialise listening sockets for a given port * @c: Execution context - * @ns: In pasta mode, if set, bind with loopback address in namespace + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL if not configured * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: 0 on (partial) success, negative error code on (complete) failure */ -int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr, - const char *ifname, in_port_t port) +int udp_sock_init(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port) { union udp_listen_epoll_ref uref = { - .pif = ns ? PIF_SPLICE : PIF_HOST, + .pif = pif, .port = port, }; int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; + int (*socks)[NUM_PORTS]; ASSERT(!c->no_udp); + ASSERT(pif_is_socket(pif)); - if (!addr && c->ifi4 && c->ifi6 && !ns) { + if (pif == PIF_HOST) + socks = udp_splice_init; + else + socks = udp_splice_ns; + + if (!addr && c->ifi4 && c->ifi6 && pif == PIF_HOST) { int s; /* Attempt to get a dual stack socket */ s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST, NULL, ifname, port, uref.u32); - udp_splice_init[V4][port] = s < 0 ? -1 : s; - udp_splice_init[V6][port] = s < 0 ? -1 : s; + socks[V4][port] = s < 0 ? -1 : s; + socks[V6][port] = s < 0 ? -1 : s; if (IN_INTERVAL(0, FD_REF_MAX, s)) return 0; } if ((!addr || inany_v4(addr)) && c->ifi4) { - if (!ns) { - r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST, - addr ? addr : &inany_any4, ifname, - port, uref.u32); + const union inany_addr *a = addr ? addr : &inany_any4; - udp_splice_init[V4][port] = r4 < 0 ? -1 : r4; - } else { - r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_SPLICE, - &inany_loopback4, ifname, - port, uref.u32); - udp_splice_ns[V4][port] = r4 < 0 ? -1 : r4; - } + if (pif == PIF_SPLICE) + a = &inany_loopback4; + + r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname, + port, uref.u32); + + socks[V4][port] = r4 < 0 ? -1 : r4; } if ((!addr || !inany_v4(addr)) && c->ifi6) { - if (!ns) { - r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST, - addr ? addr : &inany_any6, ifname, - port, uref.u32); + const union inany_addr *a = addr ? addr : &inany_any6; - udp_splice_init[V6][port] = r6 < 0 ? -1 : r6; - } else { - r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_SPLICE, - &inany_loopback6, ifname, - port, uref.u32); - udp_splice_ns[V6][port] = r6 < 0 ? -1 : r6; - } + if (pif == PIF_SPLICE) + a = &inany_loopback6; + + r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname, + port, uref.u32); + + socks[V6][port] = r6 < 0 ? -1 : r6; } if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6)) @@ -1221,7 +1223,8 @@ static void udp_port_rebind(struct ctx *c, bool outbound) if ((c->ifi4 && socks[V4][port] == -1) || (c->ifi6 && socks[V6][port] == -1)) - udp_sock_init(c, outbound, NULL, NULL, port); + udp_sock_init(c, outbound ? PIF_SPLICE : PIF_HOST, + NULL, NULL, port); } } diff --git a/udp.h b/udp.h index f1d83f38..03e8dc54 100644 --- a/udp.h +++ b/udp.h @@ -15,8 +15,9 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, uint8_t ttl, const struct pool *p, int idx, const struct timespec *now); -int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr, - const char *ifname, in_port_t port); +int udp_sock_init(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port); int udp_init(struct ctx *c); void udp_port_rebind_all(struct ctx *c); void udp_update_l2_buf(const unsigned char *eth_d); -- 2.51.1

Currently, outbound forwards (-T, -U) are handled by sockets bound to the loopback address. Typically we create two sockets, one for 127.0.0.1 and one for ::1. This has some disadvantages: * The guest can't connect via 127.0.0.0/8 addresses other than 127.0.0.1 * We can't use dual-stack sockets, we have to have separate sockets for IPv4 and IPv6. The restriction exists for a reason though. If the guest has any interfaces other than pasta (e.g. a VPN tunnel) external hosts could reach the host via the forwards. Especially combined with -T auto / -U auto this would make it very easy to make a mistake with nasty security implications. We can achieve this a different way, however. Don't bind to a specific address, but _do_ use SO_BINDTODEVICE to restrict the sockets to the "lo" interface. Note that although traffic to a local but non-loopback address is passed over the 'lo' interface (as seen by netfilter and dumpcap), it doesn't count as attached to that interface for the purposes of SO_BINDTODEVICE (information from the routing table overrides the "physical" interface). So, this change doesn't help for bug 100. It's also not a complete fix for bug 113, it does however: * Get us a step closer to fixing bug 113 * Slightly simplify the code * Make things a bit easier to allow more flexible binding on the guest in in future Link: https://bugs.passt.top/show_bug.cgi?id=100 Link: https://bugs.passt.top/show_bug.cgi?id=113 Signed-off-by: David Gibson --- pif.c | 6 ------ tcp.c | 19 ++----------------- udp.c | 10 +++------- 3 files changed, 5 insertions(+), 30 deletions(-) diff --git a/pif.c b/pif.c index 85904f35..db447b4f 100644 --- a/pif.c +++ b/pif.c @@ -81,12 +81,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, ASSERT(pif_is_socket(pif)); - if (pif == PIF_SPLICE) { - /* Sanity checks */ - ASSERT(!ifname); - ASSERT(addr && inany_is_loopback(addr)); - } - if (!addr) { ref.fd = sock_l4_dualstack(c, type, port, ifname); } else { diff --git a/tcp.c b/tcp.c index 2abb8be4..bd25952f 100644 --- a/tcp.c +++ b/tcp.c @@ -2618,21 +2618,6 @@ int tcp_sock_init(const struct ctx *c, uint8_t pif, return 0; } -/** - * tcp_ns_sock_init() - Init socket to listen for spliced outbound connections - * @c: Execution context - * @port: Port, host order - */ -static void tcp_ns_sock_init(const struct ctx *c, in_port_t port) -{ - ASSERT(!c->no_tcp); - - if (c->ifi4) - tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback4, NULL, port); - if (c->ifi6) - tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback6, NULL, port); -} - /** * tcp_ns_socks_init() - Bind sockets in namespace for outbound connections * @arg: Execution context @@ -2651,7 +2636,7 @@ static int tcp_ns_socks_init(void *arg) if (!bitmap_isset(c->tcp.fwd_out.map, port)) continue; - tcp_ns_sock_init(c, port); + tcp_sock_init(c, PIF_SPLICE, NULL, "lo", port); } return 0; @@ -2826,7 +2811,7 @@ static void tcp_port_rebind(struct ctx *c, bool outbound) if ((c->ifi4 && socks[port][V4] == -1) || (c->ifi6 && socks[port][V6] == -1)) { if (outbound) - tcp_ns_sock_init(c, port); + tcp_sock_init(c, PIF_SPLICE, NULL, "lo", port); else tcp_sock_init(c, PIF_HOST, NULL, NULL, port); } diff --git a/udp.c b/udp.c index 3d097fbb..f3d445cf 100644 --- a/udp.c +++ b/udp.c @@ -1213,14 +1213,10 @@ static void udp_port_rebind(struct ctx *c, bool outbound) if ((c->ifi4 && socks[V4][port] == -1) || (c->ifi6 && socks[V6][port] == -1)) { - if (outbound) { - udp_sock_init(c, PIF_SPLICE, - &inany_loopback4, NULL, port); - udp_sock_init(c, PIF_SPLICE, - &inany_loopback6, NULL, port); - } else { + if (outbound) + udp_sock_init(c, PIF_SPLICE, NULL, "lo", port); + else udp_sock_init(c, PIF_HOST, NULL, NULL, port); - } } } } -- 2.51.1

sockaddr_inany can contain either an IPv4 or IPv6 socket address, so the relevant length for bind() or connect() can vary. In pif_sockaddr() we return that length, and in sock_l4_sa() we take it as an extra parameter. However, sockaddr_inany always contains exactly a sockaddr_in or a sockaddr_in6 each with a fixed size. Therefore we can derive the relevant length from the family, and don't need to pass it around separately. Make a tiny helper to get the relevant address length, and update all interfaces to use that approach instead. In the process, fix a buglet in tcp_flow_repair_bind(): we passed sizeof(union sockaddr_inany) to bind() instead of the specific length for the address family. Since the sizeof() is always longer than the specific length, this is probably fine, but not theoretically correct. Signed-off-by: David Gibson --- flow.c | 21 ++++++++------------- icmp.c | 3 ++- inany.h | 17 +++++++++++++++++ pif.c | 15 +++++---------- pif.h | 2 +- tcp.c | 20 +++++++++----------- tcp_splice.c | 5 ++--- udp.c | 8 +++----- util.c | 8 +++----- util.h | 4 ++-- 10 files changed, 52 insertions(+), 51 deletions(-) diff --git a/flow.c b/flow.c index 743860d6..11e3f283 100644 --- a/flow.c +++ b/flow.c @@ -163,15 +163,13 @@ static void flowside_from_af(struct flowside *side, sa_family_t af, * @err: Filled in with errno if something failed * @type: Socket epoll type * @sa: Socket address - * @sl: Length of @sa */ struct flowside_sock_args { const struct ctx *c; int fd; int err; enum epoll_type type; - const struct sockaddr *sa; - socklen_t sl; + const union sockaddr_inany *sa; }; /** flowside_sock_splice() - Create and bind socket for PIF_SPLICE based on flowside @@ -185,8 +183,8 @@ static int flowside_sock_splice(void *arg) ns_enter(a->c); - a->fd = sock_l4_sa(a->c, a->type, a->sa, a->sl, NULL, - a->sa->sa_family == AF_INET6); + a->fd = sock_l4_sa(a->c, a->type, a->sa, NULL, + a->sa->sa_family == AF_INET6); a->err = errno; return 0; @@ -207,11 +205,10 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, { const char *ifname = NULL; union sockaddr_inany sa; - socklen_t sl; ASSERT(pif_is_socket(pif)); - pif_sockaddr(c, &sa, &sl, pif, &tgt->oaddr, tgt->oport); + pif_sockaddr(c, &sa, pif, &tgt->oaddr, tgt->oport); switch (pif) { case PIF_HOST: @@ -222,13 +219,12 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, else if (sa.sa_family == AF_INET6) ifname = c->ip6.ifname_out; - return sock_l4_sa(c, type, &sa, sl, ifname, + return sock_l4_sa(c, type, &sa, ifname, sa.sa_family == AF_INET6); case PIF_SPLICE: { struct flowside_sock_args args = { - .c = c, .type = type, - .sa = &sa.sa, .sl = sl, + .c = c, .type = type, .sa = &sa, }; NS_CALL(flowside_sock_splice, &args); errno = args.err; @@ -257,10 +253,9 @@ int flowside_connect(const struct ctx *c, int s, uint8_t pif, const struct flowside *tgt) { union sockaddr_inany sa; - socklen_t sl; - pif_sockaddr(c, &sa, &sl, pif, &tgt->eaddr, tgt->eport); - return connect(s, &sa.sa, sl); + pif_sockaddr(c, &sa, pif, &tgt->eaddr, tgt->eport); + return connect(s, &sa.sa, socklen_inany(&sa)); } /** flow_log_ - Log flow-related message diff --git a/icmp.c b/icmp.c index 35faefb9..9564c496 100644 --- a/icmp.c +++ b/icmp.c @@ -312,8 +312,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, ASSERT(flow_proto[pingf->f.type] == proto); pingf->ts = now->tv_sec; - pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0); + pif_sockaddr(c, &sa, PIF_HOST, &tgt->eaddr, 0); msh.msg_name = &sa; + msh.msg_namelen = socklen_inany(&sa); msh.msg_iov = iov; msh.msg_iovlen = cnt; msh.msg_control = NULL; diff --git a/inany.h b/inany.h index 7ca5cbd3..61b36fb4 100644 --- a/inany.h +++ b/inany.h @@ -67,6 +67,23 @@ union sockaddr_inany { struct sockaddr_in6 sa6; }; +/** socklen_inany() - Get relevant address length for sockaddr_inany address + * @sa: sockaddr_inany socket address + * + * Return: socket address length for bind() or connect(), from IP family in @sa + */ +static inline socklen_t socklen_inany(const union sockaddr_inany *sa) +{ + switch (sa->sa_family) { + case AF_INET: + return sizeof(sa->sa4); + case AF_INET6: + return sizeof(sa->sa6); + default: + ASSERT(0); + } +} + /** inany_v4 - Extract IPv4 address, if present, from IPv[46] address * @addr: IPv4 or IPv6 address * diff --git a/pif.c b/pif.c index 331942e7..e6a50806 100644 --- a/pif.c +++ b/pif.c @@ -30,12 +30,11 @@ static_assert(ARRAY_SIZE(pif_type_str) == PIF_NUM_TYPES, /** pif_sockaddr() - Construct a socket address suitable for an interface * @c: Execution context * @sa: Pointer to sockaddr to fill in - * @sl: Updated to relevant length of initialised @sa * @pif: Interface to create the socket address * @addr: IPv[46] address * @port: Port (host byte order) */ -void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl, +void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, uint8_t pif, const union inany_addr *addr, in_port_t port) { const struct in_addr *v4 = inany_v4(addr); @@ -47,7 +46,6 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl, sa->sa4.sin_addr = *v4; sa->sa4.sin_port = htons(port); memset(&sa->sa4.sin_zero, 0, sizeof(sa->sa4.sin_zero)); - *sl = sizeof(sa->sa4); } else { sa->sa_family = AF_INET6; sa->sa6.sin6_addr = addr->a6; @@ -57,7 +55,6 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl, else sa->sa6.sin6_scope_id = 0; sa->sa6.sin6_flowinfo = 0; - *sl = sizeof(sa->sa6); } } @@ -85,7 +82,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, .sa6.sin6_port = htons(port), }; union epoll_ref ref; - socklen_t sl; int ret; ASSERT(pif_is_socket(pif)); @@ -97,12 +93,11 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, } if (!addr) { - ref.fd = sock_l4_sa(c, type, &sa, sizeof(sa.sa6), - ifname, false); + ref.fd = sock_l4_sa(c, type, &sa, ifname, false); } else { - pif_sockaddr(c, &sa, &sl, pif, addr, port); - ref.fd = sock_l4_sa(c, type, &sa, sl, - ifname, sa.sa_family == AF_INET6); + pif_sockaddr(c, &sa, pif, addr, port); + ref.fd = sock_l4_sa(c, type, &sa, ifname, + sa.sa_family == AF_INET6); } if (ref.fd < 0) diff --git a/pif.h b/pif.h index f029282b..0f7f6672 100644 --- a/pif.h +++ b/pif.h @@ -57,7 +57,7 @@ static inline bool pif_is_socket(uint8_t pif) return pif == PIF_HOST || pif == PIF_SPLICE; } -void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl, +void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, uint8_t pif, const union inany_addr *addr, in_port_t port); int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, const union inany_addr *addr, const char *ifname, diff --git a/tcp.c b/tcp.c index 3202d338..b8a98523 100644 --- a/tcp.c +++ b/tcp.c @@ -1457,12 +1457,11 @@ static void tcp_bind_outbound(const struct ctx *c, { const struct flowside *tgt = &conn->f.side[TGTSIDE]; union sockaddr_inany bind_sa; - socklen_t sl; - pif_sockaddr(c, &bind_sa, &sl, PIF_HOST, &tgt->oaddr, tgt->oport); + pif_sockaddr(c, &bind_sa, PIF_HOST, &tgt->oaddr, tgt->oport); if (!inany_is_unspecified(&tgt->oaddr) || tgt->oport) { - if (bind(s, &bind_sa.sa, sl)) { + if (bind(s, &bind_sa.sa, socklen_inany(&bind_sa))) { char sstr[INANY_ADDRSTRLEN]; flow_dbg_perror(conn, @@ -1522,7 +1521,6 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, union flow *flow; int s = -1, mss; uint64_t hash; - socklen_t sl; if (!(flow = flow_alloc())) return; @@ -1555,7 +1553,7 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, if ((s = tcp_conn_sock(af)) < 0) goto cancel; - pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, tgt->eport); + pif_sockaddr(c, &sa, PIF_HOST, &tgt->eaddr, tgt->eport); /* Use bind() to check if the target address is local (EADDRINUSE or * similar) and already bound, and set the LOCAL flag in that case. @@ -1567,7 +1565,7 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, * * So, if bind() succeeds, close the socket, get a new one, and proceed. */ - if (bind(s, &sa.sa, sl)) { + if (bind(s, &sa.sa, socklen_inany(&sa))) { if (errno != EADDRNOTAVAIL && errno != EACCES) conn_flag(c, conn, LOCAL); } else { @@ -1607,7 +1605,7 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, tcp_bind_outbound(c, conn, s); - if (connect(s, &sa.sa, sl)) { + if (connect(s, &sa.sa, socklen_inany(&sa))) { if (errno != EINPROGRESS) { tcp_rst(c, conn); goto cancel; @@ -1626,7 +1624,8 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, tcp_epoll_ctl(c, conn); if (c->mode == MODE_VU) { /* To rebind to same oport after migration */ - sl = sizeof(sa); + socklen_t sl = sizeof(sa); + if (getsockname(s, &sa.sa, &sl) || inany_from_sockaddr(&tgt->oaddr, &tgt->oport, &sa) < 0) err_perror("Can't get local address for socket %i", s); @@ -3611,11 +3610,10 @@ static int tcp_flow_repair_bind(const struct ctx *c, struct tcp_tap_conn *conn) { const struct flowside *sockside = HOSTFLOW(conn); union sockaddr_inany a; - socklen_t sl; - pif_sockaddr(c, &a, &sl, PIF_HOST, &sockside->oaddr, sockside->oport); + pif_sockaddr(c, &a, PIF_HOST, &sockside->oaddr, sockside->oport); - if (bind(conn->sock, &a.sa, sizeof(a))) { + if (bind(conn->sock, &a.sa, socklen_inany(&a))) { int rc = -errno; flow_perror(conn, "Failed to bind socket for migrated flow"); return rc; diff --git a/tcp_splice.c b/tcp_splice.c index 5efd8597..717766af 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -352,7 +352,6 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn) sa_family_t af = inany_v4(&tgt->eaddr) ? AF_INET : AF_INET6; uint8_t tgtpif = conn->f.pif[TGTSIDE]; union sockaddr_inany sa; - socklen_t sl; int one = 1; if (tgtpif == PIF_HOST) @@ -380,9 +379,9 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn) conn->s[1]); } - pif_sockaddr(c, &sa, &sl, tgtpif, &tgt->eaddr, tgt->eport); + pif_sockaddr(c, &sa, tgtpif, &tgt->eaddr, tgt->eport); - if (connect(conn->s[1], &sa.sa, sl)) { + if (connect(conn->s[1], &sa.sa, socklen_inany(&sa))) { if (errno != EINPROGRESS) { flow_trace(conn, "Couldn't connect socket for splice: %s", strerror_(errno)); diff --git a/udp.c b/udp.c index 9c009502..d414ca32 100644 --- a/udp.c +++ b/udp.c @@ -780,7 +780,6 @@ static void udp_sock_to_sock(const struct ctx *c, int from_s, int n, const struct udp_flow *uflow = udp_at_sidx(tosidx); uint8_t topif = pif_at_sidx(tosidx); int to_s = uflow->s[tosidx.sidei]; - socklen_t sl; int i; if ((n = udp_sock_recv(c, from_s, udp_mh_recv, n)) <= 0) @@ -791,7 +790,7 @@ static void udp_sock_to_sock(const struct ctx *c, int from_s, int n, = udp_mh_recv[i].msg_len; } - pif_sockaddr(c, &udp_splice_to, &sl, topif, + pif_sockaddr(c, &udp_splice_to, topif, &toside->eaddr, toside->eport); sendmmsg(to_s, udp_mh_splice, n, MSG_NOSIGNAL); @@ -999,7 +998,6 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, flow_sidx_t tosidx; in_port_t src, dst; uint8_t topif; - socklen_t sl; ASSERT(!c->no_udp); @@ -1041,7 +1039,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, s = uflow->s[tosidx.sidei]; ASSERT(s >= 0); - pif_sockaddr(c, &to_sa, &sl, topif, &toside->eaddr, toside->eport); + pif_sockaddr(c, &to_sa, topif, &toside->eaddr, toside->eport); for (i = 0, j = 0; i < (int)p->count - idx && j < UIO_MAXIOV; i++) { const struct udphdr *uh_send; @@ -1054,7 +1052,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, return p->count - idx; mm[i].msg_hdr.msg_name = &to_sa; - mm[i].msg_hdr.msg_namelen = sl; + mm[i].msg_hdr.msg_namelen = socklen_inany(&to_sa); if (data.cnt) { int cnt; diff --git a/util.c b/util.c index ab23463b..7df50c69 100644 --- a/util.c +++ b/util.c @@ -44,17 +44,15 @@ * @c: Execution context * @type: epoll type * @sa: Socket address to bind to - * @sl: Length of @sa * @ifname: Interface for binding, NULL for any * @v6only: Set IPV6_V6ONLY socket option * * Return: newly created socket, negative error code on failure */ int sock_l4_sa(const struct ctx *c, enum epoll_type type, - const void *sa, socklen_t sl, - const char *ifname, bool v6only) + const union sockaddr_inany *sa, const char *ifname, bool v6only) { - sa_family_t af = ((const struct sockaddr *)sa)->sa_family; + sa_family_t af = sa->sa_family; bool freebind = false; int fd, y = 1, ret; uint8_t proto; @@ -146,7 +144,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type, } } - if (bind(fd, sa, sl) < 0) { + if (bind(fd, &sa->sa, socklen_inany(sa)) < 0) { /* We'll fail to bind to low ports if we don't have enough * capabilities, and we'll fail to bind on already bound ports, * this is fine. This might also fail for ICMP because of a diff --git a/util.h b/util.h index a0b2ada6..f8294199 100644 --- a/util.h +++ b/util.h @@ -206,10 +206,10 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, #include "packet.h" struct ctx; +union sockaddr_inany; int sock_l4_sa(const struct ctx *c, enum epoll_type type, - const void *sa, socklen_t sl, - const char *ifname, bool v6only); + const union sockaddr_inany *sa, const char *ifname, bool v6only); int sock_unix(char *sock_path); void sock_probe_mem(struct ctx *c); long timespec_diff_ms(const struct timespec *a, const struct timespec *b); -- 2.51.1