[PATCH 4/5] NET: Make ts_recent_stamp unsigned

[PATCH 4/5] NET: Make ts_recent_stamp unsigned

[Chuck Lever]

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>
---

 include/linux/tcp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index bac17c5..129ddb4 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -206,7 +206,7 @@ struct tcp_sack_block {
 
 struct tcp_options_received {
 /*	PAWS/RTTM data	*/
-	long	ts_recent_stamp;/* Time we stored ts_recent (for aging) */
+	unsigned long ts_recent_stamp;/* Time we stored ts_recent (for aging) */
 	u32	ts_recent;	/* Time stamp to echo next		*/
 	u32	rcv_tsval;	/* Time stamp value             	*/
 	u32	rcv_tsecr;	/* Time stamp echo reply        	*/

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

[David Miller]

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.

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.

Thanks.

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

[Chuck Lever]

[-- 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