[PATCH 1/2] tcp: setup random timestamp offset when write_seq already set

[PATCH 1/2] tcp: setup random timestamp offset when write_seq already set

[Alexey Kodanev]

Found that when random offset enabled (default) TCP client can
still start new connections with and without random offsets. Later,
if server does active close and re-use sockets in TIME-WAIT state,
new SYN from client can be rejected on PAWS check inside
tcp_timewait_state_process().

Here is how to reproduce it with LTP netstress tool:
    netstress -R 1 &
    netstress -H 127.0.0.1 -lr 1000000 -a1

    [...]
    < S  seq 1956977072 win 43690 TS val 295618 ecr 459956970
    > .  ack 1956911535 win 342 TS val 459967184 ecr 1547117608
    < R  seq 1956911535 win 0 length 0
+1. < S  seq 1956977072 win 43690 TS val 296640 ecr 459956970
    > S. seq 657450664 ack 1956977073 win 43690 TS val 459968205 ecr 296640

Fixes: 95a22caee396 ("tcp: randomize tcp timestamp offsets for each connection")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 net/ipv4/tcp_ipv4.c |    7 ++++++-
 net/ipv6/tcp_ipv6.c |    8 +++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fe9da4f..7269e9e 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -232,12 +232,17 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	sk->sk_gso_type = SKB_GSO_TCPV4;
 	sk_setup_caps(sk, &rt->dst);
 
-	if (!tp->write_seq && likely(!tp->repair))
+	if (!tp->write_seq && likely(!tp->repair)) {
 		tp->write_seq = secure_tcp_sequence_number(inet->inet_saddr,
 							   inet->inet_daddr,
 							   inet->inet_sport,
 							   usin->sin_port,
 							   &tp->tsoffset);
+	} else if (likely(!tp->repair)) {
+		secure_tcp_sequence_number(inet->inet_saddr, inet->inet_daddr,
+					   inet->inet_sport, usin->sin_port,
+					   &tp->tsoffset);
+	}
 
 	inet->inet_id = tp->write_seq ^ jiffies;
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 4c60c6f..1eceeb9 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -284,12 +284,18 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
 
 	sk_set_txhash(sk);
 
-	if (!tp->write_seq && likely(!tp->repair))
+	if (!tp->write_seq && likely(!tp->repair)) {
 		tp->write_seq = secure_tcpv6_sequence_number(np->saddr.s6_addr32,
 							     sk->sk_v6_daddr.s6_addr32,
 							     inet->inet_sport,
 							     inet->inet_dport,
 							     &tp->tsoffset);
+	} else if (likely(!tp->repair)) {
+		secure_tcpv6_sequence_number(np->saddr.s6_addr32,
+					     sk->sk_v6_daddr.s6_addr32,
+					     inet->inet_sport, inet->inet_dport,
+					     &tp->tsoffset);
+	}
 
 	err = tcp_connect(sk);
 	if (err)
-- 
1.7.1

[PATCH 2/2] tcp: account for ts offset only if tsecr not zero

[Alexey Kodanev]

We can get SYN with zero tsecr, don't apply offset in this case.

Fixes: ee684b6f2830 ("tcp: send packets with a socket timestamp")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 net/ipv4/tcp_minisocks.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 28ce5ee..baff824 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -106,7 +106,8 @@ enum tcp_tw_status
 		tcp_parse_options(skb, &tmp_opt, 0, NULL);
 
 		if (tmp_opt.saw_tstamp) {
-			tmp_opt.rcv_tsecr	-= tcptw->tw_ts_offset;
+			if (tmp_opt.rcv_tsecr)
+				tmp_opt.rcv_tsecr -= tcptw->tw_ts_offset;
 			tmp_opt.ts_recent	= tcptw->tw_ts_recent;
 			tmp_opt.ts_recent_stamp	= tcptw->tw_ts_recent_stamp;
 			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
-- 
1.7.1

Re: [PATCH 1/2] tcp: setup random timestamp offset when write_seq already set

[Alexey Kodanev]

Hi,
On 18.02.2017 3:56, Alexey Kodanev wrote:
> Found that when random offset enabled (default) TCP client can
> still start new connections with and without random offsets. Later,
> if server does active close and re-use sockets in TIME-WAIT state,
> new SYN from client can be rejected on PAWS check inside
> tcp_timewait_state_process().
>

Actually, on second thoughts, we can just copy tsoffset from tw socket:

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 89a95da..f40a61d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -126,6 +126,7 @@ int tcp_twsk_unique(struct sock *sk, struct sock
*sktw, void *twp)
                        tp->write_seq = 1;
                tp->rx_opt.ts_recent       = tcptw->tw_ts_recent;
                tp->rx_opt.ts_recent_stamp = tcptw->tw_ts_recent_stamp;
+               tp->tsoffset               = tcptw->tw_ts_offset;
                sock_hold(sktw);
                return 1;
        }

Will test this and send a new version.

Thanks,
Alexey

Re: [PATCH 1/2] tcp: setup random timestamp offset when write_seq already set

[David Miller]

From: Alexey Kodanev <alexey.kodanev@oracle.com>
Date: Sat, 18 Feb 2017 03:56:11 +0300

> @@ -232,12 +232,17 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>  	sk->sk_gso_type = SKB_GSO_TCPV4;
>  	sk_setup_caps(sk, &rt->dst);
>  
> -	if (!tp->write_seq && likely(!tp->repair))
> +	if (!tp->write_seq && likely(!tp->repair)) {
>  		tp->write_seq = secure_tcp_sequence_number(inet->inet_saddr,
>  							   inet->inet_daddr,
>  							   inet->inet_sport,
>  							   usin->sin_port,
>  							   &tp->tsoffset);
> +	} else if (likely(!tp->repair)) {
> +		secure_tcp_sequence_number(inet->inet_saddr, inet->inet_daddr,
> +					   inet->inet_sport, usin->sin_port,
> +					   &tp->tsoffset);
> +	}

This would be so much easier to understand if it were coded as:

	if (!tp->repair) {
		seq = secure_tcp_sequence_number(...);
		if (!tp->write_seq)
			tp->write_seq = seq;
	}

Re: [PATCH 1/2] tcp: setup random timestamp offset when write_seq already set

[Alexey Kodanev]

On 20.02.2017 18:18, David Miller wrote:
> This would be so much easier to understand if it were coded as:
> 	if (!tp->repair) {
> 		seq = secure_tcp_sequence_number(...);
> 		if (!tp->write_seq)
> 			tp->write_seq = seq;
> 	}

Hi David,

Thought about not adding extra variable here...
Agree,it looks much better and easier to follow.

Should I send a patch with this version modified as you
suggested or we can inherit ts offsetfrom tcptw->tw_ts_offset
in tcp_twsk_unique()?

Thanks,
Alexey