Date: Thu, 11 Jun 2009 10:18:20 -0400 From: John Baldwin <jhb@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r193941 - head/sys/netinet Message-ID: <200906111018.20703.jhb@freebsd.org> In-Reply-To: <20090611124334.A21109@delplex.bde.org> References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <20090611124334.A21109@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906111018.20703.jhb>