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=202510 header.b=tUEM+UR1; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 4BE7A5A061A for ; Wed, 08 Oct 2025 02:42:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1759884176; bh=Rj6eu+EET4QZTSWjdusWuEFjqIA0bIQnNbo3EelH96c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tUEM+UR1OT7RGsfq5SYk+ftgazzzp5urS+FmvvrqHtf92HpMoOdDvv9bdmQeGpSjV BsMW4lFyMyWSM05q0cOCK8r1MlxxmZzKuhEVwpVcA4Cp72iA6XgWL5Az6W+X+5olNt MqXGts8HbB+tjSKXe6R0saeUS8qgg6+cKGmXNcovJ8v7VSIHrb53u8SY+yk0mBkJPK ggeERoCFqxmXcZcNYYAkYk8hkwoF5ACasSqLBvEHCXNQdJ7MkhQCEUu52pv/ZZqQ4N L71FWbBbM4IFIHnOgtmk/S4bbNk1eArLR6alvSD0VQOoD3PCx+VymZcYGCxKeJUzZo 1dkA0iRBo+Eeg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4chDkS0MdLz4wB8; Wed, 8 Oct 2025 11:42:56 +1100 (AEDT) Date: Wed, 8 Oct 2025 11:41:23 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/4] tcp: Don't consider FIN flags with mismatching sequence Message-ID: References: <20251002000646.2136202-1-sbrivio@redhat.com> <20251002000646.2136202-4-sbrivio@redhat.com> <20251002135104.1a662029@elisabeth> <20251007003254.57ce5e26@elisabeth> <20251008004249.6dc894fa@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="JwLWjmYSZS07aBnc" Content-Disposition: inline In-Reply-To: <20251008004249.6dc894fa@elisabeth> Message-ID-Hash: O2BH5FEHOBID27OK66GF2NT6S3WKDKA2 X-Message-ID-Hash: O2BH5FEHOBID27OK66GF2NT6S3WKDKA2 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: --JwLWjmYSZS07aBnc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 08, 2025 at 12:42:49AM +0200, Stefano Brivio wrote: > On Tue, 7 Oct 2025 10:34:01 +1100 > David Gibson wrote: > > On Tue, Oct 07, 2025 at 12:32:54AM +0200, Stefano Brivio wrote: > > > On Fri, 3 Oct 2025 13:43:32 +1000 > > > David Gibson wrote: [snip] > > > > @@ -1820,6 +1817,8 @@ static int tcp_data_from_tap(const struct ctx= *c, struct tcp_tap_conn *conn, > > > > break; > > > > seq_from_tap +=3D size; > > > > iov_i +=3D count; > > > > + if (th->fin) > > > > + fin =3D 1; > > > > =20 > > > > if (keep =3D=3D i) > > > > keep =3D -1; > > > >=20 > > > >=20 > > > > We'd need to double check that the "accept data segment" path is sa= fe > > > > with len =3D=3D 0, of course. =20 > > >=20 > > > For sure it's not before d2c33f45f7be ("tcp: Convert > > > tcp_data_from_tap() to use iov_tail"), because we might add > > > zero-length segments to the tcp_iov array, and that would make > > > backporting an otherwise simple and critical fix to slightly older > > > versions rather complicated. =20 > >=20 > > Kinda. It's not that complicated to deal with that case, by wrapping > > the actual data processing in an `if (len) { ... }` >=20 > That's needed for sure, but do we risk looping forever on a particular > batch of FIN segments without data with a given series of sequence > numbers? > > Right now the loop terminates because the sequence moves forward. I'm > not sure what happens without data while moving 'keep' around. Maybe it > takes a few minutes to figure out (I haven't tried) but I wouldn't call > that trivial. That should be fine, because we need to advance the sequence by one for the FIN anyway, so we will move forwards. I might have forgotten that in my quick example, which is a bug, but an easy one to fix. > > > After that commit, I'm not sure about the behaviour of > > > iov_tail_clone(). I think it will return 0, but it should be tested > > > (that assumption, itself, but also that the fix still works). =20 > >=20 > > Right, I think it's safe in that case - that's what I was looking at. >=20 > Note that I didn't test this. Understood. > > > > But I think that will treat dataless and > > > > with-data FINs the same way, and let them use the keep mechanism. = =20 > > >=20 > > > Given that the only advantage of doing this would be to handle a rare > > > (I guess) corner case, that is, an out-of-sequence FIN segment with > > > data, which is not critical anyway because the FIN will be > > > retransmitted, I would rather keep this (critical) fix as it is. > > >=20 > > > I would suggest filing a separate ticket or anyway sending a separate > > > patch if you want to fix that other case. =20 > >=20 > > Fair enough. I'll wait for you to get the basic fix merge, then I > > might batch this cleanup/fixup with the reorg to clarify bytes_acked > > handling. >=20 > Okay, thanks, merged now. >=20 > --=20 > Stefano >=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 --JwLWjmYSZS07aBnc Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjlszIACgkQzQJF27ox 2GfI3A//RzEtxqw+Sh+gqWYVlMO6o7yGRe3H9JE+qxUEaKYz1EflFzCy6fHNmRLx G781qFtuhzndKUrJttxETbN3jdQWtiQj7/mAUPAHKemdUpFsZXKoQJDv85sLpjdY AMsytE1rAOp92pUSjL9ytABcJNM4Bg1LpnjbHmwN4Pyrxm4T6qKn5HPR6IPzKjeN lxozn4AXKWV1iEMH4a3/zLUARgVDf6YiolMfxc51RmKx0KLqMB2TyA2PxjVwuCg3 qZQ/UMyd+wxzkTKRVhHj9VyrZC9R8yFzOmKPpfv/kmpdVjGukoMciirfsG+ZdKCH nddUK/fhekZA0oqYwq6hPz8wwJwaEQAqGkaYc7fpO3GHiui+xajyYhsZ/xb0j3k1 GoLKs7CUgbOAp78e/BI2zIHcU8TXOSY8Rw90OFbb4gKEa0tXdXFOcR2NlM6ZCY1I GQ/lvTTQ+9H83RIUT1kLJGYWyH6jfkaJUe5eMUo1uEaiVkguAyj87G+Lwi3VMYbB /emaivXWNX+ROqI4wgmyf1atzKnScxeazm6JnRtLGHJtabX0xnrNk9AAfryQycBJ Yv1A/AK/Zv2VifLAyytTsINORDaeZrR6KEfGG4kLXdktyRQvqBnt9uPfpSloy/OB yqwigkqBtycN7Xt58/CvSwULUkGl21nYofMLAXmrTmOe81ual3c= =Qy8g -----END PGP SIGNATURE----- --JwLWjmYSZS07aBnc--