netdev - Re: [PATCH net-next] tcp: use an RB tree for ooo receive queue

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 08 Sep 2016 06:38:21 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Yaogong Wang <wygivan@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>
Subject: Re: [PATCH net-next] tcp: use an RB tree for ooo receive queue

On Thu, 2016-09-08 at 14:02 +0300, Ilpo Järvinen wrote:
> On Wed, 7 Sep 2016, Eric Dumazet wrote:

> While testing, was there any check done for the data that was delivered
> in order to ensure that no corruption occured (either by you or Yaogong)? 
> ...This kind of changes have some potential to cause some corruption to
> the stream content and it would be nice to be sure there wasn't any
> accidents.

Sure, we did tests on real GFE, serving real data to users.

My ssh/scp sessions did not catch any error, even with up to 10% packet
losses.

> >  	}
> >  
> > -	seq = TCP_SKB_CB(skb)->seq;
> > -	end_seq = TCP_SKB_CB(skb)->end_seq;
> 
> I hate to nitpick but moving these variables earlier and the 
> TCP_SKB_CB(skb)->seq/end_seq simplifications seem unrelated and
> could be done in another patch?

You are right.

I could split this patch in 3 parts if David requests it.

1) One patch adding skb_rbtree_purge(struct rb_root *root) helper
2) One patch doing this seq/end_seq cleanup.
3) RB patch


> > -
> > -	for (;;) {
> > -		struct sk_buff *next = NULL;
> >  
> > -		if (!skb_queue_is_last(&tp->out_of_order_queue, skb))
> > -			next = skb_queue_next(&tp->out_of_order_queue, skb);
> > -		skb = next;
> > +	for (head = skb;;) {
> > +		skb = tcp_skb_next(skb, NULL);
> >  
> > -		/* Segment is terminated when we see gap or when
> > -		 * we are at the end of all the queue. */
> > +		/* Range is terminated when we see a gap or when
> > +		 * we are at the queue end.
> > +		 */
> >  		if (!skb ||
> >  		    after(TCP_SKB_CB(skb)->seq, end) ||
> >  		    before(TCP_SKB_CB(skb)->end_seq, start)) {
> > -			tcp_collapse(sk, &tp->out_of_order_queue,
> > +			tcp_collapse(sk, NULL, &tp->out_of_order_queue,
> >  				     head, skb, start, end);
> > -			head = skb;
> > -			if (!skb)
> > -				break;
> > -			/* Start new segment */
> > +			goto new_range;
> > +		}
> > +
> > +		if (unlikely(before(TCP_SKB_CB(skb)->seq, start)))
> >  			start = TCP_SKB_CB(skb)->seq;
> > +		if (after(TCP_SKB_CB(skb)->end_seq, end))
> >  			end = TCP_SKB_CB(skb)->end_seq;
> > -		} else {
> > -			if (before(TCP_SKB_CB(skb)->seq, start))
> > -				start = TCP_SKB_CB(skb)->seq;
> > -			if (after(TCP_SKB_CB(skb)->end_seq, end))
> > -				end = TCP_SKB_CB(skb)->end_seq;
> > -		}
> >  	}
> >  }
> 
> I tried long to think if I could propose alternative layout which would 
> make this function to exit from the end but couldn't come up anything 
> sensible. As is, it's always exiting within that top if block which is
> somewhat unintuitive :-).
> 
> Acked-By: Ilpo Järvinen <ilpo.jarvinen@...sinki>
> 

Thanks for the review !