Sometimes we use sa_family_t for variables and parameters containing a socket address family, other times we use a plain int. Since sa_family_t is what's actually used in struct sockaddr and friends, standardise on that. Signed-off-by: David Gibson # Conflicts: # tcp_conn.h --- icmp.c | 6 +++--- icmp.h | 4 ++-- inany.h | 3 ++- tcp.c | 12 ++++++------ tcp.h | 2 +- tcp_conn.h | 2 +- tcp_splice.c | 2 +- udp.c | 2 +- udp.h | 2 +- util.c | 2 +- util.h | 2 +- 11 files changed, 20 insertions(+), 19 deletions(-) diff --git a/icmp.c b/icmp.c index 9434fc5a..faa38c81 100644 --- a/icmp.c +++ b/icmp.c @@ -62,7 +62,7 @@ static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS]; * @af: Address family (AF_INET or AF_INET6) * @ref: epoll reference */ -void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref ref) +void icmp_sock_handler(const struct ctx *c, sa_family_t af, union epoll_ref ref) { struct icmp_id_sock *const id_sock = af == AF_INET ? &icmp_id_map[V4][ref.icmp.id] : &icmp_id_map[V6][ref.icmp.id]; @@ -156,7 +156,7 @@ static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_sock) * Return: Newly opened ping socket fd, or -1 on failure */ static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock *id_sock, - int af, uint16_t id) + sa_family_t af, uint16_t id) { uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6"; @@ -209,7 +209,7 @@ cancel: * * Return: count of consumed packets (always 1, even if malformed) */ -int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af, +int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, const struct pool *p, const struct timespec *now) { diff --git a/icmp.h b/icmp.h index 00835971..263101db 100644 --- a/icmp.h +++ b/icmp.h @@ -10,8 +10,8 @@ struct ctx; -void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref ref); -int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af, +void icmp_sock_handler(const struct ctx *c, sa_family_t af, union epoll_ref ref); +int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, const struct pool *p, const struct timespec *now); void icmp_timer(const struct ctx *c, const struct timespec *now); diff --git a/inany.h b/inany.h index 90e98f16..fe652ff7 100644 --- a/inany.h +++ b/inany.h @@ -60,7 +60,8 @@ static inline bool inany_equals(const union inany_addr *a, * @af: Address family of @addr * @addr: struct in_addr (IPv4) or struct in6_addr (IPv6) */ -static inline void inany_from_af(union inany_addr *aa, int af, const void *addr) +static inline void inany_from_af(union inany_addr *aa, + sa_family_t af, const void *addr) { if (af == AF_INET6) { aa->a6 = *((struct in6_addr *)addr); diff --git a/tcp.c b/tcp.c index 905d26f6..fdf56713 100644 --- a/tcp.c +++ b/tcp.c @@ -1262,7 +1262,7 @@ static void tcp_hash_remove(const struct ctx *c, * Return: connection pointer, if found, -ENOENT otherwise */ static struct tcp_tap_conn *tcp_hash_lookup(const struct ctx *c, - int af, const void *faddr, + sa_family_t af, const void *faddr, in_port_t eport, in_port_t fport) { union inany_addr aany; @@ -1904,8 +1904,8 @@ static void tcp_bind_outbound(const struct ctx *c, int s, sa_family_t af) * @optlen: Bytes in options: caller MUST ensure available length * @now: Current timestamp */ -static void tcp_conn_from_tap(struct ctx *c, - int af, const void *saddr, const void *daddr, +static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, + const void *saddr, const void *daddr, const struct tcphdr *th, const char *opts, size_t optlen, const struct timespec *now) { @@ -2480,7 +2480,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn, * * Return: count of consumed packets */ -int tcp_tap_handler(struct ctx *c, uint8_t pif, int af, +int tcp_tap_handler(struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, const struct pool *p, int idx, const struct timespec *now) { @@ -2857,7 +2857,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, +static int tcp_sock_init_af(const struct ctx *c, sa_family_t af, in_port_t port, const void *addr, const char *ifname) { union tcp_listen_epoll_ref tref = { @@ -3009,7 +3009,7 @@ static int tcp_ns_socks_init(void *arg) * @pool: Pool of sockets to refill * @af: Address family to use */ -void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af) +void tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af) { int i; diff --git a/tcp.h b/tcp.h index b9f546d3..875006ed 100644 --- a/tcp.h +++ b/tcp.h @@ -14,7 +14,7 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref); void tcp_listen_handler(struct ctx *c, union epoll_ref ref, const struct timespec *now); void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events); -int tcp_tap_handler(struct ctx *c, uint8_t pif, int af, +int tcp_tap_handler(struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, const struct pool *p, int idx, const struct timespec *now); int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, diff --git a/tcp_conn.h b/tcp_conn.h index a5f5cfe0..20c7cb8b 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -160,7 +160,7 @@ bool tcp_splice_flow_defer(union flow *flow); void tcp_splice_timer(const struct ctx *c, union flow *flow); int tcp_conn_pool_sock(int pool[]); int tcp_conn_new_sock(const struct ctx *c, sa_family_t af); -void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af); +void tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af); void tcp_splice_refill(const struct ctx *c); #endif /* TCP_CONN_H */ diff --git a/tcp_splice.c b/tcp_splice.c index 26d32065..cc9745e8 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -399,7 +399,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, */ if (pif == PIF_SPLICE) { int *p = CONN_V6(conn) ? init_sock_pool6 : init_sock_pool4; - int af = CONN_V6(conn) ? AF_INET6 : AF_INET; + sa_family_t af = CONN_V6(conn) ? AF_INET6 : AF_INET; s = tcp_conn_pool_sock(p); if (s < 0) diff --git a/udp.c b/udp.c index b5b8f8a7..c839e269 100644 --- a/udp.c +++ b/udp.c @@ -814,7 +814,7 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, * #syscalls sendmmsg */ int udp_tap_handler(struct ctx *c, uint8_t pif, - int af, const void *saddr, const void *daddr, + sa_family_t af, const void *saddr, const void *daddr, const struct pool *p, int idx, const struct timespec *now) { struct mmsghdr mm[UIO_MAXIOV]; diff --git a/udp.h b/udp.h index 087e4820..f3d5d6d2 100644 --- a/udp.h +++ b/udp.h @@ -11,7 +11,7 @@ void udp_portmap_clear(void); void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now); -int udp_tap_handler(struct ctx *c, uint8_t pif, int af, +int udp_tap_handler(struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, const struct pool *p, int idx, const struct timespec *now); int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, diff --git a/util.c b/util.c index 21b35ff9..8acce233 100644 --- a/util.c +++ b/util.c @@ -97,7 +97,7 @@ found: * * Return: newly created socket, negative error code on failure */ -int sock_l4(const struct ctx *c, int af, uint8_t proto, +int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto, const void *bind_addr, const char *ifname, uint16_t port, uint32_t data) { diff --git a/util.h b/util.h index d2320f8c..e0df26c6 100644 --- a/util.h +++ b/util.h @@ -212,7 +212,7 @@ struct ipv6_opt_hdr { __attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); } char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto, size_t *dlen); -int sock_l4(const struct ctx *c, int af, uint8_t proto, +int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto, const void *bind_addr, const char *ifname, uint16_t port, uint32_t data); void sock_probe_mem(struct ctx *c); -- 2.43.0

