buglwIP - 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: 

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.

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

The wireshark traces I attached 10/13 showed a PC (192.168.29.67) connecting to the lwip device (192.168.29.78) but I attached a new one that shows two lwip devices. This time 192.168.29.78 connects as a TCP client to 192.168.29.56. 192.168.29.56 starts fragmenting packets and with wireshark TCP checksum validation enabled, you can see that the TCP checksum is invalid for the fragmented packets.

I'm not sure why at this point. Any ideas?


(file #38749)

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.

As noted in the repro. info below my application sends ~1000 byte packets. With the old code (from Aug 16) I just set the netif mtu to 700 in low_level_init (netif->mtu = 700;) and then defined the following in lwipopts:

#define LWIP_ND6_ALLOW_RA_UPDATES 0 (so mtu doesn't get changed on me)
#define TCP_CALCULATE_EFF_SEND_MSS 0 (so MSS stays larger than MTU which causes ip fragmentation)

Then I just start running my application and I can see lwip sending the data in fragments.

With this new code lwip seems to send the fragment just like before but the fragmented packet is not ACKed by the other side (PC). I can't see why it doesn't get ACKed because the fragmented packet looks the same compared to the fragments generated by the older lwip code.

I am attaching some wireshark captures showing the difference between the two attempts (one with older code and one with newer code) in case someone has time to look at it.

I don't know what I am doing wrong yet.

(file #38724, file #38725)

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...
(since this is the last open issue targeted at 2.0.0...)

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
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>
Group administrator
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.

When execution reaches ip_frag() I notice that p->payload is pointing at the IP header. It is then incremented further into the data payload as chunks are sent out. The len member is modified appropriately (decremented) as chunks are sent. tot_len is not modified, it is left at the original value. So, at the end of ip_frag, tot_len does not equal len as they did in the beginning.

So, that right there seems to me to be the problem. tot_len > len and pbuf->next is null. I can't see that there is anything in tcp_output_segment() or anywhere else that fixes that. So, when a re-transmission happens we have a corrupt pbuf. tcp_output_segment() attempts to adjust p->payload to point back to the tcp header and to adjust p->len and p->tot_len. p->len gets adjusted correctly because it was modified in ip_frag() relating to the current position of p->payload. But since p->tot_len is the orignal total length (including the ip header) the adjustment doesn't work and it ends up still greater than p->len.

perhaps the fix could be as simple as setting tot_len = len before exiting ip_frag although that doesn't feel right and may affect other areas. I'm not sure. And I am not sure how all this would work with chained pbufs ???



Here is how I force this problem to occur (if it helps)

My application that I am running to test this sends packets that are ~1000 bytes. So, I set my mtu to be 700 so it will force ip fragmentation. I also have to #define TCP_CALCULATE_EFF_SEND_MSS 0 so the MSS is not changed. I then start my application which starts sending 1000 byte packets. I can see with Wireshark they are all getting fragmented. Then I put a breakpoint in tcp_rexmit_rto() and just wait to hit it. It doesn't take very long since I am on a Wi-Fi link. If on a local Ethernet link re-transmissions are very rare so one way to force it to happen is in ip_frag() where the netif->output() function is called do something like this:
if(dbg++ != 35){
netif->output(netif, rambuf, dest);
}
So when dbg reaches 35 the fragment will simply not be sent and a re-transmit will occur.

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.

Maybe this is due to 'len' being an u16_t? There might be compilers where implicit integer promotion messes up the -= calculation that should take care of fixing the pbuf length.

What compiler & platform are you seeing this on? Does it help to make 'len' a (signed!) 'int' instead of 'u16_t'?

I know what tcp does here to the pbuf is rather hacky, but the problem is not in ip_frag, it's in tcp_output_segment().

Simon Goldschmidt <goldsimon>
Group administrator
Thu 18 Feb 2016 08:23:21 PM UTC, comment #5: 

Sorry my answer took so long.

I think I now see what the problem is. However, currently I don't really know a fix to it. Your patch surely breaks at least one of my port's drivers that implements deferred transmission by keeping the tx pbufs on a list.

I'm not sure whether ip_frag is correct in changing the pbuf, but at least, TCP probably shouldn't retransmit if p->ref > 1. The tcp retransmit case of setting the payload pointer back to what it (possibly) was before the first transmission is kind of odd, I agree. :-)

Simon Goldschmidt <goldsimon>
Group administrator
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:

    /* Can just adjust p directly for needed offset. */
    p->payload = (u8_t *)p->payload + poff;
    p->len -= poff;

This seems like a problem because TCP references the pbuf and may try to re-transmit it later. When the retry happens, I think that the problem occurs when tcp_output_segment() checks the seg->p->payload and compares it against the seg->tcphdr. It tries to adjust the payload and len members but it is going on the assumption that the payload may have been decremented to add on headers. But it does not take into account that payload may have been incremented to point into the data. That is where I think the pbuf gets messed up.

I guess I don't know enough to say this is a problem with TCP (shouldn't ref the pbuf later) or with ip_frag (shouldn't modify the pbuf)

Specifically, we were able to catch this in a specific case where in ip_output_if_opt() before calling ip_frag() the p->tot_len != p->len AND p->next was NULL! The call stack showed it came from a tcp_slowtmr retransmit. The code assumes that if tot_len > len then there is another pbuf in the chain and it ended up referencing a null pointer. We got looking in ip_frag and figured that this was happening because the payload pointer was modified.

This fix is to make sure the structure (p->len and p->payload) are not changed upon exiting from what they were when entering


Zach Smith <zsmith>
Fri 20 Nov 2015 02:51:58 PM UTC, comment #3: 

Op,

Is IP frag's modification occurring during the TX of the TCP packet?  If so, there is still only a single reference to the pbuf (held by TCP layer) and it will be modified as the lower layers (IP, ARP, etc) perform their output processing.  As long as they aren't modifying TCP header or payload, things should be fine.

If the packet is enqueued in a driver or lower level, then the reference count would need to be incremented and measures would need to be taken if a modification happens at a later time that could contend with TCP retransmission.  Simon and I discussed this in https://savannah.nongnu.org/task/?7896


Joel Cunningham <jcunningham>
Group Member
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 ).

