buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26251, RST process in TIME_WAIT TCP state

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #26251: RST process in TIME_WAIT TCP state

Submitter:  Oleg Tyshev <olegreen>
Submitted:  Mon 20 Apr 2009 11:16:04 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Sun 25 Oct 2009 11:45:37 AM UTC, comment #15: 

Checked in like in last comment.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 23 Oct 2009 01:46:40 PM UTC, comment #14: 

A relatively complete implementation of RFC 1337 vs. RFC 793 (chapter 3.9 Event Processing - Segment Arrives) is below. This is relatively complete (the 'acceptable' check is not fully made - only for SYN - but I guess it's OK like that), but the code is also biger than before:

/* RFC 1337: in TIME_WAIT, ignore RST and ACK FINs + any 'acceptable' segments */
/* RFC 793 3.9 Event Processing - Segment Arrives:

  • - first check sequence number - we skip that one in TIME_WAIT (always
  •   acceptable since we only send ACKs)
  • - second check the RST bit (... return) */

if (flags & TCP_RST)  {
  return ERR_OK;
}
/* - fourth, check the SYN bit, */
if (flags & TCP_SYN) {
  /* If an incoming segment is not acceptable, an acknowledgment
     should be sent in reply */
  if (TCP_SEQ_BETWEEN(seqno, pcb->rcv_nxt, pcb->rcv_nxt+pcb->rcv_wnd)) {
    /* If the SYN is in the window it is an error, send a reset */
    /* @todo: send RST */
    tcp_rst(ackno, seqno + tcplen, &(iphdr->dest), &(iphdr->src),
      tcphdr->dest, tcphdr->src);
    return ERR_OK;
  }
} else if (flags & TCP_FIN) {
  /* - eighth, check the FIN bit: Remain in the TIME-WAIT state.
       Restart the 2 MSL time-wait timeout.*/
  pcb->tmr = tcp_ticks;
}

if ((tcplen > 0))  {
  /* Acknowledge data, FIN or out-of-window SYN */
  pcb->flags |= TF_ACK_NOW;
  return tcp_output(pcb);
}
return ERR_OK;

Simon Goldschmidt <goldsimon>
Group administrator
Fri 23 Oct 2009 10:08:28 AM UTC, comment #13: 

RFC 793 - I meant the same page that you mentioned.

Actually window in TIME_WAIT state already fast closed.
Acceptable only FIN retransmits.
Old segments will be ACKed.
I understood RFC that SYN is out of window and
"If the SYN is not in the window this step would not be reached
and an ack would have been sent in the first step (sequence
number check)."

Now I am not sure, what is better ACK, RST or noting.
I'll try to see other implementations of TCP.

Re: global variables comment #10
I don't like current state with global variables too.
Global variables should have some prefix e.g. "g" or
to be joined in one structure otherwise it is difficult to understand scope.

Oleg Tyshev <olegreen>
Fri 23 Oct 2009 07:07:14 AM UTC, comment #12: 

I'd rather make it

if (((flags & (TCP_RST | TCP_SYN)) == 0) && (tcplen > 0))  {

because I thought ACKing SYNs in TIME_WAIT wasn't OK. RFC 793 says we shall sent a RST if the SYN is in the window and go to CLOSED. However that's what RFC 1337 wants to avoid so I'd ignore the SYN (or should we send a RST and stay in TIME_WAIT?). A SYN is outside the window wouldn't happen in this state... so I'd ignore it, too.

See this excerpt from RFC 793, page 70:

>>> snip >>>

fourth, check the SYN bit,

      SYN-RECEIVED
      ESTABLISHED STATE
      FIN-WAIT STATE-1
      FIN-WAIT STATE-2
      CLOSE-WAIT STATE
      CLOSING STATE
      LAST-ACK STATE
      TIME-WAIT STATE

        If the SYN is in the window it is an error, send a reset, any
        outstanding RECEIVEs and SEND should receive "reset" responses,
        all segment queues should be flushed, the user should also
        receive an unsolicited general "connection reset" signal, enter
        the CLOSED state, delete the TCB, and return.

        If the SYN is not in the window this step would not be reached
        and an ack would have been sent in the first step (sequence
        number check).
<<< snap <<<


(BTW: I don't see anything regarding SYN in TIME-WAIT on page 71 ??)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 22 Oct 2009 05:10:48 PM UTC, comment #11: 

Ooops

tcplen already contains length of FIN and SYN:
tcplen = p->tot_len + ((flags & (TCP_FIN | TCP_SYN)) ? 1 : 0);

So will be better

/* RFC 1337: in TIME_WAIT, ignore RST and ACK FINs + any 'acceptable' segments */
if (((flags & TCP_RST) == 0) && (tcplen > 0)) {
pcb->flags |= TF_ACK_NOW;
return tcp_output(pcb);
}
return ERR_OK;

ACK to the SYN would be acceptable according to RFC 793 too
(page 71)

Oleg Tyshev <olegreen>
Thu 22 Oct 2009 04:30:13 PM UTC, comment #10: 

Thanks for the info. I still can't get used to these global variables in lwIP when I have to avoid global variables at work ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 22 Oct 2009 04:00:22 PM UTC, comment #9: 

One note:

we have in file tcp_in.c static variable "flags".
It is global for scope of the file.
I don't like name convention, but now we can use global flags instead of local.

So, line
u16_t flags = TCPH_FLAGS(tcphdr);
can be deleted.

Oleg Tyshev <olegreen>
Thu 22 Oct 2009 01:37:48 PM UTC, comment #8: 


> Planned Release:                   1.4.0 => 1.3.2


I take that as an agreement to the code in comment #7 :-)
-> checked in.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Oct 2009 03:57:46 PM UTC, comment #7: 

Given the last few comments, the new body of tcp_timewait_input() would be:

u16_t flags = TCPH_FLAGS(tcphdr);
/* RFC 1337: in TIME_WAIT, ignore RST and ACK FINs + any 'acceptable' segments */
if (((flags & TCP_RST) == 0) && ((flags & TCP_FIN) || (tcplen > 0)))  {
  pcb->flags |= TF_ACK_NOW;
  return tcp_output(pcb);
}
return ERR_OK;

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Oct 2009 05:23:03 PM UTC, comment #6: 


>- send RST on SYN,

I don't think that it is good idea to send RST to SYN.
Fast in each RFC describing SYN flood is not advised to reply with SYN if it is not possible to process it.

>- ACK a FIN and reset pcb->tmr
>- ACK acceptable seqno (unless RST set)

Yes, we should ACK any acceptable seqno.
But which seqno is acceptable? Any?

>accepting SYNs in TIME_WAIT as described in RFC 1122

It exists many variants of generation initial sequence number and in general this number is random.

So it is impossible to identify "initial sequence number for the new connection to be larger than the largest sequence number it used on the previous connection incarnation"

Oleg Tyshev <olegreen>
Mon 19 Oct 2009 04:15:05 PM UTC, comment #5: 

Oh, and while we're at it: we could think about accepting SYNs in TIME_WAIT as described in RFC 1122...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Oct 2009 04:09:39 PM UTC, comment #4: 

According RFC 793 (although that is somewhat outdated), there is more to do in TIME_WAIT, we should also

- send RST on SYN,
- ACK a FIN and reset pcb->tmr
- ACK acceptable seqno (unless RST set)

Thus leaving pcb->rcv_nxt as it is might make sense, since we already have received all data we expect.

Not calling tcp_output() twice is also a good idea :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Oct 2009 02:36:51 PM UTC, comment #3: 

OK, with RST agree with you:
ignore means send nothing.

Now each data packet in TIME_WAIT state will be acknowledged.
tcp_timewait_input() -> tcp_output()

In tcp_output()

if (pcb->flags & TF_ACK_NOW &&
    (seg == NULL || ... ) {
   ....
   tcphdr->ackno = htonl(pcb->rcv_nxt);
   ....
 ip_output(p ...);
 return ERR_OK;
}

In TIME_WAIT we already received all data and seems to me rcv_nxt should not be changed.

---

Some general notes about tcp_timewait_input() function:

We have following implementation for tcp_ack_now
#define tcp_ack_now(pcb) (pcb)->flags |= TF_ACK_NOW; \
                         tcp_output(pcb)
It means for (tcplen > 0)
 function tcp_output would be called twice.
It is not bad, but we can avoid it.

  if (tcplen > 0) {
-    tcp_ack_now(pcb);
+    pcb->flags |= TF_ACK_NOW;
  }
  return tcp_output(pcb);

Oleg Tyshev <olegreen>
Sun 18 Oct 2009 08:47:09 AM UTC, comment #2: 

Hmm, RFC1337 recommends to ignore RST segments in TIME-WAIT state, I doubt that this means sending an ACK for this.

I guess to implement that recommendation, we should add this code at the start of tcp_timewait_input():

if((TCPH_FLAGS(tcphdr) & TCP_RST) {
  /* RFC 1337: ignore RST segments in TIME_WAIT */
  return ERR_OK;
}

Also, I think this can be targeted to 1.3.2.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 24 Apr 2009 02:04:47 PM UTC, comment #1: 

Sorry, for TIME_WAIT exists tcp_timewait_input() and as reply to RST would be sent ACK without close pcb.
It follows recommendation of RFC 1337.

But why we have following lines in tcp_timewait_input()
I doesn't undestand.
if (TCP_SEQ_GT(seqno + tcplen, pcb->rcv_nxt)) {
  pcb->rcv_nxt = seqno + tcplen;
}

It is not check of the window.
Random packet with probability 50% changes rcv_nxt.
Where will be used rcv_nxt later?

Oleg Tyshev <olegreen>
Mon 20 Apr 2009 11:16:04 AM UTC, original submission:  

Last week I saw RFC 1337 "TIME-WAIT Assassination Hazards in TCP".
As a simplest fix for TWA hazard in capital 4 was suggested following:

(F1) Ignore RST segments in TIME-WAIT state.

If the 2 minute MSL is enforced, this fix avoids all three hazards.

This is the simplest fix.  One could also argue that it is formally the correct thing to do; since allowing time for old duplicate segments to die is one of TIME-WAIT state's functions, the state should not be truncated by a RST segment.

May be we can filter RST segments in TIME-WAIT state (function tcp_process())?

Oleg Tyshev <olegreen>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Updated the item)
  • -email is unavailable- added by olegreen (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-10-25 goldsimon Open/ClosedOpen Closed
        StatusIn Progress Fixed
    2009-10-23 goldsimon StatusFixed In Progress
        Open/ClosedClosed Open
    2009-10-22 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-10-22 kieranm Planned Release1.4.0 1.3.2
    2009-04-21 kieranm Planned Release 1.4.0

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code