Fri 08 Jan 2010 03:11:21 PM UTC, comment #17:
Closing this as it doesn't seem too important (for an extra bugfix release).
|
Sun 27 Dec 2009 11:20:22 AM UTC, comment #16:
Unfortunately, there's a bug in the patch from comment #14 which has made it into the 1.3.2 release:
In tcp_in.c:tcp_receive(), line 1210 (and 1217 for the assertion),
next->tcphdr->seqno
must be used instead of
pcb->ooseq->tcphdr->seqno
One could argue if this happens often (incoming segment being greater than the first on ooseq and partly overlapping the second on ooseq) as most often, retransmitted segments will be the same as before, so I don't know how serious this bug is.
Still, running the corresponding unit test once showed that there was a bug...
Oh, and about that "a bit hard to follow what it's changing and why": I'll add some comments there, I think that's easier to read than the graphics.
I'll check it in but leave this open to discuss what to do about this bug regarding the release.
|
Thu 24 Dec 2009 03:30:42 PM UTC, comment #15:
I'm committing both these patches (with a few modifications, references to next->mpTcpHdr for example), although I must confess the second one is a bit hard to follow what it's changing and why.
Resolving this bug fixed.
|
Tue 15 Dec 2009 01:11:59 PM UTC, comment #14:
2nd patch for processing oos queue after receiving in-sequence segment
(Now in-sequence segment doesn't delete segments from oos queue)
(file #19272)
|
Fri 11 Dec 2009 01:15:29 PM UTC, comment #13:
Oh, that makes sense... in that case, the check should be included or we lose the FIN - it might be retransmitted, but that's inefficient.
In any case: the current code doesn't have this problem. I'd still check this in now, I've also locally adjusted the tests to verify it works.
|
Fri 11 Dec 2009 11:18:34 AM UTC, comment #12:
Ahh, now I understand. The length being used in the comparison (cseg->len) is the payload length, not the sequence length, and so does not include any FIN bits.
Oleg is right, we do therefore need to check whether there is a FIN byte in the old packet before discarding it, and move that byte to the new packet if the new packet does not also have a FIN.
|
Fri 11 Dec 2009 11:03:28 AM UTC, comment #11:
It is another question, what should we do when we have data behind the FIN,
but I mean here length of data
....45 - length of data 2 + FIN flag
...345 - length of data 3, but without FIN flag
tcp length of both packets is 3,
but seqno of 1st is 4 and 2nd is 3.
In this case we doesn't have contradiction.
|
Fri 11 Dec 2009 10:52:12 AM UTC, comment #10:
The other end is wrong and we can do what we like. Seqno 5 in the old packet was a FIN, seqno 5 in the new packet is not a FIN. Which is correct? We should probably reset the connection in this case.
|
Fri 11 Dec 2009 10:45:34 AM UTC, comment #9:
May be I have made not correct description
old -
seqno = 4
len = 2
flag = FIN
new -
seqno = 3
len = 3
result -
seqno = 3
len = 3
flag = FIN
|
Fri 11 Dec 2009 10:36:29 AM UTC, comment #8:
The FIN uses a byte of sequence space, so if two packets finish on the same sequence byte and one has a FIN and the other does not, that is wrong. I'm happy with the change as Simon has made it.
|
Fri 11 Dec 2009 10:30:32 AM UTC, comment #7:
You are right in 4th comment.
It should be
TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) | TCP_FIN);
All cases are not covered with 1st line
if (TCPH_FLAGS(cseg->tcphdr) & TCP_FIN) {
This is neccesary for case when old segment has FIN flag but
new one - not.
e.g.
old - (4-5)FIN
new - (3-5)
result (3-5)FIN
|
Thu 10 Dec 2009 05:37:49 PM UTC, comment #6:
After all, the extra FIN check is not at all necessary, since that case should already be covered by the first line in tcp_oos_insert_segment():
if (TCPH_FLAGS(cseg->tcphdr) & TCP_FIN) {
I've extended the ooseq unit test and it shows that the suggested patch works: data is delivered in correct order and the number of segments on ooseq is sometimes smaller than without the patch.
|
Thu 10 Dec 2009 03:51:09 PM UTC, comment #5:
Anyway, I would have expected it to work without the extra check for the FIN flag as that should already be covered by comparing the sequence numbers.
|
Thu 10 Dec 2009 02:27:52 PM UTC, comment #4:
That's true, I would have expected somthing like:
TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) | TCP_FIN);
|
Thu 10 Dec 2009 10:46:25 AM UTC, comment #3:
I can see what you mean with the GT -> GEQ change, but I don't understand that FIN change:
+ /* cseg with FIN already processed */
+ if (TCPH_FLAGS(next->mpTcpHdr) & TCP_FIN) {
+ TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) & ~TCP_FIN);
+ }
This reads to me as:
if (old packet has FIN set) {
remove FIN from new packet's flags;
}
Can you explain why you'd want to do that?
|
Thu 10 Dec 2009 10:04:26 AM UTC, comment #2:
Suppuse we have rcv_nxt==0
1st fragment
seqno=2
len=2
(2-3)
It is queued into oos.
2nd fragment
seqno=1
len=3
(1-3)
Without change we have on oos queue two fragments
(1-1)(2-3)
with change only
(1-3)
|
Wed 09 Dec 2009 07:36:11 PM UTC, comment #1:
> In function tcp_oos_insert_segment()
> some segments covered with new segment are deleted.
I thought that was what we wanted to do with our last changes? Can you go into detail a bit more on what's bad here?
|
Wed 09 Dec 2009 03:36:48 PM UTC, original submission:
In function tcp_oos_insert_segment()
some segments covered with new segment are deleted.
It would be better instead of
while (next &&
TCP_SEQ_GT((seqno + cseg->len),
(next->tcphdr->seqno + next->len))) {
struct tcp_seg *old_seg = next;
next = next->next;
tcp_seg_free(old_seg);
}
write
while (next &&
- TCP_SEQ_GT((seqno + cseg->len),
+ TCP_SEQ_GEQ((seqno + cseg->len),
(next->tcphdr->seqno + next->len))) {
struct tcp_seg *old_seg = next;
next = next->next;
tcp_seg_free(old_seg);
}
or if we don't want to lose FIN flag
while (next &&
- TCP_SEQ_GT((seqno + cseg->len),
+ TCP_SEQ_GEQ((seqno + cseg->len),
(next->tcphdr->seqno + next->len))) {
+ /* cseg with FIN already processed */
+ if (TCPH_FLAGS(next->mpTcpHdr) & TCP_FIN) {
+ TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) & ~TCP_FIN);
+ }
struct tcp_seg *old_seg = next;
next = next->next;
tcp_seg_free(old_seg);
}
|