On Mon, Nov 07, 2022 at 07:07:53PM +0100, Stefano Brivio wrote:

On Fri, 4 Nov 2022 19:43:24 +1100 David Gibson wrote:

...

--- tcp.c | 9 ++++----- tcp.h | 1 - tcp_splice.c | 13 +++++++------ 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/tcp.c b/tcp.c index 713248f..3d48d6e 100644 --- a/tcp.c +++ b/tcp.c @@ -761,8 +761,7 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_conn *conn) { int m = (conn->flags & IN_EPOLL) ? EPOLL_CTL_MOD : EPOLL_CTL_ADD; union epoll_ref ref = { .r.proto = IPPROTO_TCP, .r.s = conn->sock, - .r.p.tcp.tcp.index = conn - tc, - .r.p.tcp.tcp.v6 = CONN_V6(conn) }; + .r.p.tcp.tcp.index = conn - tc, }; struct epoll_event ev = { .data.u64 = ref.u64 };

if (conn->events == CLOSED) { @@ -2857,6 +2856,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, if (c->tcp.conn_count >= TCP_MAX_CONNS) return;

+ sa.ss_family = AF_UNSPEC; /* FIXME: stop clang-tidy complaining */

No need for /* FIXME */ here in my opinion,

"FIXME" maybe isn't quite the right sentiment. The issue here is I don't think this should be necessary. AFAIK sa.ss_family will be written on any succesful return from accept4(), it's just the clang-tidy doesn't seem to know that.

valgrind should also hit this

I'm not sure. Depends whether valgrind's knowledge of syscall behaviours has the same bug or not.

-- perhaps move to initialiser though.

...

sl = sizeof(sa); s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); if (s < 0) @@ -2868,7 +2868,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, conn->ws_to_tap = conn->ws_from_tap = 0; conn_event(c, conn, SOCK_ACCEPTED);

- if (ref.r.p.tcp.tcp.v6) { + if (sa.ss_family == AF_INET6) { struct sockaddr_in6 sa6;

memcpy(&sa6, &sa, sizeof(sa6)); @@ -3147,8 +3147,7 @@ static void tcp_sock_init6(const struct ctx *c, int ns, const struct in6_addr *addr, const char *ifname, in_port_t port) { - union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.outbound = ns, - .tcp.v6 = 1 }; + union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.outbound = ns, };

Undocumented style rule: no comma at the end, it's post-modern C anyway.

Fixed.

...

bool spliced = false, tap = true; int s;

diff --git a/tcp.h b/tcp.h index 3fabb5a..85ac750 100644 --- a/tcp.h +++ b/tcp.h @@ -45,7 +45,6 @@ union tcp_epoll_ref { uint32_t listen:1, splice:1, outbound:1, - v6:1,

Don't forget to update the comment above.

Fixed.

...

timer:1, index:20; } tcp; diff --git a/tcp_splice.c b/tcp_splice.c index 99c5fa7..1f26ff4 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -211,12 +211,10 @@ static int tcp_splice_epoll_ctl(const struct ctx *c, int m = (conn->flags & IN_EPOLL) ? EPOLL_CTL_MOD : EPOLL_CTL_ADD; union epoll_ref ref_a = { .r.proto = IPPROTO_TCP, .r.s = conn->a, .r.p.tcp.tcp.splice = 1, - .r.p.tcp.tcp.index = conn - tc, - .r.p.tcp.tcp.v6 = CONN_V6(conn) }; + .r.p.tcp.tcp.index = conn - tc, }; union epoll_ref ref_b = { .r.proto = IPPROTO_TCP, .r.s = conn->b, .r.p.tcp.tcp.splice = 1, - .r.p.tcp.tcp.index = conn - tc, - .r.p.tcp.tcp.v6 = CONN_V6(conn) }; + .r.p.tcp.tcp.index = conn - tc, };

Commas.

Fixed.

...

struct epoll_event ev_a = { .data.u64 = ref_a.u64 }; struct epoll_event ev_b = { .data.u64 = ref_b.u64 }; uint32_t events_a, events_b; @@ -579,12 +577,15 @@ void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref, struct tcp_splice_conn *conn;

