From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id A3C495A026D for ; Mon, 6 Nov 2023 03:39:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1699238366; bh=G0n8yKEDs+7Pprt55PJPQ/C06qt9PD2ZYfLni23sXNI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=c5zxq1lwNymSwrzeORaJhhBtzcUn8gLe19AMDetrmoMfPaCbLemCTqBDEntn82peZ EGh30ij7ns55xsXs3v2jsVHs513eZ7UEcJSC3Nek6A5qpTHlDYsDPo2QQlDFWxNqvp 4QoJXRkD8ldQI6jlQ8WdJBdKBBmoP0aIH6OqQ/Q4= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4SNwXt6Mqgz4xPY; Mon, 6 Nov 2023 13:39:26 +1100 (AEDT) Date: Mon, 6 Nov 2023 13:39:14 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 10/11] tcp_splice: Exploit side symmetry in tcp_splice_destroy() Message-ID: References: <20231012015114.2612066-1-david@gibson.dropbear.id.au> <20231012015114.2612066-11-david@gibson.dropbear.id.au> <20231103172213.35610a3f@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WJ+fQvAupzuqJmT1" Content-Disposition: inline In-Reply-To: <20231103172213.35610a3f@elisabeth> Message-ID-Hash: GZ5Z2AU7RMGCETZAFXBKEELGDI27RIUG X-Message-ID-Hash: GZ5Z2AU7RMGCETZAFXBKEELGDI27RIUG 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.8 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: --WJ+fQvAupzuqJmT1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 03, 2023 at 05:22:13PM +0100, Stefano Brivio wrote: > On Thu, 12 Oct 2023 12:51:13 +1100 > David Gibson wrote: >=20 > > tcp_splice_destroy() has some close-to-duplicated logic handling closin= g of > > the socket and ipies for each side of the connection. We can use a loop > ^^^^^ pipes Oops, fixed. > > across the sides to reduce the duplication. > >=20 > > Signed-off-by: David Gibson > > --- > > tcp_splice.c | 32 ++++++++++++++------------------ > > 1 file changed, 14 insertions(+), 18 deletions(-) > >=20 > > diff --git a/tcp_splice.c b/tcp_splice.c > > index 99ef8a4..239f6d2 100644 > > --- a/tcp_splice.c > > +++ b/tcp_splice.c > > @@ -258,30 +258,26 @@ void tcp_splice_conn_update(const struct ctx *c, = struct tcp_splice_conn *new) > > void tcp_splice_destroy(struct ctx *c, union tcp_conn *conn_union) > > { > > struct tcp_splice_conn *conn =3D &conn_union->splice; > > + int side; > > =20 > > - if (conn->events & SPLICE_ESTABLISHED) { > > - /* Flushing might need to block: don't recycle them. */ > > - if (conn->pipe[0][0] !=3D -1) { > > - close(conn->pipe[0][0]); > > - close(conn->pipe[0][1]); > > - conn->pipe[0][0] =3D conn->pipe[0][1] =3D -1; > > + for (side =3D 0; side < SIDES; side++) { > > + if (conn->events & SPLICE_ESTABLISHED) { > > + /* Flushing might need to block: don't recycle them. */ > > + if (conn->pipe[side][0] !=3D -1) { > > + close(conn->pipe[side][0]); > > + close(conn->pipe[side][1]); > > + conn->pipe[side][0] =3D conn->pipe[side][1] =3D -1; > > + } > > } > > - if (conn->pipe[1][0] !=3D -1) { > > - close(conn->pipe[1][0]); > > - close(conn->pipe[1][1]); > > - conn->pipe[1][0] =3D conn->pipe[1][1] =3D -1; > > + > > + if (side =3D=3D 0 || conn->events & SPLICE_CONNECT) { > > + close(conn->s[side]); > > + conn->s[side] =3D -1; > > } > > - } > > =20 > > - if (conn->events & SPLICE_CONNECT) { > > - close(conn->s[1]); > > - conn->s[1] =3D -1; > > + conn->read[side] =3D conn->written[side] =3D 0; > > } > > =20 > > - close(conn->s[0]); > > - conn->s[0] =3D -1; > > - conn->read[0] =3D conn->written[0] =3D conn->read[1] =3D conn->writte= n[1] =3D 0; >=20 > With this, on SPLICE_CONNECT, we would close the [0] side, but not the > [1] side. SPLICE_CONNECT means we already have an open socket for [1], > though. I think it should be: >=20 > [loop on sides] >=20 > if (side =3D=3D 1 || conn->events & SPLICE_CONNECT) { > close(conn->s[side]); > conn->s[1] =3D -1; > } > } >=20 > and then we still need to unconditionally close conn->s[0]. Perhaps we co= uld > take both parts outside of the loop: Uh.. I think you're misreading. In the updated code we have: if (side =3D=3D 0 || conn->events & SPLICE_CONNECT) { close(conn->s[side]); conn->s[side] =3D -1; } That's an OR, so we always close side 0, and we close side 1 iff we have SPLICE_CONNECT, which matches what you're describing. > if (conn->events & SPLICE_CONNECT) { > close(conn->s[1]); > conn->s[1] =3D -1; > } >=20 > close(conn->s[0]); > conn->s[0] =3D -1; > conn->read[side] =3D conn->written[side] =3D 0; >=20 > The handling for the SPLICE_ESTABLISHED case looks correct to me. We are relying on the fact that setting SPLICE_ESTABLISHED doesn't clear SPLICE_CONNECT. --=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 --WJ+fQvAupzuqJmT1 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmVIUcoACgkQzQJF27ox 2GfYYxAAjClTJPCtHD902WSINP3XYeV4/KIQm7nYN3M97vImuYH9yvKj2stkkQgf uFgyOQtOCdSoonyv9rqQQ1YYwp+ug6cB27rRiJmpwWqKDgcolwXhlfyEYO719Gwu HKQQ2KXbYnyMF91gF9tzLN2b6Ox4US0SY2f8C9Cl8YkVlsNU5Nx/vxZupsLDmyGl Aj6LFSfCG+45uRwcAkAI8PtlHludqM2WjSW+yMPy89WR+aZTitzU36UWU51pK24/ udu+Il1MiQwHkQ5joDWLNSd1EWJgtBXYF+pTIynuh6jID7uJo0hm/WLr0U9g4HFn klnD8wqcK5Spahw3FXAkp51xQm+nEkCQOcIH7AnAjC58vCw6Vk27Ui9KDH67otAb yXkegB9ZGvgJrfvcudok3Sb+T6A3PFsiDp9N07IzSU0YmYIq3NMBWlIwJK/FKaHC RByRAaTUHww1uEfgO8+6uCuGFmSwDo/WNqiMc6EAwgNLRe8i7W4Nf12SuHKA8oe5 jKGoPErEjS+gtDdBr5GHb/Y2nejE90M667XqljAlWhiHGyt3wH6eK8TmdnyTBjyd hlODNJmt/QuisivRNwRj/ZOm7Ja6c/wX4PxllKG6r1O5EYsUYUq8bDd6fGNrm8FH 6rNn/S0EQSOCoIzQ+6IuQMIbng/DAViK0O/OZd4P+50Ph1RHPLU= =nnrV -----END PGP SIGNATURE----- --WJ+fQvAupzuqJmT1--