On Mon, Nov 07, 2022 at 07:07:53PM +0100, Stefano Brivio wrote:On Fri, 4 Nov 2022 19:43:24 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:"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.--- 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,valgrind should also hit thisI'm not sure. Depends whether valgrind's knowledge of syscall behaviours has the same bug or not.-- perhaps move to initialiser though.Fixed.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.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.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.-- 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+ 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))