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=vwD3XbXx; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 72DD95A061C for ; Wed, 05 Nov 2025 05:24:54 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1762316692; bh=Fjm+gcz0YSG41IgkEupKtxwTpBCQC0alwLGdgiAJmSc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=vwD3XbXxvLMLq0jNg21MlTiQskh/KpkY0Hxzih3tBg2MU9iMKT3UEiFP7y6wN0Dg+ xhT8EDo2U6LnKvcCst4m7FribR/oGthk7cPmEsOcCIZS812Z+UMnWC+kchxrcpB9Ie CnBBGdm7dnzj21xVT1Oo8t0nx0UTCy3taHeek/s3NW+RN+kFupvyFzwRIhh4Q7Elew mCjNG4FIBQsywzLndj5eH1FDRX6KchtccZf+RYLpDmVlGla3AnDCoataYeUc6m798K ccb9vDJalihSt3/bIvk4miKTY02u+wxtkYyvgVI6JD1/2qbWkI6buMAQrTj7LV14PH gVQAV+u06VILQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4d1XKc0khFz4wBD; Wed, 05 Nov 2025 15:24:52 +1100 (AEDT) Date: Wed, 5 Nov 2025 15:19:22 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v7 4/5] tcp: Update data retransmission timeout Message-ID: References: <20251031054242.7334-1-yuhuang@redhat.com> <20251031054242.7334-5-yuhuang@redhat.com> <20251104054225.2b2a5a8c@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="otbooTaSZiozZSmh" Content-Disposition: inline In-Reply-To: <20251104054225.2b2a5a8c@elisabeth> Message-ID-Hash: ECNY3BM5B5DF76ET5EKOQIL44MYIXTLK X-Message-ID-Hash: ECNY3BM5B5DF76ET5EKOQIL44MYIXTLK 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: Yumei Huang , 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: --otbooTaSZiozZSmh Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 04, 2025 at 05:42:25AM +0100, Stefano Brivio wrote: > On Mon, 3 Nov 2025 20:32:14 +1100 > David Gibson wrote: >=20 > > On Mon, Nov 03, 2025 at 10:57:27AM +0800, Yumei Huang wrote: > > > On Mon, Nov 3, 2025 at 9:38=E2=80=AFAM David Gibson wrote: =20 > > > > > > > > On Fri, Oct 31, 2025 at 01:42:41PM +0800, Yumei Huang wrote: [snip] > > > > > @@ -589,12 +585,10 @@ static void tcp_timer_ctl(const struct ctx = *c, struct tcp_tap_conn *conn) > > > > > if (conn->flags & ACK_TO_TAP_DUE) { > > > > > it.it_value.tv_nsec =3D (long)ACK_INTERVAL * 1000 *= 1000; > > > > > } else if (conn->flags & ACK_FROM_TAP_DUE) { > > > > > - if (!(conn->events & ESTABLISHED)) { > > > > > - int exp =3D conn->retries - c->tcp.syn_line= ar_timeouts; =20 > > > > > > > > I didn't spot it in the previous patch, but this is (theoretically) > > > > buggy. conn->retries is unsigned, so the subtraction will be > > > > performed unsigned and only then cast to signed. I think that will > > > > probably do the right thing in practice, but I don't think that's > > > > guaranteed by the C standard (and might even be UB). =20 > > >=20 > > > I'm not sure, but I just googled it. IIUC, the uint8_t (conn->retries > > > and c->tcp.syn_linear_timeouts) will go through integer promotion > > > before subtraction. So the line is like: > > >=20 > > > int exp =3D (int) conn->retries - (int) c->tcp.syn_linear_timeout= s; > > >=20 > > > Please correct me if I'm wrong. =20 > >=20 > > Huh, I thought it would only be promoted if one of the operands was an > > int. >=20 > I thought and I think so too, yes. >=20 > > But C promotion rules are really confusing, so I could well be wrong. >=20 > Some references linked at: >=20 > https://archives.passt.top/passt-dev/20240103080834.24fa0a7a@elisabeth/ >=20 > and here, it's about paragraph 6.3.1.8, "Usual arithmetic conversions" > of C11 (that's what passt uses right now). >=20 > No double or float here, so this is the relevant text: >=20 > Otherwise, the integer promotions are performed on both operands. > Then the following rules are applied to the promoted operands: >=20 > If both operands have the same type, then no further conversion > is needed >=20 > now, are they really the same type? The standard doesn't define uint8_t. > If we see it as an unsigned int, with storage limited to 8 bits, then I > would call them the same type. So, I was a bit confused in the first place - I had thought both operands were literally uint8_t typed. But that's not the case, 'retries' is an unsigned int that's bit limited. > If we see c->tcp.syn_linear_timeouts as a character type instead > (6.3.1.1, "Boolean, characters, and integers"), then the integer > conversion rank of unsigned int (conn->retries) is greater than the rank > of char, and the same text applies (promotion of char to unsigned int). >=20 > But I'm fairly sure that's not the case. We used uint8_t, not short, > and not char. >=20 > All in all I don't think there's any possibility of promotion to a > signed int: for that, you would need one of the operands to be signed. > Or two unsigned chars/shorts (see examples below). Right. I'm fairly confident the arguments will be promoted to unsigned int, not signed. > If the operation is performed unsigned, and that's what gcc appears to > do here, promoting both operands to unsigned int (32-bit): >=20 > --- > $ cat pro_uh_oh_motion.c > #include > #include >=20 > int main() > { > uint8_t a =3D 0; > struct { unsigned b:3; } x =3D { 7 }; >=20 > printf("%u %i\n", a - x.b, a - x.b); > } > $ gcc -o pro_uh_oh_motion{,.c} > $ ./pro_uh_oh_motion > 4294967289 -7 > --- >=20 > ...do we really have a problem with that? To me it looks like the > behaviour is defined, and it's what we want. So, this is a second step, where we cast the result of the subtraction into a signed int of the same size. I'm not sure if that behaviour is defined for values outside the positive range the signed type can represent. In practice the obvious result on a two's complement machine will do what we need, but I'm not sure that C guarantees that. >=20 > These pages: >=20 > https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integ= er+conversion+rules > https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+intege= r+promotion+when+performing+bitwise+operations+on+integer+types+smaller+tha= n+int >=20 > show some problematic examples. This one is close to our case: >=20 > unsigned short x =3D 45000, y =3D 50000; > unsigned int z =3D x * y; >=20 > but there we have implicit promotion of the unsigned shorts to > int, which doesn't happen in our case, because we already have an > unsigned int. Also multiplication which is a whole other thing. > See also the examples with two unsigned shorts from: >=20 > https://cryptoservices.github.io/fde/2018/11/30/undefined-behavior.html >=20 > and there the operation would be performed as signed int. >=20 > On top of that, you have the fact that the original types can't store > the result of the operation. Here it's not the case. They can't though - the result is negative which can't be stored in an unsigned type. Now, wraparound / mod 2^n behaviour *is* defined for unsigned (but not signed) integer types. The second part where we assign an unsigned "negative" value into a signed variable might not be. --=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 --otbooTaSZiozZSmh Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmkK0EkACgkQzQJF27ox 2GcgdQ/+JLP8eCw6argCOekeJFEf2U3izco26B94LB0cynGgkptNaeVfYoVEpIw4 A8eHaoKeYOyEE64JZH0IQx2CS/0x5zbZJaPRP8/ldUeArqvQCCK0dRsKJtmtiqbG Vk6Z/GZINK846jTknofprA+mqh+ZEyPJDgDWFyEPWTK8tOiWUoimfv10xfRLV9D+ vqWIxNcGRmnBzEDOie53Ca/CVTjr0t8WtlfObQC8s8k2eC+bc0ANlg7vJU9vdh/4 KA6wF2PB4Ks+oOTEIi+R0C1JXa3QRe/8kjTDnqEj3/YcIHjCmelnSfDjUO1ic35/ eGIbf7lFxzfJIiYT5GWd5P721Fq88yNs/kFGM2e8TLzugbeiyBqNPTT82va6/oJ4 SoKkWQ/EqWiDAxo5y56Q8ZbPy9HHZVdQV1GLF+u4RWaHgY4wulz8XV6JAprHVxqy fcFwo5DlmgWU0t3jrwXnA3ZUkq+oL2eZpl139On3UfOjlrKwwVadRvgu8+MVXlkW R7IgnNHTv0r6QKJLFR9a8iarzK/HJ3abeNSK8h18iaYQC0taeHG3esW30W/47+j/ nCJdwumZJXfYVmYQ2IlJhCCLYTjrnz+7dlWopmUTObmB8X0n1JxcyWHsrIHP1O14 0FuDOrlzZL8z5CbzGglRoZIjqno8ccL0Aqjx8fIPAwPp1j1btMk= =8IEZ -----END PGP SIGNATURE----- --otbooTaSZiozZSmh--