Re: [PATCH 4/5] NET: Make ts_recent_stamp unsigned

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

David Miller wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 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 <chuck.lever@oracle.com>
>> Cc: <netdev@vger.kernel.org>
> 
> 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.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 315 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
email;internet:chuck dot lever at nospam oracle dot com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard