netdev - Re: [PATCH net-next V2 1/1] tcp: Prevent needless syn-ack rexmt during TWHS

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 for Android: free password hash cracker in your pocket

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

Date:	Sat, 27 Oct 2012 16:23:05 +0300 (EEST)
From:	Julian Anastasov <ja@....bg>
To:	Eric Dumazet <eric.dumazet@...il.com>
cc:	Vijay Subramanian <subramanian.vijay@...il.com>,
	netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	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 V2 1/1] tcp: Prevent needless syn-ack rexmt
 during TWHS


	Hello,

On Sat, 27 Oct 2012, Eric Dumazet wrote:

> > @@ -598,9 +598,8 @@ void inet_csk_reqsk_queue_prune(struct sock *parent,
> >  					       &expire, &resend);
> >  				req->rsk_ops->syn_ack_timeout(parent, req);
> >  				if (!expire &&
> > -				    (!resend ||
> > -				     !req->rsk_ops->rtx_syn_ack(parent, req, NULL) ||
> > -				     inet_rsk(req)->acked)) {
> > +				    (!resend || inet_rsk(req)->acked ||
> > +				     !req->rsk_ops->rtx_syn_ack(parent, req, NULL))) {
> >  					unsigned long timeo;
> >  
> >  					if (req->retrans++ == 0)
> 
> 
> Part of the complexity of this is that req->retrans is the number of
> timeouts, serving as the exponential backoff base.
> 
> Unfortunately we have a side effect because number of retransmits is
> wrong for defer accept.
> 
> Here is what I suggest : upstream to net-next this patch we use at
> Google :
> 
> Author: Eric Dumazet <edumazet@...gle.com>
> Date:   Tue Oct 2 02:21:12 2012 -0700
> 
> net-tcp: better retrans tracking for defer-accept
> 
> For passive TCP connections using TCP_DEFER_ACCEPT facility,
> we incorrectly increment req->retrans each time timeout triggers
> while no SYNACK is sent.
> 
> Decouple req->retrans field into two fields :
> 
> num_retrans : number of retransmit
> num_timeout : number of timeouts
> 
> (retrans was renamed to make sure we didnt miss an occurrence)
> 
> introduce inet_rtx_syn_ack() helper to increment num_retrans
> only if ->rtx_syn_ack() succeeded.

	This is dangerous, the first of the cases is route
failure, what if we just added reject route for some attacker?
We will get error forever. May be it is difficult to decide
which error should change the counter. IMHO, such reliability
is not needed, we can be short of memory too.

> Use inet_rtx_syn_ack() from tcp_check_req() to increment num_retrans
> when we re-send a SYNACK in answer to a SYN. Prior to this patch,
> we were not counting these retransmits.

	Such change looks correct. Of course, it has
side effect on current TCP_DEFER_ACCEPT calculations but
it is a TCP_DEFER_ACCEPT implementation problem.

> Change tcp_v[46]_rtx_synack() to increment TCP_MIB_RETRANSSEGS
> only if a synack packet was successfuly queued.

Regards

--
Julian Anastasov <ja@....bg>
--
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