if (ref.r.p.tcp.tcp.listen) { + struct sockaddr sa;

Should this be sockaddr_storage in this case and then cast? I never remember.

So, for this patch, no it doesn't, but it will have to change later, in fact. The trick here is that we only want the sa_family field, we don't need the full socket address. struct sockaddr has that (and maybe a little extra, but not much). accept4() will truncate the socket address when it writes, but that's ok for our purposes. Using the short form may mitigate the concern below because there's less data to copy_to_user(). Of course, when I change this later to add different handling for IPv4-mapped IPv6 source addresses we will need the full address, so this will need to be a sockaddr_in6.

...

+ socklen_t sl = sizeof(sa); int s;

if (c->tcp.splice_conn_count >= TCP_SPLICE_MAX_CONNS) return;

- if ((s = accept4(ref.r.s, NULL, NULL, SOCK_NONBLOCK)) < 0) + sa.sa_family = AF_UNSPEC; /* FIXME: stop clang-tidy complaining */ + if ((s = accept4(ref.r.s, &sa, &sl, SOCK_NONBLOCK)) < 0)

Same comment about /* FIXME */ as above.

This adds a copy_to_user() and microseconds, but it's unavoidable. I tried harder to optimise for latency on the spliced path, that's why it was NULL here and not above.

...

return;

if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), @@ -595,7 +596,7 @@ void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref,

conn = CONN(c->tcp.splice_conn_count++); conn->a = s; - conn->flags = ref.r.p.tcp.tcp.v6 ? SOCK_V6 : 0; + conn->flags = (sa.sa_family == AF_INET6) ? SOCK_V6 : 0;

if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index, ref.r.p.tcp.tcp.outbound))

-- 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 Fri, 4 Nov 2022 19:43:25 +1100 David Gibson wrote:

In the tcp_conn structure we have space for an IPv6 address, and use IPv4-mapped IPv6 addresses to describe IPv4 connections. We open code the construction of those IPv4-mapped address in two places.

Avoid the duplication with a helper function.

Much nicer, indeed.

Signed-off-by: David Gibson --- tcp.c | 9 ++------- util.c | 20 ++++++++++++++++++++ util.h | 1 + 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/tcp.c b/tcp.c index 3d48d6e..26dd268 100644 --- a/tcp.c +++ b/tcp.c @@ -2217,9 +2217,7 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void *addr, sa = (struct sockaddr *)&addr4; sl = sizeof(addr4);

- memset(&conn->a.a4.zero, 0, sizeof(conn->a.a4.zero)); - memset(&conn->a.a4.one, 0xff, sizeof(conn->a.a4.one)); - memcpy(&conn->a.a4.a, addr, sizeof(conn->a.a4.a)); + encode_ip4mapped_ip6(&conn->a.a6, addr); } else { sa = (struct sockaddr *)&addr6; sl = sizeof(addr6); @@ -2902,15 +2900,12 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,

memcpy(&sa4, &sa, sizeof(sa4));

- memset(&conn->a.a4.zero, 0, sizeof(conn->a.a4.zero)); - memset(&conn->a.a4.one, 0xff, sizeof(conn->a.a4.one)); - if (IN4_IS_ADDR_LOOPBACK(&sa4.sin_addr) || IN4_IS_ADDR_UNSPECIFIED(&sa4.sin_addr) || IN4_ARE_ADDR_EQUAL(&sa4.sin_addr, &c->ip4.addr_seen)) sa4.sin_addr = c->ip4.gw;

- conn->a.a4.a = sa4.sin_addr; + encode_ip4mapped_ip6(&conn->a.a6, &sa4.sin_addr);

conn->sock_port = ntohs(sa4.sin_port); conn->tap_port = ref.r.p.tcp.tcp.index; diff --git a/util.c b/util.c index 514bd44..257d0b6 100644 --- a/util.c +++ b/util.c @@ -482,3 +482,23 @@ int write_file(const char *path, const char *buf) close(fd); return len == 0 ? 0 : -1; } + +struct ip4mapped_ip6 { + uint8_t zero[10]; + uint8_t one[2]; + struct in_addr a4; +};