Add this helper to format an inany into either IPv4 or IPv6 text format as appropriate. Signed-off-by: David Gibson --- Makefile | 6 +++--- inany.c | 35 +++++++++++++++++++++++++++++++++++ inany.h | 4 ++++ 3 files changed, 42 insertions(+), 3 deletions(-) create mode 100644 inany.c diff --git a/Makefile b/Makefile index af4fa87e..1c709229 100644 --- a/Makefile +++ b/Makefile @@ -45,9 +45,9 @@ FLAGS += -DVERSION=\"$(VERSION)\" FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c icmp.c \ - igmp.c isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c \ - passt.c pasta.c pcap.c pif.c port_fwd.c tap.c tcp.c tcp_splice.c udp.c \ - util.c + igmp.c inany.c isolation.c lineread.c log.c mld.c ndp.c netlink.c \ + packet.c passt.c pasta.c pcap.c pif.c port_fwd.c tap.c tcp.c \ + tcp_splice.c udp.c util.c QRAP_SRCS = qrap.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) diff --git a/inany.c b/inany.c new file mode 100644 index 00000000..edf0b055 --- /dev/null +++ b/inany.c @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright Red Hat + * Author: David Gibson + * + * inany.c - Types and helpers for handling addresses which could be + * IPv6 or IPv4 (encoded as IPv4-mapped IPv6 addresses) + */ + +#include +#include +#include +#include +#include + +#include "util.h" +#include "siphash.h" +#include "inany.h" + +/** inany_ntop - Convert an IPv[46] address to text format + * @src: IPv[46] address + * @dst: output buffer, minimum INANY_ADDRSTRLEN bytes + * @size: size of buffer at @dst + * + * Return: On success, a non-null pointer to @dst, NULL on failure + */ +/* cppcheck-suppress unusedFunction */ +const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size) +{ + const struct in_addr *v4 = inany_v4(src); + + if (v4) + return inet_ntop(AF_INET, v4, dst, size); + + return inet_ntop(AF_INET6, &src->a6, dst, size); +} diff --git a/inany.h b/inany.h index 2058f145..bf731166 100644 --- a/inany.h +++ b/inany.h @@ -162,4 +162,8 @@ static inline void inany_siphash_feed(struct siphash_state *state, siphash_feed(state, (uint64_t)aa->u32[2] << 32 | aa->u32[3]); } +#define INANY_ADDRSTRLEN MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN) + +const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size); + #endif /* INANY_H */ -- 2.43.0

