From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id DBB165A026F for ; Thu, 17 Nov 2022 03:08:56 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4NCNcx2TFyz4xZj; Thu, 17 Nov 2022 13:08:49 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1668650929; bh=9fj3ijqriTnFyqdHAXT305rfI4WuQqpd2ekiiVQy26c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=l8flFgvstpC05+YnHjqaUukd9nvUlgnoIyQw6z0GRueSslyJG3/PToAyJtPRsO7gt FAIe7ZBwxjUUvOxwGpkyV0tcLe5kCrpTky87JD7aCAAy1oSQLifTz+ffLpeC8ahpJR ueuTDKAjpt0/Mh5UFpIYAJwsBhOMerFL29dsewY4= Date: Thu, 17 Nov 2022 12:43:58 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 16/32] tcp: Use the same sockets to listen for spliced and non-spliced connections Message-ID: References: <20221116044212.3876516-1-david@gibson.dropbear.id.au> <20221116044212.3876516-17-david@gibson.dropbear.id.au> <20221117005405.70227aba@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JKEl21p8k1Ls74SQ" Content-Disposition: inline In-Reply-To: <20221117005405.70227aba@elisabeth> Message-ID-Hash: BO5TF2NDUICMI7QWCADNF4ZAI6K7OFY4 X-Message-ID-Hash: BO5TF2NDUICMI7QWCADNF4ZAI6K7OFY4 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --JKEl21p8k1Ls74SQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 17, 2022 at 12:54:05AM +0100, Stefano Brivio wrote: > On Wed, 16 Nov 2022 15:41:56 +1100 > David Gibson wrote: >=20 > > In pasta mode, tcp_sock_init[46]() create separate sockets to listen for > > spliced connections (these are bound to localhost) and non-spliced > > connections (these are bound to the host address). This introduces a > > subtle behavioural difference between pasta and passt: by default, pasta > > will listen only on a single host address, whereas passt will listen on > > all addresses (0.0.0.0 or ::). This also prevents us using some additi= onal > > optimizations that only work with the unspecified (0.0.0.0 or ::) addre= ss. > >=20 > > However, it turns out we don't need to do this. We can splice a connec= tion > > if and only if it originates from the loopback address. Currently we > > ensure this by having the "spliced" listening sockets listening only on > > loopback. Instead, defer the decision about whether to splice a connec= tion > > until after accept(), by checking if the connection was made from the > > loopback address. > >=20 > > Signed-off-by: David Gibson > > --- > > tcp.c | 127 +++++++++++++-------------------------------------- > > tcp_splice.c | 25 ++++++++-- > > tcp_splice.h | 5 +- > > 3 files changed, 55 insertions(+), 102 deletions(-) > >=20 > > diff --git a/tcp.c b/tcp.c > > index e66a82a..4065da7 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -434,7 +434,6 @@ static const char *tcp_flag_str[] __attribute((__un= used__)) =3D { > > }; > > =20 > > /* Listening sockets, used for automatic port forwarding in pasta mode= only */ > > -static int tcp_sock_init_lo [NUM_PORTS][IP_VERSIONS]; > > static int tcp_sock_init_ext [NUM_PORTS][IP_VERSIONS]; > > static int tcp_sock_ns [NUM_PORTS][IP_VERSIONS]; > > =20 > > @@ -2851,21 +2850,31 @@ static void tcp_conn_from_sock(struct ctx *c, u= nion epoll_ref ref, > > socklen_t sl; > > int s; > > =20 > > + assert(ref.r.p.tcp.tcp.listen); > > + assert(!ref.r.p.tcp.tcp.splice); > > + > > if (c->tcp.conn_count >=3D TCP_MAX_CONNS) > > return; > > =20 > > sl =3D sizeof(sa); > > + /* FIXME: Workaround clang-tidy not realizing that accept4() > > + * writes the socket address. See > > + * https://github.com/llvm/llvm-project/issues/58992 > > + */ > > + memset(&sa, 0, sizeof(struct sockaddr_in6)); > > s =3D accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); >=20 > Ah, interesting. That looks new by the way -- not even valgrind > complained about this. Right, valgrind seems to have better modeling of the syscall here. Note that this is harder for a static tool to get right, because the amount that accept() writes is variable. I got a reply on that bug report, saying apparently clang-tidy *should* consider sa written here, and they weren't easily able to reproduce the problem. So, I'm not sure what's going on here :(. > > if (s < 0) > > return; > > =20 > > conn =3D tc + c->tcp.conn_count++; > > =20 > > - if (ref.r.p.tcp.tcp.splice) > > - tcp_splice_conn_from_sock(c, ref, &conn->splice, s); > > - else > > - tcp_tap_conn_from_sock(c, ref, &conn->tap, s, > > - (struct sockaddr *)&sa, now); > > + if (c->mode =3D=3D MODE_PASTA && > > + tcp_splice_conn_from_sock(c, ref, &conn->splice, > > + s, (struct sockaddr *)&sa)) > > + return; > > + > > + tcp_tap_conn_from_sock(c, ref, &conn->tap, s, > > + (struct sockaddr *)&sa, now); > > } > > =20 > > /** > > @@ -3018,47 +3027,16 @@ static void tcp_sock_init4(const struct ctx *c,= const struct in_addr *addr, > > { > > in_port_t idx =3D port + c->tcp.fwd_in.delta[port]; > > union tcp_epoll_ref tref =3D { .tcp.listen =3D 1, .tcp.index =3D idx = }; > > - bool spliced =3D false, tap =3D true; > > int s; > > =20 > > - if (c->mode =3D=3D MODE_PASTA) { > > - spliced =3D !addr || IN4_IS_ADDR_UNSPECIFIED(addr) || > > - IN4_IS_ADDR_LOOPBACK(addr); > > - > > - if (!addr) > > - addr =3D &c->ip4.addr; > > - > > - tap =3D !IN4_IS_ADDR_LOOPBACK(addr); > > - } > > - > > - if (tap) { > > - s =3D sock_l4(c, AF_INET, IPPROTO_TCP, addr, ifname, port, > > - tref.u32); > > - if (s >=3D 0) > > - tcp_sock_set_bufsize(c, s); > > - else > > - s =3D -1; > > - > > - if (c->tcp.fwd_in.mode =3D=3D FWD_AUTO) > > - tcp_sock_init_ext[port][V4] =3D s; > > - } > > - > > - if (spliced) { > > - struct in_addr loopback =3D { htonl(INADDR_LOOPBACK) }; > > - tref.tcp.splice =3D 1; > > - > > - addr =3D &loopback; > > - > > - s =3D sock_l4(c, AF_INET, IPPROTO_TCP, addr, ifname, port, > > - tref.u32); > > - if (s >=3D 0) > > - tcp_sock_set_bufsize(c, s); > > - else > > - s =3D -1; > > + s =3D sock_l4(c, AF_INET, IPPROTO_TCP, addr, ifname, port, tref.u32); > > + if (s >=3D 0) > > + tcp_sock_set_bufsize(c, s); > > + else > > + s =3D -1; > > =20 > > - if (c->tcp.fwd_out.mode =3D=3D FWD_AUTO) > > - tcp_sock_init_lo[port][V4] =3D s; > > - } > > + if (c->tcp.fwd_in.mode =3D=3D FWD_AUTO) > > + tcp_sock_init_ext[port][V4] =3D s; > > } > > =20 > > /** > > @@ -3075,47 +3053,16 @@ static void tcp_sock_init6(const struct ctx *c, > > in_port_t idx =3D port + c->tcp.fwd_in.delta[port]; > > union tcp_epoll_ref tref =3D { .tcp.listen =3D 1, .tcp.v6 =3D 1, > > .tcp.index =3D idx }; > > - bool spliced =3D false, tap =3D true; > > int s; > > =20 > > - if (c->mode =3D=3D MODE_PASTA) { > > - spliced =3D !addr || > > - IN6_IS_ADDR_UNSPECIFIED(addr) || > > - IN6_IS_ADDR_LOOPBACK(addr); > > - > > - if (!addr) > > - addr =3D &c->ip6.addr; > > - > > - tap =3D !IN6_IS_ADDR_LOOPBACK(addr); > > - } > > - > > - if (tap) { > > - s =3D sock_l4(c, AF_INET6, IPPROTO_TCP, addr, ifname, port, > > - tref.u32); > > - if (s >=3D 0) > > - tcp_sock_set_bufsize(c, s); > > - else > > - s =3D -1; > > - > > - if (c->tcp.fwd_in.mode =3D=3D FWD_AUTO) > > - tcp_sock_init_ext[port][V6] =3D s; > > - } > > - > > - if (spliced) { > > - tref.tcp.splice =3D 1; > > - > > - addr =3D &in6addr_loopback; > > - > > - s =3D sock_l4(c, AF_INET6, IPPROTO_TCP, addr, ifname, port, > > - tref.u32); > > - if (s >=3D 0) > > - tcp_sock_set_bufsize(c, s); > > - else > > - s =3D -1; > > + s =3D sock_l4(c, AF_INET6, IPPROTO_TCP, addr, ifname, port, tref.u32); > > + if (s >=3D 0) > > + tcp_sock_set_bufsize(c, s); > > + else > > + s =3D -1; > > =20 > > - if (c->tcp.fwd_out.mode =3D=3D FWD_AUTO) > > - tcp_sock_init_lo[port][V6] =3D s; > > - } > > + if (c->tcp.fwd_in.mode =3D=3D FWD_AUTO) > > + tcp_sock_init_ext[port][V6] =3D s; > > } > > =20 > > /** > > @@ -3144,7 +3091,7 @@ static void tcp_ns_sock_init4(const struct ctx *c= , in_port_t port) > > { > > in_port_t idx =3D port + c->tcp.fwd_out.delta[port]; > > union tcp_epoll_ref tref =3D { .tcp.listen =3D 1, .tcp.outbound =3D 1, > > - .tcp.splice =3D 1, .tcp.index =3D idx }; > > + .tcp.index =3D idx }; > > struct in_addr loopback =3D { htonl(INADDR_LOOPBACK) }; > > int s; > > =20 > > @@ -3169,8 +3116,7 @@ static void tcp_ns_sock_init6(const struct ctx *c= , in_port_t port) > > { > > in_port_t idx =3D port + c->tcp.fwd_out.delta[port]; > > union tcp_epoll_ref tref =3D { .tcp.listen =3D 1, .tcp.outbound =3D 1, > > - .tcp.splice =3D 1, .tcp.v6 =3D 1, > > - .tcp.index =3D idx}; > > + .tcp.v6 =3D 1, .tcp.index =3D idx}; >=20 > Space missing here (from 14/32). Fixed. > > int s; > > =20 > > assert(c->mode =3D=3D MODE_PASTA); > > @@ -3337,7 +3283,6 @@ int tcp_init(struct ctx *c) > > memset(init_sock_pool6, 0xff, sizeof(init_sock_pool6)); > > memset(ns_sock_pool4, 0xff, sizeof(ns_sock_pool4)); > > memset(ns_sock_pool6, 0xff, sizeof(ns_sock_pool6)); > > - memset(tcp_sock_init_lo, 0xff, sizeof(tcp_sock_init_lo)); > > memset(tcp_sock_init_ext, 0xff, sizeof(tcp_sock_init_ext)); > > memset(tcp_sock_ns, 0xff, sizeof(tcp_sock_ns)); > > =20 > > @@ -3445,16 +3390,6 @@ static int tcp_port_rebind(void *arg) > > close(tcp_sock_init_ext[port][V6]); > > tcp_sock_init_ext[port][V6] =3D -1; > > } > > - > > - if (tcp_sock_init_lo[port][V4] >=3D 0) { > > - close(tcp_sock_init_lo[port][V4]); > > - tcp_sock_init_lo[port][V4] =3D -1; > > - } > > - > > - if (tcp_sock_init_lo[port][V6] >=3D 0) { > > - close(tcp_sock_init_lo[port][V6]); > > - tcp_sock_init_lo[port][V6] =3D -1; > > - } > > continue; > > } > > =20 > > diff --git a/tcp_splice.c b/tcp_splice.c > > index 7007501..30d49d4 100644 > > --- a/tcp_splice.c > > +++ b/tcp_splice.c > > @@ -502,19 +502,35 @@ static void tcp_splice_dir(struct tcp_splice_conn= *conn, int ref_sock, > > } > > =20 > > /** > > - * tcp_splice_conn_from_sock() - Initialize state for spliced connecti= on > > + * tcp_splice_conn_from_sock() - Attempt to init state for a spliced c= onnection > > * @c: Execution context > > * @ref: epoll reference of listening socket > > * @conn: connection structure to initialize > > * @s: Accepted socket > > + * @sa: Peer address of connection > > * > > + * Return: true if able to create a spliced connection, false otherwise > > * #syscalls:pasta setsockopt > > */ > > -void tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, > > - struct tcp_splice_conn *conn, int s) > > +bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, > > + struct tcp_splice_conn *conn, int s, > > + const struct sockaddr *sa) > > { > > assert(c->mode =3D=3D MODE_PASTA); > > =20 > > + if (ref.r.p.tcp.tcp.v6) { > > + const struct sockaddr_in6 *sa6 > > + =3D (const struct sockaddr_in6 *)sa; >=20 > Maybe you could split declaration and assignment here. Good idea, done. > > + if (!IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr)) > > + return false; > > + conn->flags =3D SPLICE_V6; > > + } else { > > + const struct sockaddr_in *sa4 =3D (const struct sockaddr_in *)sa; > > + if (!IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr)) > > + return false; > > + conn->flags =3D 0; > > + } > > + > > if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), > > sizeof(int))) { > > trace("TCP (spliced): failed to set TCP_QUICKACK on %i", > > @@ -524,11 +540,12 @@ void tcp_splice_conn_from_sock(struct ctx *c, uni= on epoll_ref ref, > > conn->c.spliced =3D true; > > c->tcp.splice_conn_count++; > > conn->a =3D s; > > - conn->flags =3D ref.r.p.tcp.tcp.v6 ? SPLICE_V6 : 0; > > =20 > > if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index, > > ref.r.p.tcp.tcp.outbound)) > > conn_flag(c, conn, CLOSING); > > + > > + return true; > > } > > =20 > > /** > > diff --git a/tcp_splice.h b/tcp_splice.h > > index f9462ae..1a915dd 100644 > > --- a/tcp_splice.h > > +++ b/tcp_splice.h > > @@ -10,8 +10,9 @@ struct tcp_splice_conn; > > =20 > > void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref, > > uint32_t events); > > -void tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, > > - struct tcp_splice_conn *conn, int s); > > +bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, > > + struct tcp_splice_conn *conn, int s, > > + const struct sockaddr *sa); > > void tcp_splice_init(struct ctx *c); > > =20 > > #endif /* TCP_SPLICE_H */ >=20 --=20 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 --JKEl21p8k1Ls74SQ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmN1kdgACgkQgypY4gEw YSLNgg/9Gs6Gq9ehERN5wx2Oqbag0ljdCilN+S5UIhKkpcAG5FLGz3xCQ6W/FkSh DFmoJxOblaucIdGItzRvmWmT44bhm6jGN2u1MeoOG9evkkrUE+Dkq8wGBNRaLf+8 KWYy6JwLIWcEIb0KmRgPp6rXXg7bhy6FBId15KRQG3EuHVtNVZyqOS46jE+PIRii 2+0MZwGANvXa5Xq/6P2YoTDQ2BMgRzuoOm5NLiMTXkdTaZDrZ1gg2eBJv4sQbIUX cTIYnQB2pNnLxFgnCnINiTHPfN3Lrgbib7BW4xHqlsjxS8d6CAPouULBsYdfqq5g 9uyn+y+TxnW6Owr16ntijmdVz3XIkqZHwP4wSuzkKxeVNWOJe1bwtdwngD67ou2N LDiTUwG2kvqZRRFrbr4AiMVz7OSWwNDYoRUWS+2d1G8n94eSIpU1K+oHuZQFNrVm VlyPqAuAT3ucrKt8jKl7PIzRhfHI1Te23C1mfqwPNYL2jqY+RyYPHQrvhKPMW3qG 2JfIus3wzxTU4xnG32PBUmWraemzrDFgUggXjB6UrPesXuT0FMtwbnQ7NZ4SXxNx w3A9bbgP5KbnowDNkXtELUhVI0nPkxQMU0Ga1TLFMh7WIGbzKM9Qu+qKAm5hfr9I xwBXIaoApXKnGx5hS7qQluFUTEbDIFsgr6ZyRMuLVj9KoUdFyBI= =wInw -----END PGP SIGNATURE----- --JKEl21p8k1Ls74SQ--