From owner-freebsd-bugs@FreeBSD.ORG Sat Jan 15 06:05:16 2011 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8FFDD106566B; Sat, 15 Jan 2011 06:05:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id 1F5658FC14; Sat, 15 Jan 2011 06:05:15 +0000 (UTC) Received: from c122-106-165-206.carlnfd1.nsw.optusnet.com.au (c122-106-165-206.carlnfd1.nsw.optusnet.com.au [122.106.165.206]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p0F652Rl004389 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 15 Jan 2011 17:05:03 +1100 Date: Sat, 15 Jan 2011 17:05:02 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans In-Reply-To: <20110115143903.K16210@besplex.bde.org> Message-ID: <20110115164429.N16715@besplex.bde.org> References: <20110115013336.A314E2845B@ice.42.org> <20110115143903.K16210@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Stefan `Sec` Zehl , freebsd-bugs@freebsd.org, FreeBSD-gnats-submit@freebsd.org Subject: Re: kern/154006: tcp "window probe" bug on 64bit X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 15 Jan 2011 06:05:16 -0000 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