There are a number of places where we want to handle either a sockaddr_in or a sockaddr_in6. In some of those we use a void *, which works ok and matches some standard library interfaces, but doesn't give a signature level hint that we're dealing with only sockaddr_in or sockaddr_in6, not (say) sockaddr_un or another type of socket address. Other places we use a sockaddr_storage, which also works, but has the same problem in addition to allocating more on the stack than we need to. Introduce union sockaddr_inany to explictly handle this case: it has variants for sockaddr_in and sockaddr_in6. Use it in a number of places where it's easy to do so. Signed-off-by: David Gibson --- icmp.c | 18 ++++++------------ inany.h | 33 +++++++++++++++++++++------------ tcp.c | 11 +++++------ tcp_splice.c | 2 +- tcp_splice.h | 3 ++- 5 files changed, 35 insertions(+), 32 deletions(-) diff --git a/icmp.c b/icmp.c index faa38c81..fb2fcafc 100644 --- a/icmp.c +++ b/icmp.c @@ -36,6 +36,8 @@ #include "passt.h" #include "tap.h" #include "log.h" +#include "siphash.h" +#include "inany.h" #include "icmp.h" #define ICMP_ECHO_TIMEOUT 60 /* s, timeout for ICMP socket activity */ @@ -67,13 +69,9 @@ void icmp_sock_handler(const struct ctx *c, sa_family_t af, union epoll_ref ref) struct icmp_id_sock *const id_sock = af == AF_INET ? &icmp_id_map[V4][ref.icmp.id] : &icmp_id_map[V6][ref.icmp.id]; const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6"; - char buf[USHRT_MAX]; - union { - struct sockaddr sa; - struct sockaddr_in sa4; - struct sockaddr_in6 sa6; - } sr; + union sockaddr_inany sr; socklen_t sl = sizeof(sr); + char buf[USHRT_MAX]; uint16_t seq; ssize_t n; @@ -86,7 +84,7 @@ void icmp_sock_handler(const struct ctx *c, sa_family_t af, union epoll_ref ref) pname, strerror(errno)); return; } - if (sr.sa.sa_family != af) + if (sr.sa_family != af) goto unexpected; if (af == AF_INET) { @@ -214,11 +212,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const struct pool *p, const struct timespec *now) { const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6"; - union { - struct sockaddr sa; - struct sockaddr_in sa4; - struct sockaddr_in6 sa6; - } sa = { .sa.sa_family = af }; + union sockaddr_inany sa = { .sa_family = af }; const socklen_t sl = af == AF_INET ? sizeof(sa.sa4) : sizeof(sa.sa6); struct icmp_id_sock *id_sock; uint16_t id, seq; diff --git a/inany.h b/inany.h index 4c4b36c3..ae6d9b25 100644 --- a/inany.h +++ b/inany.h @@ -9,6 +9,8 @@ #ifndef INANY_H #define INANY_H +struct siphash_state; + /** union inany_addr - Represents either an IPv4 or IPv6 address * @a6: Address as an IPv6 address, may be IPv4-mapped * @v4mapped.zero: All zero-bits for an IPv4 address @@ -41,6 +43,19 @@ extern const union inany_addr inany_any4; #define in4addr_loopback (inany_loopback4.v4mapped.a4) #define in4addr_any (inany_any4.v4mapped.a4) +/** union sockaddr_inany - Either a sockaddr_in or a sockaddr_in6 + * @sa_family: Address family, AF_INET or AF_INET6 + * @sa: Plain struct sockaddr (useful to avoid casts) + * @sa4: IPv4 socket address, valid if sa_family == AF_INET + * @sa6: IPv6 socket address, valid if sa_family == AF_INET6 + */ +union sockaddr_inany { + sa_family_t sa_family; + struct sockaddr sa; + struct sockaddr_in sa4; + struct sockaddr_in6 sa6; +}; + /** inany_v4 - Extract IPv4 address, if present, from IPv[46] address * @addr: IPv4 or IPv6 address * @@ -137,23 +152,17 @@ static inline void inany_from_af(union inany_addr *aa, /** inany_from_sockaddr - Extract IPv[46] address and port number from sockaddr * @aa: Pointer to store IPv[46] address * @port: Pointer to store port number, host order - * @addr: struct sockaddr_in (IPv4) or struct sockaddr_in6 (IPv6) + * @addr: AF_INET or AF_INET6 socket address */ static inline void inany_from_sockaddr(union inany_addr *aa, in_port_t *port, - const void *addr) + const union sockaddr_inany *sa) { - const struct sockaddr *sa = (const struct sockaddr *)addr; - if (sa->sa_family == AF_INET6) { - struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *)sa; - - inany_from_af(aa, AF_INET6, &sa6->sin6_addr); - *port = ntohs(sa6->sin6_port); + inany_from_af(aa, AF_INET6, &sa->sa6.sin6_addr); + *port = ntohs(sa->sa6.sin6_port); } else if (sa->sa_family == AF_INET) { - struct sockaddr_in *sa4 = (struct sockaddr_in *)sa; - - inany_from_af(aa, AF_INET, &sa4->sin_addr); - *port = ntohs(sa4->sin_port); + inany_from_af(aa, AF_INET, &sa->sa4.sin_addr); + *port = ntohs(sa->sa4.sin_port); } else { /* Not valid to call with other address families */ ASSERT(0); diff --git a/tcp.c b/tcp.c index 79b7b4d5..2bba3000 100644 --- a/tcp.c +++ b/tcp.c @@ -2666,7 +2666,7 @@ static void tcp_snat_inbound(const struct ctx *c, union inany_addr *addr) static void tcp_tap_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, struct tcp_tap_conn *conn, int s, - const struct sockaddr *sa, + const union sockaddr_inany *sa, const struct timespec *now) { conn->f.type = FLOW_TCP; @@ -2702,7 +2702,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, void tcp_listen_handler(struct ctx *c, union epoll_ref ref, const struct timespec *now) { - struct sockaddr_storage sa; + union sockaddr_inany sa; socklen_t sl = sizeof(sa); union flow *flow; int s; @@ -2710,17 +2710,16 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, if (c->no_tcp || !(flow = flow_alloc())) return; - s = accept4(ref.fd, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); + s = accept4(ref.fd, &sa.sa, &sl, SOCK_NONBLOCK); if (s < 0) goto cancel; if (c->mode == MODE_PASTA && tcp_splice_conn_from_sock(c, ref.tcp_listen, &flow->tcp_splice, - s, (struct sockaddr *)&sa)) + s, &sa)) return; - tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s, - (struct sockaddr *)&sa, now); + tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s, &sa, now); return; cancel: diff --git a/tcp_splice.c b/tcp_splice.c index 4ecc178b..9fd49412 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -438,7 +438,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, bool tcp_splice_conn_from_sock(const struct ctx *c, union tcp_listen_epoll_ref ref, struct tcp_splice_conn *conn, int s, - const struct sockaddr *sa) + const union sockaddr_inany *sa) { union inany_addr aany; in_port_t port; diff --git a/tcp_splice.h b/tcp_splice.h index 18193e4e..20f41b39 100644 --- a/tcp_splice.h +++ b/tcp_splice.h @@ -7,13 +7,14 @@ #define TCP_SPLICE_H struct tcp_splice_conn; +union sockaddr_inany; void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events); bool tcp_splice_conn_from_sock(const struct ctx *c, union tcp_listen_epoll_ref ref, struct tcp_splice_conn *conn, int s, - const struct sockaddr *sa); + const union sockaddr_inany *sa); void tcp_splice_init(struct ctx *c); #endif /* TCP_SPLICE_H */ -- 2.43.0

