buglwIP - A Lightweight TCP/IP stack - Bugs: bug #35927, missing refragmentaion in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #35927: missing refragmentaion in ip_forward

Submitter:  Malte Langermann <gagarin>
Submitted:  Wed 21 Mar 2012 08:46:49 AM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.0

Jump to the original submission

Mon 13 Aug 2012 08:23:29 PM UTC, comment #17: 

This is fixed, why didn't I close it?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 12 Apr 2012 07:03:54 PM UTC, comment #16: 

Thanks Simon - I was clearly a bit too tired when making comment 14, it's much clearer now!

Kieran Mansley <kieranm>
Group Member
Thu 12 Apr 2012 05:44:57 PM UTC, comment #15: 


> If we can't test before the release we should consider backing
> it out from 1.4.1


As it wouldn't work anyway as it is now (sending a packet bigger than MTU can't work without a bug in the netif driver...), I personally wouldn't have a problem with releasing it as it is now...

> I'm a little confused though: in the original report it uses
> netif->mtu to apparently refer to the far end's MTU. Doesn't
> that refer to our MTU?


Doesn't it refer to the MTU of the network the netif (used for sending) is connected to? If so, where's the difference between 'our MTU' and 'the far end's MTU'? If there's a router in between, things can of course still fail, but in that case, the router has to either fragement the packet or inform the originating host via ICMP.

> And why would we have enqueued something larger than our network's MTU?


We're speaking of forwarding: if the MTU of the target netif is smaller than the MTU of the source netif, we have to fragment the outgoing packet.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 09 Apr 2012 03:19:46 PM UTC, comment #14: 

If we can't test before the release we should consider backing it out from 1.4.1

I'm a little confused though: in the original report it uses netif->mtu to apparently refer to the far end's MTU.  Doesn't that refer to our MTU?  And why would we have enqueued something larger than our network's MTU?

Kieran Mansley <kieranm>
Group Member
Thu 29 Mar 2012 05:16:31 PM UTC, comment #13: 

Can't test it as my 2nd netif is IPv6 only... anyone else?

Ivan Delamer <idelamer>
Group Member
Wed 28 Mar 2012 04:33:13 PM UTC, comment #12: 


> You didn't close the bug?


Could someone please test this? I don't have a routing setup here.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 Mar 2012 02:26:04 PM UTC, comment #11: 

You didn't close the bug?

Mason <mason>
Sun 25 Mar 2012 01:17:21 PM UTC, comment #10: 

Fixed in both master and 1.4.1

Simon Goldschmidt <goldsimon>
Group administrator
Sun 25 Mar 2012 01:13:14 PM UTC, comment #9: 


> I'm suggesting outgoing destination unreachable if we are unable
> to fragment, or if DF is set (IPH_OFFSET(iphdr) & IP_DF). Or at
> least respect DF and silently drop in this last case.


I have checked DF and sent destination unreachable with code 4 (fragmentation needed but DF set) but I'm not sure what to do if IP_FRAG is disabled. I've just silently dropped the frame in this case.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 22 Mar 2012 06:57:34 PM UTC, comment #8: 

You could add '#define LWIP_MINIMAL 1' to keep a minimal footprint and feature set and add non-required and more conforming code and features if LWIP_MINIMAL is 0.  Those of us like me putting devices on corporate lans and/or the internet may appreciate as much RFC conformance and compliance as developers can add.  I know I do appreciate it.

Bill Auerbach <billauerbach>
Thu 22 Mar 2012 06:39:00 PM UTC, comment #7: 

I don't think we need to implement path discovery in icmp4 input.

It's done in IPv6 because it's mandatory, as routers can't fragment v6 packets.

I'm suggesting outgoing destination unreachable if we are unable to fragment, or if DF is set (IPH_OFFSET(iphdr) & IP_DF). Or at least respect DF and silently drop in this last case.

Ivan Delamer <idelamer>
Group Member
Thu 22 Mar 2012 06:35:58 PM UTC, comment #6: 

