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
| ||
|
Message-ID: <1351415713.30380.398.camel@edumazet-glaptop> Date: Sun, 28 Oct 2012 10:15:13 +0100 From: Eric Dumazet <eric.dumazet@...il.com> To: Julian Anastasov <ja@....bg> Cc: David Miller <davem@...emloft.net>, Vijay Subramanian <subramanian.vijay@...il.com>, netdev@...r.kernel.org, ncardwell@...gle.com, Venkat Venkatsubra <venkat.x.venkatsubra@...cle.com>, Elliott Hughes <enh@...gle.com>, Yuchung Cheng <ycheng@...gle.com> Subject: Re: [PATCH net-next] tcp: better retrans tracking for defer-accept On Sun, 2012-10-28 at 01:29 +0300, Julian Anastasov wrote: > Hello, > > On Sat, 27 Oct 2012, Eric Dumazet wrote: > > > From: Eric Dumazet <edumazet@...gle.com> > > > > For passive TCP connections using TCP_DEFER_ACCEPT facility, > > we incorrectly increment req->retrans each time timeout triggers > > while no SYNACK is sent. > > > > SYNACK are not sent for TCP_DEFER_ACCEPT that were established (for wich > > we received the ACK from client). Only the last SYNACK is > > sent so that we can receive again an ACK from client, to move the > > req into accept queue. We plan to change this later to avoid > > the useless retransmit (and potential problem as this SYNACK could be > > lost) > > > > TCP_INFO later gives wrong information to user, claiming imaginary > > retransmits. > > > > Decouple req->retrans field into two independent fields : > > > > num_retrans : number of retransmit > > num_timeout : number of timeouts > > > > num_timeout is the counter that is incremented at each timeout, > > regardless of actual SYNACK being sent or not, and used to > > compute the exponential timeout. > > > > Introduce inet_rtx_syn_ack() helper to increment num_retrans > > only if ->rtx_syn_ack() succeeded. > > > > Use inet_rtx_syn_ack() from tcp_check_req() to increment num_retrans > > when we re-send a SYNACK in answer to a (retransmitted) SYN. > > Prior to this patch, we were not counting these retransmits. > > > > Change tcp_v[46]_rtx_synack() to increment TCP_MIB_RETRANSSEGS > > only if a synack packet was successfully queued. > > > > Reported-by: Yuchung Cheng <ycheng@...gle.com> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com> > > Cc: Julian Anastasov <ja@....bg> > > Cc: Vijay Subramanian <subramanian.vijay@...il.com> > > Cc: Elliott Hughes <enh@...gle.com> > > Cc: Neal Cardwell <ncardwell@...gle.com> > > --- > > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c > > index ba48e79..b236ef0 100644 > > --- a/net/ipv4/syncookies.c > > +++ b/net/ipv4/syncookies.c > > @@ -340,7 +340,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb, > > } > > > > req->expires = 0UL; > > - req->retrans = 0; > > + req->num_retrans = 0; > > also req->num_timeout = 0; ? This is not needed here. We construct a temporary req only to be able to use get_cookie_sock() and build a full socket with : icsk->icsk_af_ops->syn_recv_sock(sk, skb, req, dst); By definition this req wont ever be used in SYN_RECV state, and never be part of the timer wheel. We init req->num_retrans to 0 because this field will be the source of newtp->total_retrans = req->num_retrans; in tcp_v{4|6}_syn_recv_sock() > > > > > /* > > * We need to lookup the route here to get at the correct > > > diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c > > index 182ab9a..4016197 100644 > > --- a/net/ipv6/syncookies.c > > +++ b/net/ipv6/syncookies.c > > @@ -214,7 +214,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb) > > ireq6->iif = inet6_iif(skb); > > > > req->expires = 0UL; > > - req->retrans = 0; > > + req->num_retrans = 0; > > also req->num_timeout = 0; ? > Same thing : this is not needed > > ireq->ecn_ok = ecn_ok; > > ireq->snd_wscale = tcp_opt.snd_wscale; > > ireq->sack_ok = tcp_opt.sack_ok; > > > @@ -1866,7 +1869,7 @@ static void get_openreq6(struct seq_file *seq, > > 0,0, /* could print option size, but that is af dependent. */ > > 1, /* timers active (only the expire timer) */ > > jiffies_to_clock_t(ttd), > > - req->retrans, > > + req->num_timeout, > > num_timeout already noted by Neal > Yes, thanks. source patch was fine, I messed the rebase. > > from_kuid_munged(seq_user_ns(seq), uid), > > 0, /* non standard timer */ > > 0, /* open_requests have no inode */ > > I don't see any problems with such patch, just > check if it is for correct tree, I see some atomic operations > with qlen_young in patch. > Arg, my net-next tree got some stuff I forgot to revert, sorry, will cleanup. > One thing to finally decide: should we use limit for > retransmissions or for timeout, is the following better?: > > if (!rskq_defer_accept) { > *expire = req->num_retrans >= thresh; > ^^^^^^^^^^^ > *resend = 1; > return; > } Not sure it matters and if this decision is part of this patch. If a retransmit fails, it seems we zap the request anyway ? inet_rtx_syn_ack() returns an error and inet_rsk(req)->acked is false -> we remove the req from queue. We dont remove the req only if we got a listen queue overflow in tcp_check_req() : we set acked to 1 in this case. listen_overflow: if (!sysctl_tcp_abort_on_overflow) { inet_rsk(req)->acked = 1; return NULL; } Using number of timeouts seems better to me. There is no point holding a req forever if we fail to retransmit SYNACKS. Client probably gave up. Thanks a lot for the review ! -- 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