Document fields even if they're obvious. Can we reuse this part in struct conn instead of using struct in6_addr there as you do later in 7/10? I don't have a strong preference though.

+ +/** + * encode_ip4mapped_ip6() - Convert an IPv4 address to an IPv4-mapped IPv6 address + * @ip6: Buffer to store the IPv4-mapped IPv6 address + * @ip4: IPv4 address, network order + */ +void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4) +{ + struct ip4mapped_ip6 *a = (struct ip4mapped_ip6 *)ip6; + + memset(&a->zero, 0, sizeof(a->zero)); + memset(&a->one, 0xff, sizeof(a->one)); + memcpy(&a->a4, ip4, sizeof(a->a4)); +} diff --git a/util.h b/util.h index 2d4e1ff..f7d6a6f 100644 --- a/util.h +++ b/util.h @@ -209,5 +209,6 @@ void write_pidfile(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4);

#endif /* UTIL_H */

-- Stefano

On Mon, Nov 07, 2022 at 07:08:27PM +0100, Stefano Brivio wrote:

On Fri, 4 Nov 2022 19:43:26 +1100 David Gibson wrote:

...

When given an IPv4 address tcp_hash_match() checks if the connection stores an IPv4-mapped IPv6 address, and if so compares the mapped part of that address to the given address. This is equivalent to converting a given IPv4 address to an IPv4-mapped IPv6 address then comparing it to the connection address using the existing IPv6 logic.

Convert to this slightly simpler form, which will also allow some further simplifications in future.

Signed-off-by: David Gibson --- tcp.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tcp.c b/tcp.c index 26dd268..508d6e9 100644 --- a/tcp.c +++ b/tcp.c @@ -1285,13 +1285,14 @@ static int tcp_opt_get(const char *opts, size_t len, uint8_t type_find, static int tcp_hash_match(const struct tcp_conn *conn, int af, const void *addr, in_port_t tap_port, in_port_t sock_port) { - if (af == AF_INET && CONN_V4(conn) && - !memcmp(&conn->a.a4.a, addr, sizeof(conn->a.a4.a)) && - conn->tap_port == tap_port && conn->sock_port == sock_port) - return 1; + struct in6_addr v4mapped; + + if (af == AF_INET) { + encode_ip4mapped_ip6(&v4mapped, addr); + addr = &v4mapped; + }

It's probably worth the simplification, just mind that this replaces a 32-bit comparison with 128 bits of writes.

I'm aware, however this is mitigated later in the series. Once I make tcp_hash_match() take a (possibly v4mapped) v6 address only it's just a 128-bit comparison, which we already effectively do split between CONN_V4() and the v4 specific path. Obviously that means constructing the v4mapped address in the caller, but we were already doing that in at least one of the cases.

...

- if (af == AF_INET6 && - IN6_ARE_ADDR_EQUAL(&conn->a.a6, addr) && + if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, addr) && conn->tap_port == tap_port && conn->sock_port == sock_port) return 1;

-- 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 Mon, Nov 07, 2022 at 07:08:38PM +0100, Stefano Brivio wrote:

On Fri, 4 Nov 2022 19:43:30 +1100 David Gibson wrote:

...

We now only extract the v4 part of the tcp_conn::a union in one place. Make this neater by using a helper to extract the IPv4 address from the mapped address in that place.

Signed-off-by: David Gibson --- tcp.c | 40 +++++++++++++++------------------------- util.c | 13 +++++++++++++ util.h | 1 + 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/tcp.c b/tcp.c index 3816a1c..6634abb 100644 --- a/tcp.c +++ b/tcp.c @@ -416,10 +416,7 @@ struct tcp6_l2_head { /* For MSS6 macro: keep in sync with tcp6_l2_buf_t */ * @ws_to_tap: Window scaling factor advertised to tap/guest * @sndbuf: Sending buffer in kernel, rounded to 2 ^ SNDBUF_BITS * @seq_dup_ack_approx: Last duplicate ACK number sent to tap - * @a.a6: IPv6 remote address, can be IPv4-mapped - * @a.a4.zero: Zero prefix for IPv4-mapped, see RFC 6890, Table 20 - * @a.a4.one: Ones prefix for IPv4-mapped - * @a.a4.a: IPv4 address + * @addr: IPv6 remote address, can be IPv4-mapped * @tap_port: Guest-facing tap port * @sock_port: Remote, socket-facing port * @wnd_from_tap: Last window size from tap, unscaled (as received) @@ -489,15 +486,8 @@ struct tcp_conn { uint8_t seq_dup_ack_approx;

- union { - struct in6_addr a6; - struct { - uint8_t zero[10]; - uint8_t one[2]; - struct in_addr a; - } a4; - } a; -#define CONN_V4(conn) IN6_IS_ADDR_V4MAPPED(&conn->a.a6) + struct in6_addr addr; +#define CONN_V4(conn) IN6_IS_ADDR_V4MAPPED(&conn->addr) #define CONN_V6(conn) (!CONN_V4(conn))

in_port_t tap_port; @@ -960,7 +950,7 @@ static int tcp_rtt_dst_low(const struct tcp_conn *conn) int i;

for (i = 0; i < LOW_RTT_TABLE_SIZE; i++) - if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, low_rtt_dst + i)) + if (IN6_ARE_ADDR_EQUAL(&conn->addr, low_rtt_dst + i)) return 1;

return 0; @@ -982,7 +972,7 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn, return;

for (i = 0; i < LOW_RTT_TABLE_SIZE; i++) { - if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, low_rtt_dst + i)) + if (IN6_ARE_ADDR_EQUAL(&conn->addr, low_rtt_dst + i)) return; if (hole == -1 && IN6_IS_ADDR_UNSPECIFIED(low_rtt_dst + i)) hole = i; @@ -994,10 +984,10 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn, if (hole == -1) return;

- memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6)); + memcpy(low_rtt_dst + hole++, &conn->addr, sizeof(conn->addr)); if (hole == LOW_RTT_TABLE_SIZE) hole = 0; - memcpy(low_rtt_dst + hole, &in6addr_any, sizeof(conn->a.a6)); + memcpy(low_rtt_dst + hole, &in6addr_any, sizeof(conn->addr)); #else (void)conn; (void)tinfo; @@ -1285,7 +1275,7 @@ static int tcp_hash_match(const struct tcp_conn *conn, const struct in6_addr *addr, in_port_t tap_port, in_port_t sock_port) { - if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, addr) && + if (IN6_ARE_ADDR_EQUAL(&conn->addr, addr) && conn->tap_port == tap_port && conn->sock_port == sock_port) return 1;

@@ -1330,7 +1320,7 @@ static void tcp_hash_insert(const struct ctx *c, struct tcp_conn *conn) { int b;

- b = tcp_hash(c, &conn->a.a6, conn->tap_port, conn->sock_port); + b = tcp_hash(c, &conn->addr, conn->tap_port, conn->sock_port); conn->next_index = tc_hash[b] ? tc_hash[b] - tc : -1; tc_hash[b] = conn; conn->hash_bucket = b; @@ -1660,7 +1650,7 @@ do { \ ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);

b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr)); - b->ip6h.saddr = conn->a.a6; + b->ip6h.saddr = conn->addr; if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr)) b->ip6h.daddr = c->ip6.addr_ll_seen; else @@ -1684,7 +1674,7 @@ do { \

ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr); b->iph.tot_len = htons(ip_len); - b->iph.saddr = conn->a.a4.a.s_addr; + b->iph.saddr = decode_ip4mapped_ip6(&conn->addr).s_addr; b->iph.daddr = c->ip4.addr_seen.s_addr;

if (check) @@ -2202,12 +2192,12 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void *addr, sa = (struct sockaddr *)&addr4; sl = sizeof(addr4);

- encode_ip4mapped_ip6(&conn->a.a6, addr); + encode_ip4mapped_ip6(&conn->addr, addr); } else { sa = (struct sockaddr *)&addr6; sl = sizeof(addr6);

- memcpy(&conn->a.a6, addr, sizeof(conn->a.a6)); + memcpy(&conn->addr, addr, sizeof(conn->addr)); }

