David Miller wrote: > From: Chuck Lever > Date: Tue, 23 Oct 2007 11:44:28 -0400 > >> The get_seconds() function returns an unsigned long. To prevent incorrect >> comparison results between values saved in ts_recent_stamp and later >> invocations of get_seconds(), change the type of ts_recent_stamp to match >> the return type of get_seconds(). >> >> Signed-off-by: Chuck Lever >> Cc: > > I see two potential problems with this patch: > > 1) If you update struct tcp_options_received you should also > update struct tcp_timewait_sock similarly. > > The fact that you missed this suggests that you didn't > grep the tree to see how else this variable is used and > this makes me extra concerned about this patch's correctness. Perhaps the result of wishful thinking on my part. I was hoping for a small and self-contained change. > 2) There are computations in the TCP stack using this member that > probably care about the signedness, such as: > > net/ipv4/tcp_ipv4.c: get_seconds() - tcptw->tw_ts_recent_stamp > 1))) { > include/net/tcp.h: if (get_seconds() >= rx_opt->ts_recent_stamp + TCP_PAWS_24DAYS) > include/net/tcp.h: if (get_seconds() >= rx_opt->ts_recent_stamp + TCP_PAWS_24DAYS) > > We should make sure we understand what is expected here, and > why it would still be correct after making both ts_recent_stamp > members unsigned. Agreed. I wonder how one could construct a series of mixed case time stamp comparisons *on purpose* (and without documentation of this assumption) that produces consistently correct results. From the invocations of get_seconds() that I sampled, the design of these comparisons seems to assume that both sides of the comparison are non-negative. However, they do not seem to account for time crossing zero.