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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 30 Jun 2014 14:50:22 +0300 From: Octavian Purdila <octavian.purdila@...el.com> To: Neal Cardwell <ncardwell@...gle.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 Sat, Jun 28, 2014 at 5:16 AM, Neal Cardwell <ncardwell@...gle.com> wrote: > 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! > Hi Neal, > 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. In that case perhaps it is better to add a new field (or rename the existing one if it is not needed anymore) to store the syn arrival time? I think it is confusing to store the syn arrival time in the "synack sent time" field. -- 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
Powered by blists - more mailing lists