[PATCH net] tcp: add proper TS val into RST packets

[PATCH net] tcp: add proper TS val into RST packets

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

RST packets sent on behalf of TCP connections with TS option (RFC 7323
TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.

A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100 ecr 0], length 0
B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss 1460,nop,nop,TS val 7264344 ecr 100], length 0
A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr 7264344], length 0

B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0 ecr 110], length 0

We need to call skb_mstamp_get() to get proper TS val,
derived from skb->skb_mstamp

Note that RFC 1323 was advocating to not send TS option in RST segment,
but RFC 7323 recommends the opposite :

  Once TSopt has been successfully negotiated, that is both <SYN> and
  <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
  segment for the duration of the connection, and SHOULD be sent in an
  <RST> segment (see Section 5.2 for details)

Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_output.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f9a8a12b62ee..1100ffe4a722 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2897,6 +2897,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority)
 	skb_reserve(skb, MAX_TCP_HEADER);
 	tcp_init_nondata_skb(skb, tcp_acceptable_seq(sk),
 			     TCPHDR_ACK | TCPHDR_RST);
+	skb_mstamp_get(&skb->skb_mstamp);
 	/* Send it off. */
 	if (tcp_transmit_skb(sk, skb, 0, priority))
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTFAILED);

Re: [PATCH net] tcp: add proper TS val into RST packets

[Neal Cardwell]

On Wed, Sep 23, 2015 at 4:19 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> RST packets sent on behalf of TCP connections with TS option (RFC 7323
> TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.
>
> A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100 ecr 0], length 0
> B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss 1460,nop,nop,TS val 7264344 ecr 100], length 0
> A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr 7264344], length 0
>
> B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0 ecr 110], length 0
>
> We need to call skb_mstamp_get() to get proper TS val,
> derived from skb->skb_mstamp
>
> Note that RFC 1323 was advocating to not send TS option in RST segment,
> but RFC 7323 recommends the opposite :
>
>   Once TSopt has been successfully negotiated, that is both <SYN> and
>   <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
>   segment for the duration of the connection, and SHOULD be sent in an
>   <RST> segment (see Section 5.2 for details)
>
> Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH net] tcp: add proper TS val into RST packets

[Yuchung Cheng]

On Wed, Sep 23, 2015 at 1:30 PM, Neal Cardwell <ncardwell@google.com> wrote:
>
> On Wed, Sep 23, 2015 at 4:19 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > RST packets sent on behalf of TCP connections with TS option (RFC 7323
> > TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.
> >
> > A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100 ecr 0], length 0
> > B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss 1460,nop,nop,TS val 7264344 ecr 100], length 0
> > A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr 7264344], length 0
> >
> > B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0 ecr 110], length 0
> >
> > We need to call skb_mstamp_get() to get proper TS val,
> > derived from skb->skb_mstamp
> >
> > Note that RFC 1323 was advocating to not send TS option in RST segment,
> > but RFC 7323 recommends the opposite :
> >
> >   Once TSopt has been successfully negotiated, that is both <SYN> and
> >   <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
> >   segment for the duration of the connection, and SHOULD be sent in an
> >   <RST> segment (see Section 5.2 for details)
Nice comment. I think it's worth adding why we choose not to follow
RFC7323 sec 5.2 recommendation on setting TSval to 0?


>
> >
> > Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
>
> Acked-by: Neal Cardwell <ncardwell@google.com>
>
> Thanks, Eric!
>
> neal

Re: [PATCH net] tcp: add proper TS val into RST packets

[Eric Dumazet]

On Wed, 2015-09-23 at 13:36 -0700, Yuchung Cheng wrote:
> On Wed, Sep 23, 2015 at 1:30 PM, Neal Cardwell <ncardwell@google.com> wrote:
> >
> > On Wed, Sep 23, 2015 at 4:19 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > RST packets sent on behalf of TCP connections with TS option (RFC 7323
> > > TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.
> > >
> > > A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100 ecr 0], length 0
> > > B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss 1460,nop,nop,TS val 7264344 ecr 100], length 0
> > > A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr 7264344], length 0
> > >
> > > B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0 ecr 110], length 0
> > >
> > > We need to call skb_mstamp_get() to get proper TS val,
> > > derived from skb->skb_mstamp
> > >
> > > Note that RFC 1323 was advocating to not send TS option in RST segment,
> > > but RFC 7323 recommends the opposite :
> > >
> > >   Once TSopt has been successfully negotiated, that is both <SYN> and
> > >   <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
> > >   segment for the duration of the connection, and SHOULD be sent in an
> > >   <RST> segment (see Section 5.2 for details)
> Nice comment. I think it's worth adding why we choose not to follow
> RFC7323 sec 5.2 recommendation on setting TSval to 0?

We revert to prior behavior. This is a bug fix, as it clearly was not
even considered in 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")

In 5 years, if/when all TCP stack are RFC 7323 ready, we might consider
to decide to send TS val = 0

Problem with RFC is they suppose both senders and receivers are ready
and support new recommendations.

Linux always sent TSopt in its RST packets, and apparently it works
well.

It is premature to send TS val = 0 we believe.

[PATCH v2 net] tcp: add proper TS val into RST packets

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

RST packets sent on behalf of TCP connections with TS option (RFC 7323
TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.

A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100
ecr 0], length 0
B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss
1460,nop,nop,TS val 7264344 ecr 100], length 0
A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr
7264344], length 0