http://git.savannah.gnu.org/cgit/lwip.git/commit/?id=97ef85c9aa0291cb4b1670ffd29299014b18ffad

Sylvain Rochet <gradator>
Group Member
Thu 19 Nov 2015 06:29:32 AM UTC, comment #1: 


> [..] that seems to work for us.


The patch might indeed work for you, but it's not a general solution as pbufs may be put on a list to be transmitted later.

If not also done in the netif driver, at least etharp has to queue UDP packets when the MAC address has to be resolved before sending.

Could you clearly describe what you mention as "problem"?

In fact, the real problem to me is that TCP keeps a reference to pbufs while all other TX functions pass a buffer for sending to the stack and don't reuse it later...

Simon Goldschmidt <goldsimon>
Group administrator
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.

I have attached a patch to ip_frag() that restores the pbuf before exiting that seems to work for us.

Zach Smith <zsmith>

 

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

Attached Files
file #38749:  ip_frag_two_lwip_devices.pcapng added by zsmith (138KiB - application/octet-stream)
file #38724:  ip_frag_lwip_20100816.pcapng added by zsmith (383KiB - application/octet-stream)
file #38725:  ip_frag_lwip_20101013.pcapng added by zsmith (56KiB - application/octet-stream)
file #35483:  ip_frag_no_modify_p.patch added by zsmith (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Updated the item)
  • -email is unavailable- added by jcunningham (Posted a comment)
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by zsmith (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-10-19 dziegel StatusReady For Test Fixed
        Open/ClosedOpen 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 StatusIn Progress Ready For Test
    2016-08-25 goldsimon StatusNeed Info In Progress
        Planned ReleaseNone 2.0.0
    2016-03-16 goldsimon StatusNone Need Info
    2016-02-18 goldsimon Assigned toNone goldsimon
    2015-11-17 zsmith Attached File- Added ip_frag_no_modify_p.patch, #35483

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code