The epoll references for both TCP listening sockets and UDP sockets includes a port number. This gives the destination port that traffic to that socket will be sent to on the other side. That will usually be the same as the socket's bound port, but might not if the -t, -u, -T or -U options are given with different original and forwarded port numbers. As we move towards a more flexible forwarding model for passt, it's going to become possible for that destination port to vary depending on more things (for example the source or destination address). So, it will no longer make sense to have a fixed value for a listening socket. Change to simpler semantics where this field in the reference gives the bound port of the socket. We apply the translations to the correct destination port later on, when we're actually forwarding. Signed-off-by: David Gibson --- tcp.c | 8 ++++---- tcp.h | 2 +- tcp_splice.c | 4 ++++ udp.c | 14 ++++++++------ 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/tcp.c b/tcp.c index 2bba3000..3722dc09 100644 --- a/tcp.c +++ b/tcp.c @@ -2676,7 +2676,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, conn_event(c, conn, SOCK_ACCEPTED); inany_from_sockaddr(&conn->faddr, &conn->fport, sa); - conn->eport = ref.port; + conn->eport = ref.port + c->tcp.fwd_in.delta[ref.port]; tcp_snat_inbound(c, &conn->faddr); @@ -2860,7 +2860,7 @@ static int tcp_sock_init_af(const struct ctx *c, sa_family_t af, in_port_t port, const void *addr, const char *ifname) { union tcp_listen_epoll_ref tref = { - .port = port + c->tcp.fwd_in.delta[port], + .port = port, .pif = PIF_HOST, }; int s; @@ -2922,7 +2922,7 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port) { union tcp_listen_epoll_ref tref = { - .port = port + c->tcp.fwd_out.delta[port], + .port = port, .pif = PIF_SPLICE, }; int s; @@ -2948,7 +2948,7 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port) static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port) { union tcp_listen_epoll_ref tref = { - .port = port + c->tcp.fwd_out.delta[port], + .port = port, .pif = PIF_SPLICE, }; int s; diff --git a/tcp.h b/tcp.h index 875006ed..5e6756d4 100644 --- a/tcp.h +++ b/tcp.h @@ -37,7 +37,7 @@ union tcp_epoll_ref { /** * union tcp_listen_epoll_ref - epoll reference portion for TCP listening - * @port: Port number we're forwarding *to* (listening port plus delta) + * @port: Bound port number of the socket * @pif: pif in which the socket is listening * @u32: Opaque u32 value of reference */ diff --git a/tcp_splice.c b/tcp_splice.c index 9fd49412..40ecb5d4 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -401,6 +401,8 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, int *p = CONN_V6(conn) ? init_sock_pool6 : init_sock_pool4; sa_family_t af = CONN_V6(conn) ? AF_INET6 : AF_INET; + port += c->tcp.fwd_out.delta[port]; + s = tcp_conn_pool_sock(p); if (s < 0) s = tcp_conn_new_sock(c, af); @@ -409,6 +411,8 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, ASSERT(pif == PIF_HOST); + port += c->tcp.fwd_in.delta[port]; + /* If pool is empty, refill it first */ if (p[TCP_SOCK_POOL_SIZE-1] < 0) NS_CALL(tcp_sock_refill_ns, c); diff --git a/udp.c b/udp.c index f2be0080..f5b86568 100644 --- a/udp.c +++ b/udp.c @@ -765,6 +765,11 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, if (c->no_udp || !(events & EPOLLIN)) return; + if (ref.udp.pif == PIF_SPLICE) + dstport += c->udp.fwd_out.f.delta[dstport]; + else if (ref.udp.pif == PIF_HOST) + dstport += c->udp.fwd_in.f.delta[dstport]; + if (v6) { mmh_recv = udp6_l2_mh_sock; udp6_localname.sin6_port = htons(dstport); @@ -992,16 +997,13 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, const void *addr, const char *ifname, in_port_t port) { union udp_epoll_ref uref = { .splice = (c->mode == MODE_PASTA), - .orig = true }; + .orig = true, .port = port }; int s, r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; - if (ns) { + if (ns) uref.pif = PIF_SPLICE; - uref.port = (in_port_t)(port + c->udp.fwd_out.f.delta[port]); - } else { + else uref.pif = PIF_HOST; - uref.port = (in_port_t)(port + c->udp.fwd_in.f.delta[port]); - } if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4) { uref.v6 = 0; -- 2.43.0

Currently tcp_splice_flow_defer() contains specific logic to determine if we're far enough initialised that we need to close pipes and/or sockets. This is potentially fragile if we change something about the order in which we do things. We can simplify this by initialising the pipe and socket fields to -1 very early, then close()ing them if and only if they're non negative. This lets us remove a special case cleanup if our connect() fails. This will already trigger a CLOSING event, and the socket fd in question is populated in the connection structure. Thus we can let the new cleanup logic handle it rather than requiring an explicit close(). Signed-off-by: David Gibson --- tcp_splice.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 40ecb5d4..f0343eb5 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -246,16 +246,14 @@ bool tcp_splice_flow_defer(union flow *flow) return false; for (side = 0; side < SIDES; side++) { - if (conn->events & SPLICE_ESTABLISHED) { - /* Flushing might need to block: don't recycle them. */ - if (conn->pipe[side][0] != -1) { - close(conn->pipe[side][0]); - close(conn->pipe[side][1]); - conn->pipe[side][0] = conn->pipe[side][1] = -1; - } + /* Flushing might need to block: don't recycle them. */ + if (conn->pipe[side][0] >= 0) { + close(conn->pipe[side][0]); + close(conn->pipe[side][1]); + conn->pipe[side][0] = conn->pipe[side][1] = -1; } - if (side == 0 || conn->events & SPLICE_CONNECT) { + if (conn->s[side] >= 0) { close(conn->s[side]); conn->s[side] = -1; } @@ -284,8 +282,6 @@ static int tcp_splice_connect_finish(const struct ctx *c, int i = 0; for (side = 0; side < SIDES; side++) { - conn->pipe[side][0] = conn->pipe[side][1] = -1; - for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) { if (splice_pipe_pool[i][0] >= 0) { SWAP(conn->pipe[side][0], @@ -361,12 +357,8 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn, } if (connect(conn->s[1], sa, sl)) { - if (errno != EINPROGRESS) { - int ret = -errno; - - close(sock_conn); - return ret; - } + if (errno != EINPROGRESS) + return -errno; conn_event(c, conn, SPLICE_CONNECT); } else { conn_event(c, conn, SPLICE_ESTABLISHED); @@ -459,6 +451,9 @@ bool tcp_splice_conn_from_sock(const struct ctx *c, conn->f.type = FLOW_TCP_SPLICE; conn->flags = inany_v4(&aany) ? 0 : SPLICE_V6; conn->s[0] = s; + conn->s[1] = -1; + conn->pipe[0][0] = conn->pipe[0][1] = -1; + conn->pipe[1][0] = conn->pipe[1][1] = -1; if (tcp_splice_new(c, conn, ref.port, ref.pif)) conn_flag(c, conn, CLOSING); -- 2.43.0

Our allocation scheme for flow entries means there are some non-obvious constraints on when what things can be done with an entry. Add a big doc comment explaining the life cycle. In addition, make a FLOW_START() macro to mark one of the important transitions. This encourages correct usage, by making it natural to only access the flow type specific structure after calling it. It also logs that a new flow has been created, which is useful for debugging. We also add logging when a flow's lifecycle ends. This doesn't need a new helper, because it can only happen either from flow_alloc_cancel() or from the flow deferred handler. Signed-off-by: David Gibson --- flow.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-- flow.h | 5 ++++ tcp.c | 15 +++++------ tcp_splice.c | 11 ++++---- tcp_splice.h | 5 ++-- 5 files changed, 94 insertions(+), 18 deletions(-) diff --git a/flow.c b/flow.c index beb9749c..a155b54b 100644 --- a/flow.c +++ b/flow.c @@ -34,6 +34,45 @@ static_assert(ARRAY_SIZE(flow_proto) == FLOW_NUM_TYPES, /* Global Flow Table */ +/** + * DOC: Theory of Operation - flow entry life cycle + * + * An individual flow table entry moves through these logical states, usually in + * this order. + * + * FREE - Part of the general pool of free flow table entries + * Operations: + * - flow_alloc() finds an entry and moves it to ALLOC state + * + * ALLOC - A tentatively allocated entry + * Operations: + * - flow_alloc_cancel() returns the entry to FREE state + * - FLOW_START() set the entry's type and moves to START state + * Caveats: + * - It's not safe to write fields in the flow entry + * - It's not safe to allocate other entries with flow_alloc() + * - It's not safe to return to the main epoll loop + * - It's not safe to use flow_*() logging functions + * + * START - An entry being prepared by flow type specific code + * Operations: + * - Flow type specific fields may be accessed + * - flow_*() logging functions + * - flow_alloc_cancel() returns the entry to FREE state + * Caveats: + * - Returning to the main epoll loop or allocating another entry + * with flow_alloc() implicitly moves the entry to ACTIVE state. + * + * ACTIVE - An active flow entry managed by flow type specific code + * Operations: + * - Flow type specific fields may be accessed + * - flow_*() logging functions + * - Flow may be expired by returning 'true' from flow type specific + * deferred or timer handler. This will return it to FREE state. + * Caveats: + * - It's not safe to call flow_alloc_cancel() + */ + /** * DOC: Theory of Operation - allocating and freeing flow entries * @@ -109,6 +148,39 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...) logmsg(pri, "Flow %u (%s): %s", flow_idx(f), FLOW_TYPE(f), msg); } +/** + * flow_start() - Set flow type for new flow and log + * @flow: Flow to set type for + * @type: Type for new flow + * @iniside: Which side initiated the new flow + * + * Return: @flow + * + * Should be called before setting any flow type specific fields in the flow + * table entry. + */ +union flow *flow_start(union flow *flow, enum flow_type type, + unsigned iniside) +{ + (void)iniside; + flow->f.type = type; + flow_dbg(flow, "START %s", flow_type_str[flow->f.type]); + return flow; +} + +/** + * flow_end() - Clear flow type for finished flow and log + * @flow: Flow to clear + */ +static void flow_end(union flow *flow) +{ + if (flow->f.type == FLOW_TYPE_NONE) + return; /* Nothing to do */ + + flow_dbg(flow, "END %s", flow_type_str[flow->f.type]); + flow->f.type = FLOW_TYPE_NONE; +} + /** * flow_alloc() - Allocate a new flow * @@ -157,7 +229,7 @@ void flow_alloc_cancel(union flow *flow) { ASSERT(flow_first_free > FLOW_IDX(flow)); - flow->f.type = FLOW_TYPE_NONE; + flow_end(flow); /* Put it back in a length 1 free cluster, don't attempt to fully * reverse flow_alloc()s steps. This will get folded together the next * time flow_defer_handler runs anyway() */ @@ -227,7 +299,7 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now) } if (closed) { - flow->f.type = FLOW_TYPE_NONE; + flow_end(flow); if (free_head) { /* Add slot to current free cluster */ diff --git a/flow.h b/flow.h index e9b3ce3e..8b66751b 100644 --- a/flow.h +++ b/flow.h @@ -45,6 +45,11 @@ struct flow_common { #define FLOW_TABLE_PRESSURE 30 /* % of FLOW_MAX */ #define FLOW_FILE_PRESSURE 30 /* % of c->nofile */ +union flow *flow_start(union flow *flow, enum flow_type type, + unsigned iniside); +#define FLOW_START(flow_, t_, var_, i_) \ + (&flow_start((flow_), (t_), (i_))->var_) + /** * struct flow_sidx - ID for one side of a specific flow * @side: Side referenced (0 or 1) diff --git a/tcp.c b/tcp.c index 3722dc09..e15b932f 100644 --- a/tcp.c +++ b/tcp.c @@ -1952,8 +1952,7 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, goto cancel; } - conn = &flow->tcp; - conn->f.type = FLOW_TCP; + conn = FLOW_START(flow, FLOW_TCP, tcp, TAPSIDE); conn->sock = s; conn->timer = -1; conn_event(c, conn, TAP_SYN_RCVD); @@ -2658,18 +2657,19 @@ static void tcp_snat_inbound(const struct ctx *c, union inany_addr *addr) * tcp_tap_conn_from_sock() - Initialize state for non-spliced connection * @c: Execution context * @ref: epoll reference of listening socket - * @conn: connection structure to initialize + * @flow: flow to initialise * @s: Accepted socket * @sa: Peer socket address (from accept()) * @now: Current timestamp */ static void tcp_tap_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, - struct tcp_tap_conn *conn, int s, + union flow *flow, int s, const union sockaddr_inany *sa, const struct timespec *now) { - conn->f.type = FLOW_TCP; + struct tcp_tap_conn *conn = FLOW_START(flow, FLOW_TCP, tcp, SOCKSIDE); + conn->sock = s; conn->timer = -1; conn->ws_to_tap = conn->ws_from_tap = 0; @@ -2715,11 +2715,10 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, goto cancel; if (c->mode == MODE_PASTA && - tcp_splice_conn_from_sock(c, ref.tcp_listen, &flow->tcp_splice, - s, &sa)) + tcp_splice_conn_from_sock(c, ref.tcp_listen, flow, s, &sa)) return; - tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s, &sa, now); + tcp_tap_conn_from_sock(c, ref.tcp_listen, flow, s, &sa, now); return; cancel: diff --git a/tcp_splice.c b/tcp_splice.c index 180a9ea7..576fe9be 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -424,7 +424,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, * tcp_splice_conn_from_sock() - Attempt to init state for a spliced connection * @c: Execution context * @ref: epoll reference of listening socket - * @conn: connection structure to initialize + * @flow: flow to initialise * @s: Accepted socket * @sa: Peer address of connection * @@ -432,10 +432,10 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, * #syscalls:pasta setsockopt */ bool tcp_splice_conn_from_sock(const struct ctx *c, - union tcp_listen_epoll_ref ref, - struct tcp_splice_conn *conn, int s, - const union sockaddr_inany *sa) + union tcp_listen_epoll_ref ref, union flow *flow, + int s, const union sockaddr_inany *sa) { + struct tcp_splice_conn *conn; union inany_addr aany; in_port_t port; @@ -445,7 +445,8 @@ bool tcp_splice_conn_from_sock(const struct ctx *c, if (!inany_is_loopback(&aany)) return false; - conn->f.type = FLOW_TCP_SPLICE; + conn = FLOW_START(flow, FLOW_TCP_SPLICE, tcp_splice, 0); + conn->flags = inany_v4(&aany) ? 0 : SPLICE_V6; conn->s[0] = s; conn->s[1] = -1; diff --git a/tcp_splice.h b/tcp_splice.h index 20f41b39..5a471af0 100644 --- a/tcp_splice.h +++ b/tcp_splice.h @@ -12,9 +12,8 @@ union sockaddr_inany; void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events); bool tcp_splice_conn_from_sock(const struct ctx *c, - union tcp_listen_epoll_ref ref, - struct tcp_splice_conn *conn, int s, - const union sockaddr_inany *sa); + union tcp_listen_epoll_ref ref, union flow *flow, + int s, const union sockaddr_inany *sa); void tcp_splice_init(struct ctx *c); #endif /* TCP_SPLICE_H */ -- 2.43.0

In tcp_splice_conn_from_sock(), the 'port' variable stores the source port of the connection on the originating side. In tcp_splice_new(), called directly from it, the 'port' parameter gives the _destination_ port of the originating connection and is then updated to the destination port of the connection on the other side. Similarly, in tcp_splice_conn_from_sock(), 's' is the fd of the accetped socket (on side 0), whereas in tcp_splice_new(), 's' is the fd of the connecting socket (side 1). I, for one, find having the same variable name with different meanings in such close proximity in the flow of control pretty confusing. Alter the names for greater specificity and clarity. Signed-off-by: David Gibson --- tcp_splice.c | 40 ++++++++++++++++++++-------------------- tcp_splice.h | 2 +- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 609f3242..d1263f21 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -378,29 +378,29 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn, * Return: return code from connect() */ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, - in_port_t port, uint8_t pif) + in_port_t dstport, uint8_t pif) { sa_family_t af = CONN_V6(conn) ? AF_INET6 : AF_INET; - int s = -1; + int s1; if (pif == PIF_SPLICE) { - port += c->tcp.fwd_out.delta[port]; + dstport += c->tcp.fwd_out.delta[dstport]; - s = tcp_conn_sock(c, af); + s1 = tcp_conn_sock(c, af); } else { ASSERT(pif == PIF_HOST); - port += c->tcp.fwd_in.delta[port]; + dstport += c->tcp.fwd_in.delta[dstport]; - s = tcp_conn_sock_ns(c, af); + s1 = tcp_conn_sock_ns(c, af); } - if (s < 0) { - warn("Couldn't open connectable socket for splice (%d)", s); - return s; + if (s1 < 0) { + warn("Couldn't open connectable socket for splice (%d)", s1); + return s1; } - return tcp_splice_connect(c, conn, s, port); + return tcp_splice_connect(c, conn, s1, dstport); } /** @@ -408,7 +408,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, * @c: Execution context * @ref: epoll reference of listening socket * @flow: flow to initialise - * @s: Accepted socket + * @s0: Accepted (side 0) socket * @sa: Peer address of connection * * Return: true if able to create a spliced connection, false otherwise @@ -416,28 +416,28 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, */ bool tcp_splice_conn_from_sock(const struct ctx *c, union tcp_listen_epoll_ref ref, union flow *flow, - int s, const union sockaddr_inany *sa) + int s0, const union sockaddr_inany *sa) { struct tcp_splice_conn *conn; - union inany_addr aany; - in_port_t port; + union inany_addr src; + in_port_t srcport; ASSERT(c->mode == MODE_PASTA); - inany_from_sockaddr(&aany, &port, sa); - if (!inany_is_loopback(&aany)) + inany_from_sockaddr(&src, &srcport, sa); + if (!inany_is_loopback(&src)) return false; conn = FLOW_START(flow, FLOW_TCP_SPLICE, tcp_splice, 0); - conn->flags = inany_v4(&aany) ? 0 : SPLICE_V6; - conn->s[0] = s; + conn->flags = inany_v4(&src) ? 0 : SPLICE_V6; + conn->s[0] = s0; conn->s[1] = -1; conn->pipe[0][0] = conn->pipe[0][1] = -1; conn->pipe[1][0] = conn->pipe[1][1] = -1; - if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), sizeof(int))) - flow_trace(conn, "failed to set TCP_QUICKACK on %i", s); + if (setsockopt(s0, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), sizeof(int))) + flow_trace(conn, "failed to set TCP_QUICKACK on %i", s0); if (tcp_splice_new(c, conn, ref.port, ref.pif)) conn_flag(c, conn, CLOSING); diff --git a/tcp_splice.h b/tcp_splice.h index 5a471af0..d0c6ff79 100644 --- a/tcp_splice.h +++ b/tcp_splice.h @@ -13,7 +13,7 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events); bool tcp_splice_conn_from_sock(const struct ctx *c, union tcp_listen_epoll_ref ref, union flow *flow, - int s, const union sockaddr_inany *sa); + int s0, const union sockaddr_inany *sa); void tcp_splice_init(struct ctx *c); #endif /* TCP_SPLICE_H */ -- 2.43.0

Currently creating the connected socket for a splice is split between tcp_splice_conn_from_sock(), which opens the socket, and tcp_splice_connect() which connects it. Alter tcp_splice_connect() to open its own socket based on an address family and pif we pass it. This does require a second conditional on pif, but makes for a more logical split of functionality: tcp_splice_conn_from_sock() picks the target, tcp_splice_connect() creates the connection. While we're there improve reporting of errors Signed-off-by: David Gibson --- tcp_splice.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index de5fd95c..5ba9c8ea 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -319,13 +319,14 @@ static int tcp_splice_connect_finish(const struct ctx *c, * tcp_splice_connect() - Create and connect socket for new spliced connection * @c: Execution context * @conn: Connection pointer - * @s: Accepted socket + * @af: Address family + * @pif: pif on which to create socket * @port: Destination port, host order * * Return: 0 for connect() succeeded or in progress, negative value on error */ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn, - int sock_conn, in_port_t port) + sa_family_t af, uint8_t pif, in_port_t port) { struct sockaddr_in6 addr6 = { .sin6_family = AF_INET6, @@ -340,7 +341,18 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn, const struct sockaddr *sa; socklen_t sl; - conn->s[1] = sock_conn; + if (pif == PIF_HOST) + conn->s[1] = tcp_conn_sock(c, af); + else if (pif == PIF_SPLICE) + conn->s[1] = tcp_conn_sock_ns(c, af); + else + ASSERT(0); + + if (conn->s[1] < 0) { + warn("Couldn't open connectable socket for splice (%d)", + conn->s[1]); + return conn->s[1]; + } if (setsockopt(conn->s[1], SOL_TCP, TCP_QUICKACK, &((int){ 1 }), sizeof(int))) { @@ -387,7 +399,7 @@ bool tcp_splice_conn_from_sock(const struct ctx *c, struct tcp_splice_conn *conn; union inany_addr src; sa_family_t af; - int s1; + uint8_t pif1; ASSERT(c->mode == MODE_PASTA); @@ -409,24 +421,16 @@ bool tcp_splice_conn_from_sock(const struct ctx *c, flow_trace(conn, "failed to set TCP_QUICKACK on %i", s0); if (ref.pif == PIF_SPLICE) { + pif1 = PIF_HOST; dstport += c->tcp.fwd_out.delta[dstport]; - - s1 = tcp_conn_sock(c, af); } else { ASSERT(ref.pif == PIF_HOST); + pif1 = PIF_SPLICE; dstport += c->tcp.fwd_in.delta[dstport]; - - s1 = tcp_conn_sock_ns(c, af); - } - - if (s1 < 0) { - warn("Couldn't open connectable socket for splice (%d)", s1); - conn_flag(c, conn, CLOSING); - return true; } - if (tcp_splice_connect(c, conn, s1, dstport)) + if (tcp_splice_connect(c, conn, af, pif1, dstport)) conn_flag(c, conn, CLOSING); return true; -- 2.43.0

This makes several tweaks to improve the logic which decides whether we're able to use the splice method for a new connection. * Rather than only calling tcp_splice_conn_from_sock() in pasta mode, we check for pasta mode within it, better localising the checks. * Previously if we got a connection from a non-loopback address we'd always fall back to the "tap" path, even if the connection was on a socket in the namespace. If we did get a non-loopback address on a namespace socket, something has gone wrong and the "tap" path certainly won't be able to handle it. Report the error and close, rather than passing it along to tap. Signed-off-by: David Gibson --- inany.c | 1 - tcp.c | 3 +-- tcp_splice.c | 48 ++++++++++++++++++++++++++++++++++-------------- 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/inany.c b/inany.c index c11e2aa9..1c165b14 100644 --- a/inany.c +++ b/inany.c @@ -39,7 +39,6 @@ const union inany_addr inany_any4 = { * * Return: On success, a non-null pointer to @dst, NULL on failure */ -/* cppcheck-suppress unusedFunction */ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size) { const struct in_addr *v4 = inany_v4(src); diff --git a/tcp.c b/tcp.c index c06d1cc4..d61fb17b 100644 --- a/tcp.c +++ b/tcp.c @@ -2733,8 +2733,7 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, if (s < 0) goto cancel; - if (c->mode == MODE_PASTA && - tcp_splice_conn_from_sock(c, ref.tcp_listen, flow, s, &sa)) + if (tcp_splice_conn_from_sock(c, ref.tcp_listen, flow, s, &sa)) return; tcp_tap_conn_from_sock(c, ref.tcp_listen, flow, s, &sa, now); diff --git a/tcp_splice.c b/tcp_splice.c index 49075e5c..1937850f 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -405,14 +405,44 @@ bool tcp_splice_conn_from_sock(const struct ctx *c, sa_family_t af; uint8_t pif1; - ASSERT(c->mode == MODE_PASTA); - - inany_from_sockaddr(&src, &srcport, sa); - if (!inany_is_loopback(&src)) + if (c->mode != MODE_PASTA) return false; + inany_from_sockaddr(&src, &srcport, sa); af = inany_v4(&src) ? AF_INET : AF_INET6; + switch (ref.pif) { + case PIF_SPLICE: + if (!inany_is_loopback(&src)) { + char str[INANY_ADDRSTRLEN]; + + /* We can't use flow_err() etc. because we haven't set + * the flow type yet + */ + warn("Bad source address %s for splice, closing", + inany_ntop(&src, str, sizeof(str))); + + /* We *don't* want to fall back to tap */ + flow_alloc_cancel(flow); + return true; + } + + pif1 = PIF_HOST; + dstport += c->tcp.fwd_out.delta[dstport]; + break; + + case PIF_HOST: + if (!inany_is_loopback(&src)) + return false; + + pif1 = PIF_SPLICE; + dstport += c->tcp.fwd_in.delta[dstport]; + break; + + default: + return false; + } + conn = FLOW_START(flow, FLOW_TCP_SPLICE, tcp_splice, 0); conn->flags = af == AF_INET ? 0 : SPLICE_V6; @@ -424,16 +454,6 @@ bool tcp_splice_conn_from_sock(const struct ctx *c, if (setsockopt(s0, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), sizeof(int))) flow_trace(conn, "failed to set TCP_QUICKACK on %i", s0); - if (ref.pif == PIF_SPLICE) { - pif1 = PIF_HOST; - dstport += c->tcp.fwd_out.delta[dstport]; - } else { - ASSERT(ref.pif == PIF_HOST); - - pif1 = PIF_SPLICE; - dstport += c->tcp.fwd_in.delta[dstport]; - } - if (tcp_splice_connect(c, conn, af, pif1, dstport)) conn_flag(c, conn, CLOSING); -- 2.43.0

TCP connections should typically not have wildcard addresses (0.0.0.0 or ::) nor a zero port number for either endpoint. It's not entirely clear (at least to me) if it's strictly against the RFCs to do so, but at any rate the socket interfaces often treat those values specially[1], so it's not really possible to manipulate such connections. Likewise they should not have broadcast or multicast addresses for either endpoint. However, nothing prevents a guest from creating a SYN packet with such values, and it's not entirely clear what the effect on passt would be. To ensure sane behaviour, explicitly check for this case and drop such packets, logging a debug warning (we don't want a higher level, because that would allow a guest to spam the logs). We never expect such an address on an accept()ed socket either, but just in case, check for it as well. [1] Depending on context as "unknown", "match any" or "kernel, pick something for me" Signed-off-by: David Gibson --- tcp.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 7 deletions(-) diff --git a/tcp.c b/tcp.c index 31d4e87b..236a8d23 100644 --- a/tcp.c +++ b/tcp.c @@ -284,6 +284,7 @@ #include #include #include +#include #include /* For struct tcp_info */ @@ -1930,27 +1931,59 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, const struct tcphdr *th, const char *opts, size_t optlen, const struct timespec *now) { + in_port_t srcport = ntohs(th->source); + in_port_t dstport = ntohs(th->dest); struct sockaddr_in addr4 = { .sin_family = AF_INET, - .sin_port = th->dest, + .sin_port = htons(dstport), .sin_addr = *(struct in_addr *)daddr, }; struct sockaddr_in6 addr6 = { .sin6_family = AF_INET6, - .sin6_port = th->dest, + .sin6_port = htons(dstport), .sin6_addr = *(struct in6_addr *)daddr, }; const struct sockaddr *sa; struct tcp_tap_conn *conn; union flow *flow; + int s = -1, mss; socklen_t sl; - int s, mss; - - (void)saddr; if (!(flow = flow_alloc())) return; + if (af == AF_INET) { + if (IN4_IS_ADDR_UNSPECIFIED(saddr) || + IN4_IS_ADDR_BROADCAST(saddr) || + IN4_IS_ADDR_MULTICAST(saddr) || srcport == 0 || + IN4_IS_ADDR_UNSPECIFIED(daddr) || + IN4_IS_ADDR_BROADCAST(daddr) || + IN4_IS_ADDR_MULTICAST(daddr) || dstport == 0) { + char sstr[INET_ADDRSTRLEN], dstr[INET_ADDRSTRLEN]; + + debug("Invalid endpoint in TCP SYN: %s:%hu -> %s:%hu", + inet_ntop(AF_INET, saddr, sstr, sizeof(sstr)), + srcport, + inet_ntop(AF_INET, daddr, dstr, sizeof(dstr)), + dstport); + goto cancel; + } + } else if (af == AF_INET6) { + if (IN6_IS_ADDR_UNSPECIFIED(saddr) || + IN6_IS_ADDR_MULTICAST(saddr) || srcport == 0 || + IN6_IS_ADDR_UNSPECIFIED(daddr) || + IN6_IS_ADDR_MULTICAST(daddr) || dstport == 0) { + char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN]; + + debug("Invalid endpoint in TCP SYN: %s:%hu -> %s:%hu", + inet_ntop(AF_INET6, saddr, sstr, sizeof(sstr)), + srcport, + inet_ntop(AF_INET6, daddr, dstr, sizeof(dstr)), + dstport); + goto cancel; + } + } + if ((s = tcp_conn_sock(c, af)) < 0) goto cancel; @@ -2001,8 +2034,8 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, sl = sizeof(addr6); } - conn->fport = ntohs(th->dest); - conn->eport = ntohs(th->source); + conn->fport = dstport; + conn->eport = srcport; conn->seq_init_from_tap = ntohl(th->seq); conn->seq_from_tap = conn->seq_init_from_tap + 1; @@ -2732,6 +2765,33 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, if (s < 0) goto cancel; + if (sa.sa_family == AF_INET) { + const struct in_addr *addr = &sa.sa4.sin_addr; + in_port_t port = sa.sa4.sin_port; + + if (IN4_IS_ADDR_UNSPECIFIED(addr) || + IN4_IS_ADDR_BROADCAST(addr) || + IN4_IS_ADDR_MULTICAST(addr) || port == 0) { + char str[INET_ADDRSTRLEN]; + + err("Invalid endpoint from TCP accept(): %s:%hu", + inet_ntop(AF_INET, addr, str, sizeof(str)), port); + goto cancel; + } + } else if (sa.sa_family == AF_INET6) { + const struct in6_addr *addr = &sa.sa6.sin6_addr; + in_port_t port = sa.sa6.sin6_port; + + if (IN6_IS_ADDR_UNSPECIFIED(addr) || + IN6_IS_ADDR_MULTICAST(addr) || port == 0) { + char str[INET6_ADDRSTRLEN]; + + err("Invalid endpoint from TCP accept(): %s:%hu", + inet_ntop(AF_INET6, addr, str, sizeof(str)), port); + goto cancel; + } + } + if (tcp_splice_conn_from_sock(c, ref.tcp_listen.pif, ref.tcp_listen.port, flow, s, &sa)) return; -- 2.43.0

port_fwd_scan_udp() handles UDP, as the name suggests, but its function comment has the wrong function name and references TCP, due to a bad copy/pasta from port_fwd_scan_tcp(). Signed-off-by: David Gibson --- port_fwd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/port_fwd.c b/port_fwd.c index 6f6c836c..a7121fe2 100644 --- a/port_fwd.c +++ b/port_fwd.c @@ -85,7 +85,7 @@ void port_fwd_scan_tcp(struct port_fwd *fwd, const struct port_fwd *rev) } /** - * port_fwd_scan_tcp() - Scan /proc to update TCP forwarding map + * port_fwd_scan_udp() - Scan /proc to update UDP forwarding map * @fwd: Forwarding information to update * @rev: Forwarding information for the reverse direction * @tcp_fwd: Corresponding TCP forwarding information -- 2.43.0