Sun 25 Oct 2009 11:45:37 AM UTC, comment #15:
Checked in like in last comment.
|
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;
|
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.
|
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 ??)
|
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)
|
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 ;-)
|
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.
|
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.
|
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;
|
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"
|
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...
|
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 :-)
|
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);
|
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.
|
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?
|
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())?
|