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: <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
Powered by blists - more mailing lists