netdev - Re: [net-next v2 02/13] tcp: tcp_v[46]_conn_request: fix snt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADVnQykKYPVt-g8WtE29Xu_0Si-Lx_pMxnix4i2BqFhAV9BD8Q@mail.gmail.com>
Date:	Fri, 27 Jun 2014 22:16:35 -0400
From:	Neal Cardwell <ncardwell@...gle.com>
To:	Octavian Purdila <octavian.purdila@...el.com>
Cc:	Netdev <netdev@...r.kernel.org>, Daniel Lee <longinus00@...il.com>,
	Yuchung Cheng <ycheng@...gle.com>, Jerry Chu <hkchu@...gle.com>
Subject: Re: [net-next v2 02/13] tcp: tcp_v[46]_conn_request: fix snt_synack initialization

On Wed, Jun 25, 2014 at 10:09 AM, Octavian Purdila
<octavian.purdila@...el.com> wrote:
> Commit 016818d07 (tcp: TCP Fast Open Server - take SYNACK RTT after
> completing 3WHS) changes the code to only take a snt_synack timestamp
> when a SYNACK transmit or retransmit succeeds. This behaviour is later
> broken by commit 843f4a55e (tcp: use tcp_v4_send_synack on first
> SYN-ACK), as snt_synack is now updated even if tcp_v4_send_synack
> fails.
>
> Also, commit 3a19ce0ee (tcp: IPv6 support for fastopen server) misses
> the required IPv6 updates for 016818d07.
>
> This patch makes sure that snt_synack is updated only when the SYNACK
> trasnmit/retransmit succeeds, for both IPv4 and IPv6.
>
> Cc: Cardwell <ncardwell@...gle.com>
> Cc: Daniel Lee <longinus00@...il.com>
> Cc: Yuchung Cheng <ycheng@...gle.com>
>
> Signed-off-by: Octavian Purdila <octavian.purdila@...el.com>

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 08ae3da..a962455 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -497,6 +497,8 @@ static int tcp_v6_send_synack(struct sock *sk, struct dst_entry *dst,
>                 skb_set_queue_mapping(skb, queue_mapping);
>                 err = ip6_xmit(sk, skb, fl6, np->opt, np->tclass);
>                 err = net_xmit_eval(err);
> +               if (!tcp_rsk(req)->snt_synack && !err)
> +                       tcp_rsk(req)->snt_synack = tcp_time_stamp;
>         }

Octavian, thanks for finding this issue!

I think we should resolve the inconsistency you have discovered by
converging on the approach in Yuchung's 843f4a55e (tcp: use
tcp_v4_send_synack on first SYN-ACK), which restores the original
behavior of snt_synack from the commit where it was added by Jerry:
9ad7c049f0f79 ("tcp: RFC2988bis + taking RTT sample from 3WHS for the
passive open side") in v3.1. In that commit tcp_v4_conn_request() and
tcp_v6_conn_request() unconditionally store the time at which the
server received the first client SYN in snt_synack:

  tcp_rsk(req)->snt_synack = tcp_time_stamp;

At Google we have found that behavior very useful because it allows
extracting useful performance metrics summarizing how long it took to
establish a passive TCP connection.

So I would suggest that we fix this inconsistency you have noted by
removing the two lines in tcp_v4_send_synack() that touch snt_synack.
That is the approach we have been using at Google recently, and I
think it was an oversight that we didn't remove those two lines.

Thanks!

neal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html