Date: Sat, 15 Jan 2011 17:05:02 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: Stefan `Sec` Zehl <sec@42.org>, freebsd-bugs@freebsd.org, FreeBSD-gnats-submit@freebsd.org Subject: Re: kern/154006: tcp "window probe" bug on 64bit Message-ID: <20110115164429.N16715@besplex.bde.org> In-Reply-To: <20110115143903.K16210@besplex.bde.org> References: <20110115013336.A314E2845B@ice.42.org> <20110115143903.K16210@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 15 Jan 2011, Bruce Evans wrote:
> ...
> Later code in tcp_output uses bogus casts to long and larger code instead:
>
> % if (recwin < (long)(tp->rcv_adv - tp->rcv_nxt))
> % recwin = (long)(tp->rcv_adv - tp->rcv_nxt);
> % if (recwin > (long)TCP_MAXWIN << tp->rcv_scale)
> % recwin = (long)TCP_MAXWIN << tp->rcv_scale;
> % ...
> % if (recwin > 0 && SEQ_GT(tp->rcv_nxt + recwin, tp->rcv_adv))
> % tp->rcv_adv = tp->rcv_nxt + recwin;
>
> Note that the first statement avoids using the technically incorrect
> SEQ_FOO() although its internals are better (cast to int instead of
> long). It uses cases essentially like yours. Then further analysis
^^^^^ a cast
> is simpler because everything is converted to long. The second starement
> is similar to the first half of the broken expression. Large code using
> if's and else's and tests (x >= y) before subtracting y from x is much
> easier to get right than 1 complicated 1-statement expression like the
> broken one. It takes these (x >= y) tests to make code with mixed types
> obviously correct. But I prefer small fast code with ints for everything,
> since type analyis is too hard.
But the casts to long are not good. Here they have no effect except
to break the warning about the bad type of `recwin'. recwin has type
long, so assignment to it does the same conversion as the cast, possibly
with a warning about the implicit conversion if it might overflow (can
overflow only on 32-bit arches). The code depends on rcv_adv being
sequentially >= rcv_next with or without the cast. Otherwise, the
difference is huge unsigned int, and the cast only changes this (by
benign overflow) on 32-bit arches.
This can be fixed by casting to int instead of long (now the cast may have
an effect, and breaking the warning may be intential), or my proposed
SEQ_DIFF() macros work well here:
int recwin, sd;
...
sd = SEQ_NONNEG_NONLARGE_DIFF(tp->rcv_adv, tp->rcv_nxt);
/*
* SEQ_DIFF() supports negative differences;
* SEQ_NONNEG_NONGLARGE_DIFF() KASSERT()s that they don't happen and
* are not too large. This name is too long.
*/
if (recwin < sd)
recwin = sd;
...
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110115164429.N16715>