[Click] a bug in tcprewriter.cc:apply_sack() and/or tcprewriter.cc:apply()?

[Eddie Kohler]

Hi Weidong,

Well, poo.  Maybe this is the problem other people have reported.   
You've done more detail in tracking it down, though; thanks!

(1) Do you have a trace file + config that can reliably replicate the  
problem?
(2) Do you know what the value of 'len' was in   
TCPRewriter::apply_sack()?  I wonder if it was less than 0.  That  
would cause some serious issues.

Would love your further aid.

Eddie


On Sep 29, 2005, at 11:55 AM, Weidong Cui wrote:

> Hi There,
>
> I think there is a bug in apply_sack() in tcprewriter.cc though I  
> couldn't figure out the fix.  Here is the story.
>
> 1. I used a downstream tcprewriter to manipulate TCP sequence  
> numbers for a TCP proxy module.  I manually assign the seqno_delta  
> for the reverse mapping in the TCP proxy code:
>
>         // update seqno_delta in tcprewriter
>         if (TCPRewriter::TCPMapping *p_mapping = _tcp_rewriter- 
> >get_mapping (IP_PROTO_TCP, tcp_flow)) {
>             tcp_state->_server_syn_seqno = ntohl (tcph->th_seq);
>             int32_t delta;
>             if (tcp_state->_filter_syn_seqno >= tcp_state- 
> >_server_syn_seqno)
>                 delta = tcp_state->_filter_syn_seqno - tcp_state- 
> >_server_syn_seqno;
>             else
>                 delta = -1 * (tcp_state->_server_syn_seqno -  
> tcp_state->_filter_syn_seqno);
>             p_mapping->update_seqno_delta (tcp_state- 
> >_server_syn_seqno, delta);
>         }
>
> 2. Randomly (in the sense of running time, but in all three runs)  
> Click fell into an infinite loop (used 99% CPU while normally it  
> was around 10%).  After stopping it in gdb, I found Click stopped  
> at the line of "break" in tcprewriter.cc:apply_sack():
>
>     // finish off csum_delta calculation
>     for (uint16_t *csum = csum_begin; reinterpret_cast<uint8_t *> 
> (csum) < end_sack; csum++)
>         csum_delta += *csum;
>     break;
>
> 3. I found that there is condition check on if there is a  
> reverse_seqno_delta in tcprewriter.cc:apply() before calling  
> tpcrewriter.cc:apply_sack():
>
>     // update SACK sequence numbers
>     if ((tcph->th_off > 8
>      || (tcph->th_off == 8
>          && *(reinterpret_cast<const uint32_t *>(tcph + 1)) != htonl 
> (0x0101080A)))
>     && reverse()->have_seqno_delta())
>     csum_delta += reverse()->apply_sack(tcph, p->transport_length());
>
> This made me think that it's because I manually added  
> reverse_seqno_delta.  Also there shouldn't be no SACK in all the  
> TCP connections seen by the tcprewriter because the TCP proxy  
> refuses all TCP options during three-way handshakes before anything  
> reaches the tcprewriter.
>
> 4. I commented off the above lines in tcprewriter.cc:apply() and  
> the problem was gone.
>
> Please let me know if you have any questions.
>
> Thanks,
> Weidong
>
> -- 
> _____________________________________________________
> Weidong Cui
> Ph.D. Candidate      wdc at EECS.Berkeley.EDU
> EECS, UC Berkeley    http://www.cs.berkeley.edu/~wdc/
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>