From owner-svn-src-head@FreeBSD.ORG Thu Jun 11 14:22:29 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9F947106566C; Thu, 11 Jun 2009 14:22:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 652838FC13; Thu, 11 Jun 2009 14:22:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 153D746B2C; Thu, 11 Jun 2009 10:22:29 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id B11668A06A; Thu, 11 Jun 2009 10:22:27 -0400 (EDT) From: John Baldwin To: Bruce Evans Date: Thu, 11 Jun 2009 10:18:20 -0400 User-Agent: KMail/1.9.7 References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <20090611124334.A21109@delplex.bde.org> In-Reply-To: <20090611124334.A21109@delplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906111018.20703.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Thu, 11 Jun 2009 10:22:27 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r193941 - head/sys/netinet X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jun 2009 14:22:30 -0000 On Wednesday 10 June 2009 11:53:16 pm Bruce Evans wrote: > On Wed, 10 Jun 2009, John Baldwin wrote: > > > Log: > > Change a few members of tcpcb that store cached copies of ticks to be ints > > instead of unsigned longs. This fixes a few overflow edge cases on 64-bit > > platforms. Specifically, if an idle connection receives a packet shortly > > I think the variables should still be unsigned (ints now). Otherwise there > is at best benign undefined behaviour when the variables overflow at > INT_MAX, while the code is apparently designed to work with unsigned > values (it casts to int to look at differences between the unsigned values). I wanted to match 'ticks' and figured changing 'ticks' was a far larger headache. > > Modified: head/sys/netinet/tcp_input.c > > ============================================================================== > > --- head/sys/netinet/tcp_input.c Wed Jun 10 18:26:02 2009 (r193940) > > +++ head/sys/netinet/tcp_input.c Wed Jun 10 18:27:15 2009 (r193941) > > @@ -1778,7 +1778,7 @@ tcp_do_segment(struct mbuf *m, struct tc > > TSTMP_LT(to.to_tsval, tp->ts_recent)) { > > > > /* Check to see if ts_recent is over 24 days old. */ > > - if ((int)(ticks - tp->ts_recent_age) > TCP_PAWS_IDLE) { > > + if ((ticks - tp->ts_recent_age) > TCP_PAWS_IDLE) { > > The variables are now ints, and there is now also overflow in the > subtraction. E.g., INT_MAX - INT_MIN now overflows. On 2's complement > machines it normally overflows to -2, which is probably the wrong value > (the large unsigned value 0xFFF...FEU is probably correct), but it is > the same value as is given by casting the difference of unsigned values > to int ((int)0xFFF...FEU = -2). This is because, when representing times > mod UINT_MAX+1, all time differences up to UINT_MAX can be represented, > but only if we keep track of which operand is older (here we probably do > know that ts_recent_age is older); if we don't keep track then we normally > assume that (unsigned) differences smaller than INT_MAX mean that the > difference is nonnegative while differences larger than INT_MAX mean > that the difference is negative. The signed interpreatation works well > if we know that the nonnegative differences never exceed INT_MAX. I believe in these cases that 1) the differences should never exceed INT_MAX, and 2) we do know which value should be older. > Style bug: without the cast, the above has excessive parentheses. Ok. > > Modified: head/sys/netinet/tcp_usrreq.c > > ============================================================================== > > --- head/sys/netinet/tcp_usrreq.c Wed Jun 10 18:26:02 2009 (r193940) > > +++ head/sys/netinet/tcp_usrreq.c Wed Jun 10 18:27:15 2009 (r193941) > > @@ -1823,7 +1823,7 @@ db_print_tcpcb(struct tcpcb *tp, const c > > tp->snd_recover); > > > > db_print_indent(indent); > > - db_printf("t_maxopd: %u t_rcvtime: %lu t_startime: %lu\n", > > + db_printf("t_maxopd: %u t_rcvtime: %u t_startime: %u\n", > > tp->t_maxopd, tp->t_rcvtime, tp->t_starttime); > > > > db_print_indent(indent); > > You still print all the times as unsigned. Since the variables are now > signed, this is now a printf format error, which gcc still doesn't detect > but I usually do. Ok. > > Modified: head/sys/netinet/tcp_var.h > > ============================================================================== > > --- head/sys/netinet/tcp_var.h Wed Jun 10 18:26:02 2009 (r193940) > > +++ head/sys/netinet/tcp_var.h Wed Jun 10 18:27:15 2009 (r193941) > > @@ -139,8 +139,8 @@ struct tcpcb { > > > > u_int t_maxopd; /* mss plus options */ > > > > - u_long t_rcvtime; /* inactivity time */ > > - u_long t_starttime; /* time connection was established */ > > + int t_rcvtime; /* inactivity time */ > > + int t_starttime; /* time connection was established */ > > int t_rtttime; /* round trip time */ > > tcp_seq t_rtseq; /* sequence number being timed */ > > > > @@ -167,7 +167,7 @@ struct tcpcb { > > u_char rcv_scale; /* window scaling for recv window */ > > u_char request_r_scale; /* pending window scaling */ > > u_int32_t ts_recent; /* timestamp echo data */ > > - u_long ts_recent_age; /* when last updated */ > > + int ts_recent_age; /* when last updated */ > > u_int32_t ts_offset; /* our timestamp offset */ > > > > tcp_seq last_ack_sent; > > @@ -175,7 +175,7 @@ struct tcpcb { > > u_long snd_cwnd_prev; /* cwnd prior to retransmit */ > > u_long snd_ssthresh_prev; /* ssthresh prior to retransmit */ > > tcp_seq snd_recover_prev; /* snd_recover prior to retransmit */ > > - u_long t_badrxtwin; /* window for retransmit recovery */ > > + int t_badrxtwin; /* window for retransmit recovery */ > > u_char snd_limited; /* segments limited transmitted */ > > /* SACK related state */ > > int snd_numholes; /* number of holes seen by sender */ > > > > Should all be changed to u_int? > > There still seem to be some very nice sign extension/overflow bugs. > E.g., `ticks' is (bogusly) signed. After changing the above variables > to int to be bug for bug compatible with `ticks', the semantics of > expressions not directly touched in this commit is changed too. E.g., > there is the expression `ticks < tp->t_badrxtwin'. Changing the signedness > of the type of tp_t_badrxtwin stops promotion of `ticks' to unsigned in > this expression. However, this expression seems to have been just broken > before, and the change only moves the bug slightly (from times near where > `ticks' wraps around at to to times near where `ticks' overflows at > INT_MAX). To handle wraparound and avoid overflow, such expressions should > be written as (int)(ticks - tp->t_badrxtwin) < 0 where the variables have > unsigned type. This seems to have already been done for all the other > variables changed in this commit, except the cast to int is missing in > some cases. Yes, I noticed the t_badrxtwin breakage but wasn't sure how best to fix it (or at least thought that it should be a separate followup since it was already broken with wraparound). I considered making it work more like the other variables such as t_rcvtime that merely store a cached value of 'ticks' and then use 'ticks - foo' and compare it with the given interval. This would entail renaming 't_badrxtwin' to 't_badrxttime' or some such. That seems clearer to me than '(int)(ticks - t_badrxtwin) < 0'. -- John Baldwin