B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0
ecr 110], length 0

We need to call skb_mstamp_get() to get proper TS val,
derived from skb->skb_mstamp

Note that RFC 1323 was advocating to not send TS option in RST segment,
but RFC 7323 recommends the opposite :

  Once TSopt has been successfully negotiated, that is both <SYN> and
  <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
  segment for the duration of the connection, and SHOULD be sent in an
  <RST> segment (see Section 5.2 for details)

Note this RFC recommends to send TS val = 0, but we believe it is
premature : We do not know if all TCP stacks are properly
handling the receive side :

   When an <RST> segment is
   received, it MUST NOT be subjected to the PAWS check by verifying an
   acceptable value in SEG.TSval, and information from the Timestamps
   option MUST NOT be used to update connection state information.
   SEG.TSecr MAY be used to provide stricter <RST> acceptance checks.


In 5 years, if/when all TCP stack are RFC 7323 ready, we might consider
to decide to send TS val = 0, if it buys something.

Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_output.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f9a8a12b62ee..1100ffe4a722 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2897,6 +2897,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority)
 	skb_reserve(skb, MAX_TCP_HEADER);
 	tcp_init_nondata_skb(skb, tcp_acceptable_seq(sk),
 			     TCPHDR_ACK | TCPHDR_RST);
+	skb_mstamp_get(&skb->skb_mstamp);
 	/* Send it off. */
 	if (tcp_transmit_skb(sk, skb, 0, priority))
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTFAILED);

Re: [PATCH v2 net] tcp: add proper TS val into RST packets

[Yuchung Cheng]

On Wed, Sep 23, 2015 at 2:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> RST packets sent on behalf of TCP connections with TS option (RFC 7323
> TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.
>
> A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100
> ecr 0], length 0
> B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss
> 1460,nop,nop,TS val 7264344 ecr 100], length 0
> A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr
> 7264344], length 0
>
> B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0
> ecr 110], length 0
>
> We need to call skb_mstamp_get() to get proper TS val,
> derived from skb->skb_mstamp
>
> Note that RFC 1323 was advocating to not send TS option in RST segment,
> but RFC 7323 recommends the opposite :
>
>   Once TSopt has been successfully negotiated, that is both <SYN> and
>   <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
>   segment for the duration of the connection, and SHOULD be sent in an
>   <RST> segment (see Section 5.2 for details)
>
> Note this RFC recommends to send TS val = 0, but we believe it is
> premature : We do not know if all TCP stacks are properly
> handling the receive side :
>
>    When an <RST> segment is
>    received, it MUST NOT be subjected to the PAWS check by verifying an
>    acceptable value in SEG.TSval, and information from the Timestamps
>    option MUST NOT be used to update connection state information.
>    SEG.TSecr MAY be used to provide stricter <RST> acceptance checks.
>
>
> In 5 years, if/when all TCP stack are RFC 7323 ready, we might consider
> to decide to send TS val = 0, if it buys something.
>
> Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>

Nice fix (and comment)!

> ---
>  net/ipv4/tcp_output.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index f9a8a12b62ee..1100ffe4a722 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2897,6 +2897,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority)
>         skb_reserve(skb, MAX_TCP_HEADER);
>         tcp_init_nondata_skb(skb, tcp_acceptable_seq(sk),
>                              TCPHDR_ACK | TCPHDR_RST);
> +       skb_mstamp_get(&skb->skb_mstamp);
>         /* Send it off. */
>         if (tcp_transmit_skb(sk, skb, 0, priority))
>                 NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTFAILED);
>
>

Re: [PATCH v2 net] tcp: add proper TS val into RST packets

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Sep 2015 14:00:21 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> RST packets sent on behalf of TCP connections with TS option (RFC 7323
> TCP timestamps) have incorrect TS val (set to 0), but correct TS ecr.
> 
> A > B: Flags [S], seq 0, win 65535, options [mss 1000,nop,nop,TS val 100
> ecr 0], length 0
> B > A: Flags [S.], seq 2444755794, ack 1, win 28960, options [mss
> 1460,nop,nop,TS val 7264344 ecr 100], length 0
> A > B: Flags [.], ack 1, win 65535, options [nop,nop,TS val 110 ecr
> 7264344], length 0
> 
> B > A: Flags [R.], seq 1, ack 1, win 28960, options [nop,nop,TS val 0
> ecr 110], length 0
> 
> We need to call skb_mstamp_get() to get proper TS val,
> derived from skb->skb_mstamp
> 
> Note that RFC 1323 was advocating to not send TS option in RST segment,
> but RFC 7323 recommends the opposite :
> 
>   Once TSopt has been successfully negotiated, that is both <SYN> and
>   <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
>   segment for the duration of the connection, and SHOULD be sent in an
>   <RST> segment (see Section 5.2 for details)
> 
> Note this RFC recommends to send TS val = 0, but we believe it is
> premature : We do not know if all TCP stacks are properly
> handling the receive side :
> 
>    When an <RST> segment is
>    received, it MUST NOT be subjected to the PAWS check by verifying an
>    acceptable value in SEG.TSval, and information from the Timestamps
>    option MUST NOT be used to update connection state information.
>    SEG.TSecr MAY be used to provide stricter <RST> acceptance checks.
> 
> 
> In 5 years, if/when all TCP stack are RFC 7323 ready, we might consider
> to decide to send TS val = 0, if it buys something.
> 
> Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks Eric.