conn->sock_port = ntohs(th->dest); @@ -2869,7 +2859,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, memcpy(&sa6.sin6_addr, src, sizeof(*src)); }

- memcpy(&conn->a.a6, &sa6.sin6_addr, sizeof(conn->a.a6)); + memcpy(&conn->addr, &sa6.sin6_addr, sizeof(conn->addr));

conn->sock_port = ntohs(sa6.sin6_port); conn->tap_port = ref.r.p.tcp.tcp.index; @@ -2888,7 +2878,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, IN4_ARE_ADDR_EQUAL(&sa4.sin_addr, &c->ip4.addr_seen)) sa4.sin_addr = c->ip4.gw;

- encode_ip4mapped_ip6(&conn->a.a6, &sa4.sin_addr); + encode_ip4mapped_ip6(&conn->addr, &sa4.sin_addr);

conn->sock_port = ntohs(sa4.sin_port); conn->tap_port = ref.r.p.tcp.tcp.index; diff --git a/util.c b/util.c index 257d0b6..ec7f57f 100644 --- a/util.c +++ b/util.c @@ -502,3 +502,16 @@ void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4) memset(&a->one, 0xff, sizeof(a->one)); memcpy(&a->a4, ip4, sizeof(a->a4)); } + +/** + * decode_ip4mapped_ip6() - Decode an IPv4-mapped IPv6 address + * @ip6: IPv6 address + * + * Returns: IPv4 address

Return: IPv4 address

Fixed. I note there are a few other instances of this mistake in the code, most by fault by the looks of it. I'll try to fix those others at some point.

...

+ */ +struct in_addr decode_ip4mapped_ip6(const struct in6_addr *ip6) +{ + const struct ip4mapped_ip6 *a = (const struct ip4mapped_ip6 *)ip6;

...if you use struct ip4_mapped_ip6 directly in struct conn, you could drop this cast, and perhaps this helper too, but I'm not sure if it has other implications.

True. As I said, I'm considering a slightly different approach now.

...

+ + return a->a4; +} diff --git a/util.h b/util.h index f7d6a6f..103211d 100644 --- a/util.h +++ b/util.h @@ -210,5 +210,6 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4); +struct in_addr decode_ip4mapped_ip6(const struct in6_addr *ip6);

#endif /* UTIL_H */

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

On Fri, 4 Nov 2022 19:43:33 +1100 David Gibson wrote:

It looks like tcp_seq_init() is supposed to advance the sequence number by one every 32ns. However we only right shift the ns part of the timespec not the seconds part, meaning that we'll advance by an extra 32 steps on each second.

I don't know if that's exploitable in any way, but it doesn't appear to be the intent, nor what RFC 6528 suggests.

Oh, oops, nice catch. Well, as long as the step, modulo 32 bits, is not 0, it's still arguably the 250 KHz / 4 µs period clock from RFC 793, so there's no practical difference (other than the overflow period). See also the note in RFC 1948: More precisely, RFC 793 specifies that the 32-bit counter be incremented by 1 in the low-order position about every 4 microseconds. Instead, Berkeley-derived kernels increment it by a constant every second [...] I kind of wonder if adding a time non-linearity like the unintended one I added actually increases entropy. But indeed ~4 seconds overflow is also not intended, and we should just stick to RFC 6528.

Signed-off-by: David Gibson --- tcp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcp.c b/tcp.c index 59e03ff..941fafb 100644 --- a/tcp.c +++ b/tcp.c @@ -2027,8 +2027,8 @@ static void tcp_seq_init(const struct ctx *c, struct tcp_conn *conn,

seq = siphash_36b((uint8_t *)&in, c->tcp.hash_secret);

- ns = now->tv_sec * 1E9; - ns += now->tv_nsec >> 5; /* 32ns ticks, overflows 32 bits every 137s */ + /* 32ns ticks, overflows 32 bits every 137s */ + ns = (now->tv_sec * 1E9 + now->tv_nsec) >> 5;

conn->seq_to_tap = seq + ns; }

-- Stefano