From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202508 header.b=ci37xzAu; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 8A0885A027A for ; Thu, 11 Sep 2025 04:40:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1757558404; bh=5LwpUfx6oiTGp5Qat+02LB/whDZfVkRY27Td4zOYU+U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ci37xzAuPWaLPNTXg7DnMiX5h6qmdpx1IpODQmzt189J102muN8cGukeB8ynlondB iRrjPjAe9GZDd2kJHOJ6pDgYAdKP8uLvGlmOWVlKLlTfvL0pfS9UmZEBuSQM3KaMb8 RYg3Cgp9TIEGOBaMCbDsVscfmo2SozH4ug8UIzLnCJKQhHlVGy/ph7ouZ87GoSaod9 6cyyjoumzBdWHs2RoxOAD42P5Tgq8phC3JBOvMF3V4Oo0DRqx/EwKLaL2vzq/Zi6E9 MirmestWVIoSQ762gt/HD7pg0W4C8DTwG/IdR2WR19XvkE/yj+1qLo67gXnQ2WPr50 vn/DDqocHXnLQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cMhc41sZgz4wB0; Thu, 11 Sep 2025 12:40:04 +1000 (AEST) Date: Thu, 11 Sep 2025 12:37:18 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v4 7/8] tcp: Fast re-transmit if half-closed, make TAP_FIN_RCVD path consistent Message-ID: References: <20250909181655.2990223-1-sbrivio@redhat.com> <20250909181655.2990223-8-sbrivio@redhat.com> <20250910115726.432bbb8d@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="rKoSqN7ar8Qh1l9a" Content-Disposition: inline In-Reply-To: <20250910115726.432bbb8d@elisabeth> Message-ID-Hash: CJVKEINH2SNNATPC4YD6QLWVV32PMQ5Z X-Message-ID-Hash: CJVKEINH2SNNATPC4YD6QLWVV32PMQ5Z 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, Jon Maloy , Paul Holzinger 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: --rKoSqN7ar8Qh1l9a Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 10, 2025 at 11:57:26AM +0200, Stefano Brivio wrote: > On Wed, 10 Sep 2025 12:27:12 +1000 > David Gibson wrote: >=20 > > On Tue, Sep 09, 2025 at 08:16:54PM +0200, Stefano Brivio wrote: > > > We currently have a number of discrepancies in the tcp_tap_handler() > > > path between the half-closed connection path and the regular one, and > > > they are mostly a result of code duplication, which comes in turn from > > > the fact that tcp_data_from_tap() deals with data transfers as well as > > > general connection bookkeeping, so we can't use it for half-closed > > > connections. > > >=20 > > > This suggests that we should probably rework it into two or more > > > functions, in the long term, but for the moment being I'm just fixing > > > one obvious issue, which is the lack of fast retransmissions in the > > > TAP_FIN_RCVD path, and a potential one, which is the fact we don't > > > handle socket flush failures. > > >=20 > > > Add fast re-transmit for half-closed connections, and handle the case > > > of socket flush (tcp_sock_consume()) flush failure in the same way as > > > tcp_data_from_tap() handles it. > > >=20 > > > Signed-off-by: Stefano Brivio =20 > >=20 > > Reviewed-by: David Gibson > >=20 > > > --- > > > tcp.c | 42 +++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 39 insertions(+), 3 deletions(-) > > >=20 > > > diff --git a/tcp.c b/tcp.c > > > index 9c70a25..5163dbf 100644 > > > --- a/tcp.c > > > +++ b/tcp.c > > > @@ -1652,6 +1652,23 @@ static int tcp_data_from_sock(const struct ctx= *c, struct tcp_tap_conn *conn) > > > return tcp_buf_data_from_sock(c, conn); > > > } > > > =20 > > > +/** > > > + * tcp_packet_data_len() - Get data (TCP payload) length for a TCP p= acket > > > + * @th: Pointer to TCP header > > > + * @l4len: TCP packet length, including TCP header > > > + * > > > + * Return: data length of TCP packet, -1 on invalid value of Data Of= fset field > > > + */ > > > +static ssize_t tcp_packet_data_len(const struct tcphdr *th, size_t l= 4len) > > > +{ > > > + size_t off =3D th->doff * 4UL; > > > + > > > + if (off < sizeof(*th) || off > l4len) > > > + return -1; > > > + > > > + return l4len - off; > > > +} > > > + > > > /** > > > * tcp_data_from_tap() - tap/guest data for established connection > > > * @c: Execution context > > > @@ -2113,9 +2130,28 @@ int tcp_tap_handler(const struct ctx *c, uint8= _t pif, sa_family_t af, > > > =20 > > > /* Established connections not accepting data from tap */ > > > if (conn->events & TAP_FIN_RCVD) { > > > - tcp_sock_consume(conn, ntohl(th->ack_seq)); > > > - tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq)); > > > - if (tcp_tap_window_update(c, conn, ntohs(th->window))) > > > + bool retr; > > > + > > > + retr =3D th->ack && !tcp_packet_data_len(th, l4len) && !th->fin &&= =20 > >=20 > > Not really in scope here, but I wonder if we should log an error > > and/or RST if we get a non-zero data length in this situation. >=20 > According to RFC 9293 we should ignore data (note: not data segments) > in this case, see 3.10.7.4 "Other states": >=20 > [...] >=20 > Seventh, process the segment text: >=20 > [...] >=20 > CLOSE-WAIT STATE >=20 > This should not occur since a FIN has been received from the remote sid= e. Ignore the segment text. >=20 > https://www.rfc-editor.org/rfc/rfc9293.html#section-3.10.7.4-2.7.2.7.1 >=20 > We could add a debug() message perhaps (in a further patch), but I don't > think we are allowed to reset the connection. Ok, makes sense. --=20 David Gibson (he or they) | 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 --rKoSqN7ar8Qh1l9a Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjCNd0ACgkQzQJF27ox 2Gcl2g/+LrEyfNZEplcsZ/6SQISjtHfeBsRrONXx8029r75ypN8YEYtVTfBj8FJ/ 02VChKxjN2KcnFTiiz5grhHWWWWOdxRjEl38JNVwo++ksztoEylMF83eRN2PVi9b KZSSAbpKbDSTL/xu89916CIw+sXIoTaULteIHugZba9wxLNUfldqJPa8AvArugFH JWMcT3fL4jAARS3rEETHPCMpbnAJmFNp14XTS3+BmUu1bBdVLjxM/rqW79i60H33 OMSvvTTf6pVSrvlIynlLtKa7DH4p/lzP0v3iU4nkGk32y4Y4er+duDCW/GjsSc7n JQRQdDBU/ywwzksGg5qnTOli2PzHtR3K0mlAgZoRpsZti4KJUDKUeEIz/L3YhRjb zwRXU+rvdB3VysG8CzOT/u3I4lBY1+yvaca401lLNnpXJMNPmSN/75Vr2IaEsW6x Q4neXSFWrLJ3ryI/GsSwmNodbYeXq4Wp6zy50S8+BvpDQ3//5lSnktF/prQhre6/ N80EGr4HikqFnRyzG8RoOpxj9LVJSzfijo8dsl/KOBD3sDKXvx2922KLpbeehgy4 wdeAl1UT2PXr08CCrNcCIyW8MeT3bbKDeer8PbiecjNMY2yRmmVee6LrR20G9oQw Oi9lnExnKJNu2Z6x+Jk1rHNWuiNJt2v48zh2nSzmaufxJRGVwaQ= =Q02Y -----END PGP SIGNATURE----- --rKoSqN7ar8Qh1l9a--