On Fri, 12 Jun 2009, John Baldwin wrote:
On Thursday 11 June 2009 11:12:58 pm Bruce Evans wrote:
On Thu, 11 Jun 2009, John Baldwin wrote:
On Wednesday 10 June 2009 11:53:16 pm Bruce Evans wrote:
On Wed, 10 Jun 2009, John Baldwin wrote:
I wanted to match 'ticks' and figured changing 'ticks' was a far larger
headache.
By changing the signedness you get undefined behaviour for the other types
too, and risk new and/or different sign extension bugs.
FWIW, the variables were signed before they were changed to unsigned and are now
back as signed again. It just seems really odd to have the types not match
'ticks' especially since many of the values are just cached copies of 'ticks'.
No, they were originally u_long and stayed that way until you changed them
(except possibly for newer variables). The type of `ticks' is just wrong,
and fixing that would take more work, but there is no reason to propagate
this bug to new variables. The original version is:
% Index: tcp_var.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/netinet/tcp_var.h,v
% retrieving revision 1.51
% retrieving revision 1.52
% diff -u -2 -r1.51 -r1.52
% --- tcp_var.h 28 Aug 1999 00:49:33 -0000 1.51
% +++ tcp_var.h 30 Aug 1999 21:17:07 -0000 1.52
% ...
% @@ -99,10 +103,10 @@
% u_int t_maxopd; /* mss plus options */
%
% - u_int t_idle; /* inactivity time */
This even had a reasonable type.
% - u_long t_duration; /* connection duration */
Maybe copying this type is responsible for the sizing bug.
% - int t_rtt; /* round trip time */
This was a short in FreeBSD-1 and presumably in Net/2. Since it is unrelated
to `ticks' and doesn't need to exceed INT_MAX (or maybe even SHRT_MAX), a
signed type is best for it.
% + u_long t_rcvtime; /* inactivity time */
% + u_long t_starttime; /* time connection was established */
New variables.
% + int t_rtttime; /* round trip time */
This was t_rtt in the above. This commit just regressed its name (to a
more verbose one with a bogus extra "t" ("ttime" = "time time"). There is
now also a t_bw_rtttime with the same bugs. Other rtt variables are
still spelled correctly, but their types are all over the map: from Mar
30 sources:
| icmp6.h:struct rttimer;
| sctp_structs.h: uint32_t prev_rtt;
| sctp_structs.h: uint8_t do_rtt;
| sctp_uio.h: uint32_t spinfo_srtt;
| sctp_uio.h: uint32_t rtt;
| sctp_uio.h: uint32_t sctps_fastretransinrtt; /* number of multiple
FR in a
| tcp.h: u_int32_t tcpi_rtt; /* Smoothed RTT in
usecs. */
| tcp.h: u_int32_t tcpi_rttvar; /* RTT variance in
usecs. */
| tcp.h: u_int32_t __tcpi_rcv_rtt;
| tcp_hostcache.h: u_long rmx_rtt; /* estimated round trip time */
| tcp_hostcache.h: u_long rmx_rttvar; /* estimated rtt variance */
| tcp_timer.h: * up in the srtt because the timestamp is often calculated at
| tcp_var.h: u_long t_starttime; /* time connection was
established */
| tcp_var.h: int t_rtttime; /* round trip time */
| tcp_var.h: int t_bw_rtttime; /* used for bandwidth
calculation */
| tcp_var.h: int t_srtt; /* smoothed round-trip time */
| tcp_var.h: int t_rttvar; /* variance in round-trip time
*/
This was short in FreeeBSD-1.
| tcp_var.h: u_int t_rttmin; /* minimum rtt allowed */
This was u_short in FreeeBSD-1.
| tcp_var.h: u_int t_rttbest; /* best rtt we've seen */
| tcp_var.h: u_long t_rttupdated; /* number of times rtt sampled
*/
| tcp_var.h: int t_rttlow; /* smallest observerved RTT */
| tcp_var.h: u_long rmx_rtt; /* estimated round trip time */
| tcp_var.h: u_long rmx_rttvar; /* estimated rtt variance */
| tcp_var.h: u_long t_starttime;
| tcp_var.h: * With these scales, srtt has 3 bits to the right of the binary
point,
| tcp_var.h: * and thus an "ALPHA" of 0.875. rttvar has 2 bits to the right of
the
| tcp_var.h:#define TCP_RTT_SCALE 32 /* multiplier for srtt;
3 bits frac. */
| tcp_var.h:#define TCP_RTT_SHIFT 5 /* shift for srtt; 3
bits frac. */
| tcp_var.h:#define TCP_RTTVAR_SCALE 16 /* multiplier for
rttvar; 2 bits */
| tcp_var.h:#define TCP_RTTVAR_SHIFT 4 /* shift for rttvar; 2
bits */
| tcp_var.h: * The initial retransmission should happen at rtt + 4 * rttvar.
| tcp_var.h: * Because of the way we do the smoothing, srtt and rttvar
| tcp_var.h: max((tp)->t_rttmin, (((tp)->t_srtt >> (TCP_RTT_SHIFT -
TCP_DELTA_SHIFT)) \
| tcp_var.h: + (tp)->t_rttvar) >> TCP_DELTA_SHIFT)
| tcp_var.h: u_long tcps_segstimed; /* segs where we tried to get
rtt */
| tcp_var.h: u_long tcps_rttupdated; /* times we succeeded */
| tcp_var.h: u_long tcps_cachedrtt; /* times cached RTT in route
updated */
| tcp_var.h: u_long tcps_cachedrttvar; /* times cached rttvar updated
*/
| tcp_var.h: u_long tcps_usedrtt; /* times RTT initialized from
route */
| tcp_var.h: u_long tcps_usedrttvar; /* times RTTVAR initialized
from rt */
| tcp_var.h: { "rttdflt", CTLTYPE_INT }, \
| vinet.h: int _tcp_inflight_rttthresh;
| vinet.h:#define V_tcp_inflight_rttthresh
VNET_INET(tcp_inflight_rttthresh)
Back to the old diff:
% tcp_seq t_rtseq; /* sequence number being timed */
%
% - int t_rxtcur; /* current retransmit value */
% + int t_rxtcur; /* current retransmit value (ticks) */
This was short in FreeBSD-1. short would probably have remained enough
with HZ=100. Less probably with HZ=1000.
% u_int t_maxseg; /* maximum segment size */
% int t_srtt; /* smoothed round-trip time */
% @@ -132,4 +136,8 @@
% tcp_cc cc_send; /* send connection count */
% tcp_cc cc_recv; /* receive connection count */
% +/* experimental */
% + u_long snd_cwnd_prev; /* cwnd prior to retransmit */
% + u_long snd_ssthresh_prev; /* ssthresh prior to retransmit */
% + u_long t_badrxtwin; /* window for retransmit recovery */
% };
Probably an error for these to be u_long. However, in old times people
worried more about int being 16 bits and used long too much. Using fixed-
width types would be worse.
%
% ...
I checked more closely for uses of these variables and found:
- most uses of t_rcvtime are of the form:
if ((ticks - tp->t_rcvtime) >= tcp_keepidle + tcp_maxidle)
This is safe with int variables modulo its undefined behaviour being
benign. The expression "ticks - tp->t_rcvtime" is almost always
parenthesized, as would be needed if it were cast, but this is
especially bogus when it is compared with an unparenthesized expression
as above.
- t_starttime is also a struct member in struct tcptw. (struct tcptw has
many related style bugs. Only 5 of its members have a name beginning
with tw_; 5 have no apparent prefix at all, 1 has an apparent ts_
prefix that is not a prefix; t_recent has the same confusing prefix as
t_starttime).
- struct tcpcb has many bugs of the same type :-(. The recently changed
ts_recent_age is one (ts_ only appears to be a prefix), and near it
is the unprefixed request_r_scale...
- Apart from the above style bugs, the ofuscated t_starttime in struct
tcptw gives another instance of the bug that you just fixed. It still
has type u_long (you didn't touch it), and is used in the problematic
way: from tcp_timewait.c:
% tw->t_starttime = tp->t_starttime;
% ...
% int
% tcp_twrecycleable(struct tcptw *tw)
% {
% tcp_seq new_iss = tw->iss;
% tcp_seq new_irs = tw->irs;
%
% INP_INFO_WLOCK_ASSERT(&tcbinfo);
% new_iss += (ticks - tw->t_starttime) * (ISN_BYTES_PER_SECOND / hz);
% new_irs += (ticks - tw->t_starttime) * (MS_ISN_BYTES_PER_SECOND / hz);
This gives the usual problem when tw_starttime was set from `ticks' before
`ticks' overflowed (ticks < INT_MAX), but `ticks' has now overflowed to be
negative.
%
% if (SEQ_GT(new_iss, tw->snd_nxt) && SEQ_GT(new_irs, tw->rcv_nxt))
% return (1);
% else
% return (0);
% }
- there are 2 broken comparisons involving t_badrxtwin altogether: from
tcp_input.c:
% if (tp->t_rxtshift == 1 &&
% ticks < tp->t_badrxtwin) {
% ...
% if (tp->t_rxtshift == 1 && ticks < tp->t_badrxtwin) {
Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
