netdev - Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection

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]

Message-Id: <20071007.145100.00679926.takano@axe-inc.co.jp>
Date:	Sun, 07 Oct 2007 14:51:00 +0900 (JST)
From:	TAKANO Ryousei <takano@...-inc.co.jp>
To:	ilpo.jarvinen@...sinki.fi
Cc:	netdev@...r.kernel.org, y-kodama@...t.go.jp
Subject: Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection

From: "Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
Subject: Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
Date: Fri, 5 Oct 2007 13:02:07 +0300 (EEST)

> On Thu, 4 Oct 2007, TAKANO Ryousei wrote:
> 
> > This patch allows to detect loss of retransmitted packets more
> > accurately by using the highest end sequence number among SACK 
> > blocks.  Before the retransmission queue is scanned, the highest
> > end sequence number (high_end_seq) is retrieved, and this value
> > is compared with the ack_seq of each packet.
> > 
> > Signed-off-by: Ryousei Takano <takano-ryousei@...t.go.jp>
> > Signed-off-by: Yuetsu Kodama <y-kodama@...t.go.jp>
> > ---
> >  net/ipv4/tcp_input.c |   14 +++++++++++---
> >  1 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index bbad2cd..12db4b3 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -978,6 +978,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
> >  	int cached_fack_count;
> >  	int i;
> >  	int first_sack_index;
> > +	__u32 high_end_seq;
> 
> No __-types when not visible to userspace please.
> 
I will fix it.

> >
> >  	if (!tp->sacked_out)
> >  		tp->fackets_out = 0;
> > @@ -1012,6 +1013,14 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
> >  	if (before(TCP_SKB_CB(ack_skb)->ack_seq, prior_snd_una - tp->max_window))
> >  		return 0;
> >  
> > +	/* Retrieve the highest end_seq among SACK blocks. */
> > +	high_end_seq = ntohl(sp[0].end_seq);
> > +	for (i = 1; i < num_sacks; i++) {
> > +		__u32 end_seq = ntohl(sp[i].end_seq);
> > +		if (after(end_seq, high_end_seq))
> > +			high_end_seq = end_seq;
> > +	}
> > +
> 
> There's one problem... Net-2.6.24 tree includes SACK block validator 
> which is being done in the marking loop. The SACK blocks would not yet be 
> validated in that position, yet this code should be protected by the 
> validation! My intention is to move the validator earlier anyway (yet to 
> be split into smaller logical patches), see:
> 
> http://marc.info/?l=linux-netdev&m=119062989408053&w=2
> 
I will check the net-2.6.24 tree and your patch.

> >  	/* SACK fastpath:
> >  	 * if the only SACK change is the increase of the end_seq of
> >  	 * the first block then only apply that SACK block
> > @@ -1161,9 +1170,8 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
> >  			}
> >  
> >  			if ((sacked&TCPCB_SACKED_RETRANS) &&
> > -			    after(end_seq, TCP_SKB_CB(skb)->ack_seq) &&
> > -			    (!lost_retrans || after(end_seq, lost_retrans)))
> > -				lost_retrans = end_seq;
> > +			    after(high_end_seq, TCP_SKB_CB(skb)->ack_seq))
> > +				lost_retrans = high_end_seq;
> 
> Just couple of thoughts, not that this change itself is incorrect...
> 
> In case sacktag uses fastpath, this code won't be executed for the skb's 
> that we would like to check (those with SACKED_RETRANS set, that are 
> below the fastpath_skb_hint). We will eventually deal with the whole queue 
> when fastpath_skb_hint gets set to NULL, with the next cumulative ACK that 
> fully ACKs an skb at the latest. Maybe there's a need for a larger surgery 
> than this to fix it. I think we need additional field to tcp_sock to avoid 
> doing a full-walk per ACK:
> 
I think the problem occurs in slowpath. For example, in case when the receiver
detects and sends back a new SACK block, the sender may fail to detect loss
of a retransmitted packet.

> Keep minimum of TCP_SKB_CB(skb)->ack_seq of rexmitted segments in 
> tcp_sock, when that's exceeded by SACK block, do a full-walk in the 
> lost_retrans worker loop like the old code does...
> 
> 
> In future, please base your work to current development tree instead of 
> linus' tree (net-2.6.24 at this point of time, there's also tcp-2.6 but 
> it's currently a bit outdated).
> 
Thanks for your suggestion. SACK processing has a heavy workload and 
it is complex. I agree to make efforts toward a more generic solution. 
Your recv_sack_cache patch seems valuable. I will continue to work in 
the net-2.6.24 tree, and resend our patches. 
Anyway, first of all, I would like to share this problem with kernel 
developers.

BTW, what is difference among netdev-2.6, net-2.6 (net-2.6.24), and 
tcp-2.6? I am not familiar with linux kernel development process.

> 
> -- 
>  i.

Regards,
Ryousei Takano
-
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