Date: Sun, 21 Mar 2010 13:15:27 +0000 From: Rui Paulo <rpaulo@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-net@freebsd.org, Chris Harrer <cjharrer@comcast.net> Subject: Re: Bug in tcp_output? Message-ID: <8FEEC471-81FB-4FCA-BA4E-8E90048EFD8B@freebsd.org> In-Reply-To: <20100321173545.O3020@delplex.bde.org> References: <006f01cac6d8$5fc03cb0$1f40b610$@net> <2FEF7D36-31B7-4FD7-9350-EECA7B38D519@freebsd.org> <20100321173545.O3020@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 Mar 2010, at 07:17, Bruce Evans wrote:
> On Sat, 20 Mar 2010, Rui Paulo wrote:
>=20
>> On 18 Mar 2010, at 20:19, Chris Harrer wrote:
>>>=20
>>> In the following block of code, running on a x86_64 platform, I =
believe that
>>> cwin should be declared as an int:
>>> ...
>>> else {
>>>=20
>>> long cwin; =DF-- Should be an int
>>> ...
>>> if (len > 0) {
>>>=20
>>> cwin =3D tp->snd_cwnd -
>>>=20
>>> (tp->snd_nxt - tp->sack_newdata) =
-
>>>=20
>>> sack_bytes_rxmt;
>>>=20
>>> if (cwin < 0)
>>>=20
>>> cwin =3D 0;
>>>=20
>>> len =3D lmin(len, cwin);
>>>=20
>>> }
>>>=20
>>> }
>>>=20
>>> }
>>>=20
>>>=20
>>>=20
>>> Consider the case where:
>>>=20
>>> sack_rxmit =3D 0
>>>=20
>>> sack_bytes_rxmt =3D 0x2238
>>>=20
>>> off =3D 0
>>>=20
>>> len =3D0xa19c
>>>=20
>>> tp->snd_cwnd =3D 0x2238
>>>=20
>>> tp->snd_nxt =3D 0xdd6d7974
>>>=20
>>> tp->sack_newdata =3D 0xdd6d6858
>>>=20
>>> In this case cwin evaluates to 0x00000000ffffe37c, which is not <0, =
but
>>> instead huge. This causes the remaining data on the socket=92s =
so->so_snd
>>> buffer to be sent to the network causing more problems at the =
receiver which
>>> is already dropping frames.
>>=20
>> I see. This is most likely a bug. Can you send-pr so this doesn't get =
lost?
>=20
> What bug do you see? This is most likely not a bug. I only see the
> following bugs
> - the suggestion to increase the fragility of the code by changing =
cwin to
> int
> - lots of whitespace lossage
> - the style bug in the declaration of cwin (nested declaration)
> - lots fragile but working code. It depends on the machine being a =
normal
> 2's complement one. It would fail on normal 1's complement machines =
and
> on abnormal 2's complement ones, but so would many other things in =
the
> kernel.
> - type and arithmetic errors that are not made at runtime resulting in =
a
> value that wouldn't work, though the runtime value would.
>=20
> Relevant code quoted again, with the whitespace fixed:
>=20
>>> cwin =3D tp->snd_cwnd -
>>> (tp->snd_nxt - tp->sack_newdata) =
-
>>> sack_bytes_rxmt;
>=20
> On 64-bit machines, with the above values, this is:
>=20
> rhs =3D (u_long)0x2238UL -
> ((tcp_seq)0xdd6d7974 -
> (tcp_seq)0xdd6d6858) -
> (int)0x2238;
> =3D (u_long)0x2238UL -
> ((uint32_t)0xdd6d7974 -
> (uint32_t)0xdd6d6858) -
> (int)0x2238;
> =3D (u_long)0x2238UL -
> (u_int)0x111c -
> (int)0x2238;
> =3D (u_long)0x111c -
> (int)0x2238;
> =3D (u_long)0x111c -
> (u_long)0x2238;
> =3D (u_long)0xffffffffffffeee4;
> cwin =3D (long)rhs;
> =3D -(long)0x111c;
>=20
> I might have made arithmetic errors too, but I'm sure that I got the
> conversions essentially correct. On machines with 64-bit u_longs,
> almost everything is evaluated modulo 2^64. This gives a large =
positive
> value, but not one with the top bits set up to only the 31st as would
> happen on machines with 32-bit u_longs. Then the final conversion to
> long gives a negative value.
Right. I made some bad calculations.
>=20
> This is fragile, but it is a standard 2's complement hack. It would
> fail mainly on normal ones complement machines when the rhs is
> (u_long)0xFF...FF. Then the lhs is probably negative 0, which is
> not less than 0.
>=20
> The fragility is essentially the same on machines with 32-bit u_longs.
> Almost everything is evaluated modulo 2^32...
>=20
> Using 64-bit u_longs for tp->snd_cwnd (and thus for almost the entire
> calculation) is exessive but doesn't cause any problems.
>=20
> Using a signed type for sack_bytes_rxmt asks for sign extension bugs =
but
> doesn't get them. Here it is promoted to a u_long so there are no
> sign extension bugs for it here.
>=20
> Using a signed type for cwin is essential for the comparison of cwin
> with 0 to work.
Right.
> This signed type should have the same size as the rhs
> to avoid even more fragility (if it were int, then you would have to
> worry about the value being changed to a non-working value by the
> implementation-defined conversion of the rhs to cwin not just for
> values larger than LONG_MAX but also for ones larger than INT_MAX.
> `int' should work in practice. This and other things depend on the
> difference of the tcp_seq's not being anywhere near as large as
> 0x7fffffff).
I assumed that Chris saw a problem with this code after being hit by =
some TCP/IP interop issue. Was this the case?
--
Rui Paulo
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8FEEC471-81FB-4FCA-BA4E-8E90048EFD8B>