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=cWCEOohT; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id E79D65A027A for ; Wed, 10 Sep 2025 04:29:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1757471394; bh=LqG5noYoZhfPu0syERYewY/SUC2I+JdCCvJr8mfToiM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=cWCEOohT4fz3Mw0WA21oUma/gt/bxmGRZvdWY5H6tVLFoqzkW9ORW4DGnUh2JMuL0 HYtK/hHqf2d8gIkgJdRkXGRpvrzNI5MTnbczuuugLijVFnLY2nFYVBroeN+bloIJRl HPVW3m78YMKbMlWJ6XO1Ifv9KSHH91zgyiCZeNUPJgTAJrHNwHnzBApnetd20noyH+ Hqw9XcdchfRn8LMM3BvyzVL4p2KhMQgBB2wyK3Ouv7Obu36d52SdHAPtdXFl0NPSf+ yDJ86aOzslIWN89yAUInHb/hVhCIYajGjC6yT00kSh5L3MhmGoElOROO4CpYXVB8jT fG8z38A49UGCg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cM4Qp4wHdz4wC1; Wed, 10 Sep 2025 12:29:54 +1000 (AEST) Date: Wed, 10 Sep 2025 12:27:12 +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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="K2/W9mcBKIdUW5Xc" Content-Disposition: inline In-Reply-To: <20250909181655.2990223-8-sbrivio@redhat.com> Message-ID-Hash: ZWB2QFSWCICQJW5IU4GH2OOKBPO2AAJP X-Message-ID-Hash: ZWB2QFSWCICQJW5IU4GH2OOKBPO2AAJP 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: --K2/W9mcBKIdUW5Xc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 Reviewed-by: David Gibson > --- > 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 packet > + * @th: Pointer to TCP header > + * @l4len: TCP packet length, including TCP header > + * > + * Return: data length of TCP packet, -1 on invalid value of Data Offset= field > + */ > +static ssize_t tcp_packet_data_len(const struct tcphdr *th, size_t l4len) > +{ > + 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 p= if, 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 && 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. > + ntohl(th->ack_seq) =3D=3D conn->seq_ack_from_tap && > + ntohs(th->window) =3D=3D conn->wnd_from_tap; > + > + /* On socket flush failure, pretend there was no ACK, try again > + * later > + */ > + if (th->ack && !tcp_sock_consume(conn, ntohl(th->ack_seq))) > + tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq)); > + > + if (retr) { > + flow_trace(conn, > + "fast re-transmit, ACK: %u, previous sequence: %u", > + ntohl(th->ack_seq), conn->seq_to_tap); > + > + if (tcp_rewind_seq(c, conn)) > + return -1; > + } > + > + if (tcp_tap_window_update(c, conn, ntohs(th->window)) || retr) > tcp_data_from_sock(c, conn); > =20 > if (conn->seq_ack_from_tap =3D=3D conn->seq_to_tap) { > --=20 > 2.43.0 >=20 --=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 --K2/W9mcBKIdUW5Xc Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjA4f8ACgkQzQJF27ox 2GeVFg/+PZ1YkStB8t3rvsiLe7ZUWxlTxoMW7LksjuvuKdHcmQBjbfzaONe8nans WzqRr2Rz6bDN2itvnvI8Kk8+SjaBu/JO+LzLFLEeMgJhVUYZYrbBkO3JhWinImYu kti6+j73v+thf30QwkG/mytbDyLwk6t2SLfcGAIcwTQZUr08VB2hvqbNTIHKs1+A Gywr4ixBtCoABLP547Aax23CiImQwvcgh5KloW+KyEtYpo0auDbyu2TSbFe1aXu6 sx9iUne+B+0v0Wc9wo309qPvdTxBmyW2fLwW/dN73DxbU5L9qFQME+M35mGVjED6 Ca+w8Jy6c0tfCECDYyFkEv4etT3NI322CwSly/SSH3SL/DWA+CG5NxVwzB1KHWbS GPVgxhm5V2Xk2bd+COfE0+D+j5Oa5bsieWJ3ayma3hkWRpX8iUl1rw4Kvvw9xcwe 1S9yW4VofYATziyL6TKuPCKWvBYkax/hdVr6F5QvbfS77fc3M4vpe6nmY5NZrO4V kcjNXO8LjDrIeR9xX701VXH7GZlyOtUplU2vIQ25vKuozuZGhR3ECm4hHPiOokg/ zzMaIhcjPZLUO/cJKQP0kLeNR4tOs8idV1eG2YsSD3kbYlKzIXwJpAX6MmF63Sps /0NOUrc85urY6/IA26xNvNltml/AlPvljFpyjakMe/CcmwlBcCU= =dxWP -----END PGP SIGNATURE----- --K2/W9mcBKIdUW5Xc--