lwIP - A Lightweight TCP/IP stack - Bugs: bug #46467, ip_frag() shouldn't modify pbuf in...
You are not allowed to post comments on this tracker with your current authentication level.
bug #46467: ip_frag() shouldn't modify pbuf in case of a retransmission
Submitter: | Zach Smith <zsmith> | ||
Submitted: | Tue 17 Nov 2015 09:29:56 PM UTC | ||
Category: | IPv4 | Severity: | 3 - Normal |
Item Group: | Faulty Behaviour | Status: | Fixed |
Privacy: | Public | Assigned to: | goldsimon |
Open/Closed: | Closed | Planned Release: | 2.0.0 |
lwIP version: | git head |
Jump to the original submission
Tue 18 Oct 2016 11:39:39 PM UTC, comment #13: |
Zach Smith <zsmith> |
Mon 17 Oct 2016 11:34:48 PM UTC, comment #12: It looks like the reassembled IP fragment is getting rejected because of an incorrect TCP checksum. I tried another test where I connect to my lwip device that is doing the fragmenting with another lwip device (instead of a PC). The ip packets that are not fragmented are sent and received with no error but the first fragmented packet gets rejected in lwip tcp_input() for failing tcp checksum. Also, if I enable checksum validation in wireshark I can see that the TCP checksum is invalid
|
Zach Smith <zsmith> |
Thu 13 Oct 2016 09:26:13 PM UTC, comment #11: Simon, I wanted to let you know I am trying to test this but I am having trouble. I have downloaded the newest source code today and updated. But I can't seem to get ip fragmentation to work as before. So, I haven't been able to get to testing the re-transmission of an ip fragment.
|
Zach Smith <zsmith> |
Tue 11 Oct 2016 07:48:36 AM UTC, comment #10: I've pushed a fix for this. However, I'd be glad if someone else would test this, too. It works for me, but I feel a little unconfortable pushing this so "short" before the release...
|
Simon Goldschmidt <goldsimon> |
Thu 25 Aug 2016 08:24:53 PM UTC, comment #9: Just a little clarification: this is only a problem for IP_FRAG_USES_STATIC_BUF==0 && LWIP_NETIF_TX_SINGLE_PBUF==0 |
Simon Goldschmidt <goldsimon> |
Thu 25 Aug 2016 07:55:18 PM UTC, comment #8: Sorry for the delay and thanks for the repro info. This must be fixed for 2.0.0 as ip4_frag cannot really be used in this state... |
Simon Goldschmidt <goldsimon> |
Fri 18 Mar 2016 10:38:54 PM UTC, comment #7: Oh, I see that you are right that the seg->p->len/tot_len modifications do still work if p->payload > tcphdr. They wrap around and work ok for me too. I don't think I was quite right in identifying the problem before.
|
Zach Smith <zsmith> |
Wed 16 Mar 2016 09:08:37 PM UTC, comment #6: Zach, I don't understand the part where you say that tcp_output_segment() adjusts the p->len based on the assumption that p->payload < tcphdr. For me the seg->p->len/seg->p->tot_len modifications do work if p->payload > tcphdr.
|
Simon Goldschmidt <goldsimon> |
Thu 18 Feb 2016 08:23:21 PM UTC, comment #5: Sorry my answer took so long.
|
Simon Goldschmidt <goldsimon> |
Fri 20 Nov 2015 06:48:31 PM UTC, comment #4: The IP frag modification is during TX of a TCP packet. To describe more clearly, ip_frag is modifying the payload. I understand that the lower layers do modify (decrement) the payload to add on IP/ETH headers but in ip_frag the payload is being incremented and modified to point further into the data payload as data is sent out. Specifically these lines:
|
Zach Smith <zsmith> |
Fri 20 Nov 2015 02:51:58 PM UTC, comment #3: Op,
|
Joel Cunningham <jcunningham> |
Fri 20 Nov 2015 12:03:54 AM UTC, comment #2: It remembers me that I hit that as well when adding MPPE support, I worked over it by duplicating the pbuf before changing the packet payload ( commit 97ef85c9aa ).
|
Sylvain Rochet <gradator> |
Thu 19 Nov 2015 06:29:32 AM UTC, comment #1:
|
Simon Goldschmidt <goldsimon> |
Tue 17 Nov 2015 09:29:56 PM UTC, original submission:
The ip_frag() function modifies the pbuf. This is a problem because the pbuf might be referenced again by TCP if unacked and a retry is needed.
|
Zach Smith <zsmith> |
Depends on the following items: None found
Items that depend on this one: None found
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 11 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2016-10-19 | dziegel | Status | Ready For Test | Fixed | |
Open/Closed | Open | Closed | |||
2016-10-17 | zsmith | Attached File | - | Added ip_frag_two_lwip_devices.pcapng, #38749 | |
2016-10-13 | zsmith | Attached File | - | Added ip_frag_lwip_20100816.pcapng, #38724 | |
Attached File | - | Added ip_frag_lwip_20101013.pcapng, #38725 | |||
2016-10-11 | goldsimon | Status | In Progress | Ready For Test | |
2016-08-25 | goldsimon | Status | Need Info | In Progress | |
Planned Release | None | 2.0.0 | |||
2016-03-16 | goldsimon | Status | None | Need Info | |
2016-02-18 | goldsimon | Assigned to | None | goldsimon | |
2015-11-17 | zsmith | Attached File | - | Added ip_frag_no_modify_p.patch, #35483 |
Powered by Savane 3.13-d3ae.
Corresponding source code
I think I found the problem. In ip4_frag() just after the loop where the mirrored pbufs are created (the while (left_to_copy) loop), the poff adjustment should be:
[line 788]
poff += newpbuflen;
instead of
poff = newpbuflen;
poff starts at IP_HLEN and should be incremented through the pbuf as fragments of it are sent - because now it is an offset from the original pbuf payload.
I changed that line and the fragmenting now works for me. Re-transmissions of the fragments work too.
My application always sends with contiguous pbufs (PBUF_RAM) but the code looks like it will also work for chained pbufs. I stepped through with my debugger and created a fake chained pbuf to see if it would handle it and it seemed to do it correctly.