Fixed in master. (Could someone please test this? I don't have a routing setup here.)

Any thoughts about 1.4.1?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 22 Mar 2012 06:21:10 PM UTC, comment #5: 


> "Fragmentation Needed and DF Set"


Isn't that's what's needed for path MTU discovery?

> In this case the router must send back the ICMP message. I don't think we are checking for this at the moment.


We're neither reacting on incoming code 4 nor sending out one. In fact, icmp_input() really only implements echo reply ("ping"). Outgoing packets are only created with code 3 (destination unreachable: IP protocol or UDP port unknown) and 11 (time exceeded: TTL is 0 or reassembly timeout). There's plenty of room for improvement in the icmp4 module, we just though it's small and enough like it is now, given the fact that lwIP is most often used as simple device (i.e. no routing, only one netif).

Of course we still should implement standards as long as the code doesn't get bloated (which I doubt in this case).

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Mar 2012 05:10:13 PM UTC, comment #4: 

There's ICMP Destination Unreacheable code 4:

"Fragmentation Needed and DF Set"

In this case the router must send back the ICMP message. I don't think we are checking for this at the moment.

We could also used ICMP Destination Unreacheable code 13 "Communication administratively prohibited" if IP_FRAG is not defined.

Ivan Delamer <idelamer>
Group Member
Wed 21 Mar 2012 03:48:09 PM UTC, comment #3: 

this is correct for IPv6, but not for IPv4
ICMPv4 does not have a "Packet too big" message.

An IPv4 route must implement fragmentation, see
http://www.rfc-editor.org/rfc/rfc1812.txt
Page 28, Page 45, Page 80

Malte Langermann <gagarin>
Wed 21 Mar 2012 02:42:21 PM UTC, comment #2: 

Just a note that, according to RFC, an IPv4 router has the option to either fragment the packet to fit the outgoing netif's MTU, or sending back an ICMP "Packet too big" message.

So if IP_FRAG is not defined, we should still check packet size vs netif MTU and send back a ICMP PTB if necessary.

(In IPv6 you always send a ICMPv6 PTB back, as part of path MTU discovery.)

Ivan Delamer <idelamer>
Group Member
Wed 21 Mar 2012 08:58:28 AM UTC, comment #1: 

I'd target this at 1.4.1 since it's a simple fix which still doesn't affect many systems (forwarding is not a commonly used feature in lwIP, I guess).

I'm not sure if simply calling ip_frag() is enough, but I'll test that. If it's true, 1.4.1 should be good.

(The suggested fix is slightly wrong, since we cannot return the return value of netif->output.)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Mar 2012 08:46:49 AM UTC, original submission:  

In ip_forward no ip refragmentation is done, so that if the mtu of the target Interface is smaller than the forwarding packet, the packet will be dropped.
The code should be changed form this:

core/ipv4/ip.c:
ip_forward:
266    PERF_STOP("ip_forward");
267    /* transmit pbuf on chosen interface */
268    netif->output(netif, p, &current_iphdr_dest);
269    return;
270    return_noroute:
271    snmp_inc_ipoutnoroutes();
272  }
273

to something like this (copied from ip_output in the same file)

ip_output:
774  #if IP_FRAG
775    /* don't fragment if interface has mtu set to 0 [loopif] */
776    if (netif->mtu && (p->tot_len > netif->mtu)) {
777      return ip_frag(p, netif, dest);
778    }
779  #endif /* IP_FRAG */
780
781    LWIP_DEBUGF(IP_DEBUG, ("netif->output()"));
782    return netif->output(netif, p, dest);
783  }


Malte Langermann <gagarin>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by mason (Posted a comment)
  • -email is unavailable- added by billauerbach (Posted a comment)
  • -email is unavailable- added by idelamer (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by gagarin (Submitted the item)
  • -email is unavailable- added by gagarin
  •  

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-08-13 goldsimon Open/ClosedOpen Closed
    2012-03-25 goldsimon StatusIn Progress Fixed
        Planned Release 1.4.1
    2012-03-22 idelamer StatusReady For Test In Progress
    2012-03-22 goldsimon StatusIn Progress Ready For Test
    2012-03-21 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2012-03-21 gagarin Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code