Sun 29 Nov 2009 01:25:09 PM UTC, comment #17:
I reviewed the patch and adapted the out-of-sequence unit tests to cover nearly all the out-of-sequence code.
After these are working successfully (found another bug there where segments were freed but pbufs weren't), I've checked in the patch.
Kieran, I hope you don't mind me taking over here (because of comment #14) :-)
|
Fri 27 Nov 2009 08:05:25 AM UTC, comment #16:
The unit tests I just checked in seem to confirm the patch works (although not all clauses of the code addin ooseq segments are covered, yet).
I would convert the new function to static though, to give the compiler room for improvements.
|
Mon 23 Nov 2009 05:40:45 PM UTC, comment #15:
Patch again corrected.
It was error in processing last segment.
if (next->next == NULL &&
TCP_SEQ_GT(seqno, next->tcphdr->seqno)) {
+ if (TCPH_FLAGS(next->tcphdr) & TCP_FIN) {
+ /* segment "next" already contains all data */
+ break;
+ }
(file #19118)
|
Mon 23 Nov 2009 03:11:15 PM UTC, comment #14:
I'll take a look at the patch, and if I think it's safe I'll try and get it in to 1.3.2
|
Mon 23 Nov 2009 03:08:03 PM UTC, comment #13:
The patch looks good to me. I also like the separation into smaller functions, it would do the rest of the code good, too, since it gets more readable. And at least MSVC and gcc seem to inline static functions quite well, which means that code size isn't an argument here.
However, I'm not in a position to test it and a "should work" might not be enough at this stage of release, so we might want to delay this until after the 1.3.2 release...?
|
Mon 23 Nov 2009 01:48:03 PM UTC, comment #12:
In this patch common part for insertion packet into
oos queue is moved into separate function.
I have tested this patch with FIN packets.
With other packets it should work too.
(file #19117)
|
Fri 20 Nov 2009 06:27:45 PM UTC, comment #11:
There are 4 almost the same come fragments:
1) for ackno ==
2) prev == NULL
3) prev != NULL
4) next->next == NULL
I'll try to simplify it.
|
Fri 20 Nov 2009 05:55:14 PM UTC, comment #10:
:)Patch
(file #19095)
|
Fri 20 Nov 2009 05:54:35 PM UTC, comment #9:
Sorry, I've made a lot of errors in deallocation.
There is corrected patch.
I will test it, but with slightly changed sources.
(I have other code rules)
|
Fri 20 Nov 2009 05:05:00 PM UTC, comment #8:
Selected lines do this work
If first segment overlap completely 2nd, 3rd, etc.,
they will be deleted.
> /*if (TCP_SEQ_LT(prev->tcphdr->seqno, seqno) &&
> TCP_SEQ_LT(seqno, next->tcphdr->seqno)) {*/
> if(TCP_SEQ_BETWEEN(seqno, prev->tcphdr->seqno+1, next->tcphdr->seqno-1)){
> /* The sequence number of the incoming segment is in
> between the sequence numbers of the previous and
> the next segment on ->ooseq. We trim and insert the
> incoming segment and trim the previous segment, if
> needed. */
> cseg = tcp_seg_copy(&inseg);
> if (cseg != NULL) {
> if (TCP_SEQ_GT(prev->tcphdr->seqno + prev->len, seqno)) {
> /* We need to trim the prev segment. */
> prev->len = (u16_t)(seqno - prev->tcphdr->seqno);
> pbuf_realloc(prev->p, prev->len);
> }
> prev->next = cseg;
> if (TCPH_FLAGS(cseg->tcphdr) & TCP_FIN) {
> cseg->next = NULL;
> while (next != NULL) {
> struct tcp_seg *old_seg = next;
> next = next->next;
> memp_free(MEMP_TCP_SEG, old_seg);
> }
> }
> else {
>>>>> /* delete some segments */
>>>>> while (next &&
> TCP_SEQ_GT((seqno + cseg->len),
> (next->tcphdr->seqno + next->len))) {
> struct tcp_seg *old_seg = next;
> next = next->next;
> memp_free(MEMP_TCP_SEG, old_seg);
> }
> if (next &&
> TCP_SEQ_GT(seqno + cseg->len, next->tcphdr->seqno)) {
> /* We need to trim the incoming segment. */
> cseg->len = (u16_t)(next->tcphdr->seqno - seqno);
> pbuf_realloc(cseg->p, cseg->len);
> }
> cseg->next = next;
> }
> }
> break;
|
Fri 20 Nov 2009 04:59:34 PM UTC, comment #7:
I'm guessing Oleg can test, and as it's clear that we've got a bug in this situation we can only make it better!
|
Fri 20 Nov 2009 04:50:43 PM UTC, comment #6:
> In this patch I have implemented exactly what you have suggested.
For the FIN case, yes, but not for the case when both segments don't contain a FIN. In this case we could still save memory by using only the first segment instead of trimming the first and keeping both segments. This is a rare case though, and probably not worth messing with the code.
Another question: how do we test such things? We would need a test environment that creates such segments, which would bring us to task #7930: Create automatic test cases...
|
Fri 20 Nov 2009 04:41:08 PM UTC, comment #5:
If we add to the oos-queue packet with FIN flag,
all packets after it can be dropped.
In this patch I have implemented exactly what you have suggested.
|
Fri 20 Nov 2009 04:23:15 PM UTC, comment #4:
Why does the patch test for FIN? I think this problem would also be solved if we would always try to remove a second segment instead of trimming the first (if the last seqno is the same).
I don't have a patch for that, though.
|
Fri 20 Nov 2009 04:12:04 PM UTC, comment #3:
No, we shouldn't remove FIN flag.
It is better to delete some segments.
You could try this patch, but it should be checked.
(file #19092)
|
Fri 20 Nov 2009 03:42:25 PM UTC, comment #2:
From looking at the code it looks like we do the right thing when the second segment is in order - there is lots of checking of TCP flags there - but ignore them completely when trimming if the second segment is also out of order. Whenever we trim a segment in that section (tcp_in.c:1244 -> 1363) we need to check the flags and removing any FIN if present.
|
Fri 20 Nov 2009 03:36:30 PM UTC, comment #1:
If anyone can propose a simple fix for this in the next couple of days this could go into 1.3.2
|
Thu 19 Nov 2009 04:55:13 PM UTC, original submission:
Suppouse we have on the oos_queue one segment
seqno=2100, len=100, FIN flag
After retransmit we receive out of sequence 2nd segment
seqno=1100, len=1100, FIN flag
After trimming we have on the oos queue
two segments with FIN flag
seqno=1100, len=1000, FIN flag
seqno=2100, len=100, FIN flag
If second segment will be received in-sequence - everything all right, oos queue will be deleted,
but in this case flags are not processed correct.
|