buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21655, DHCP doesn't work reliably with...

 
 

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

bug #21655: DHCP doesn't work reliably with multiple netifs

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Tue 27 Nov 2007 09:21:24 PM UTC  
 
Category:  UDP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

( Jump to the original submission)

Tue 04 Dec 2007 09:16:14 PM UTC, comment #16: 

Tested with 2 pktif-interfaces running under win32 (each with a different MAC address but connected to the same physical network). They both get an IP so everything seems to be fine.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 03 Dec 2007 08:35:24 PM UTC, comment #15: 

Checked in. The only thing missing now is a real-world-test. Hope I'll be able to do this with a virtual machine...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 03 Dec 2007 04:51:21 PM UTC, comment #14: 

Sorry, of course it does. Seems like I mixed up 'inp' and 'netif_list'...

But it was kind of early when I wrote that! ;-)

I'll take your post as approval and check it (meaning the mix of your code and mine) in.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 03 Dec 2007 03:09:30 PM UTC, comment #13: 

Oh, I didn't think it was the same - I thought the version I gave would skip inp in netif_list.

Jonathan Larmour <jifl>
Project Member
Mon 03 Dec 2007 07:26:51 AM UTC, comment #12: 

Your code does the same as my code, but your coed it's a little better to read!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 02 Dec 2007 10:49:14 PM UTC, comment #11: 

I'm not clear on why the first conditional is anything other than:
     if (first) {
?

If you want to of course you could add the extra 'if':
      if (first) {

        netif = netif_list;
        first = 0;

      } else {

        netif = netif->next;

      }
      if
(netif == inp)
          netif = netif->next;

    } while(netif != NULL);

Jonathan Larmour <jifl>
Project Member
Sun 02 Dec 2007 02:45:16 PM UTC, comment #10: 

> In that case, I think the best thing to do might be to check
> whether the 'inp' netif does match (in terms of IP address, mask,
> broadcast addr, etc.) and only resort to scanning the netif list
> if it doesn't. This will make things faster for the common case too!


Here's a patch for this. In ip_input, the code in the loop mainly stays the same but the simple for-loop is changed to a do..while and is started with inp, if that doesn't match, netif_list is searched.

The downside of this is that if inp doesn't match, it is checked twice (one time first and one time since it is on netif_list, too). But since this is a rather uncommon case, I think that's ok.

(file #14535)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 30 Nov 2007 03:30:58 PM UTC, comment #9: 

I'm not sure, but what it might be is to allow multiple netifs on the same underlying hardware interface, i.e. IP aliases. The hardware interface doesn't know about matching IPs, so it would have to just pick one netif to pass it up with. But ip_input would need to sort it out and get a proper match on the interface.

In that case, I think the best thing to do might be to check whether the 'inp' netif does match (in terms of IP address, mask, broadcast addr, etc.) and only resort to scanning the netif list if it doesn't. This will make things faster for the common case too!

Jonathan Larmour <jifl>
Project Member
Fri 30 Nov 2007 08:52:06 AM UTC, comment #8: 

The UDP patch works kind of nice, I'll check it in.

As to the patch to ip.c (checking for DHCP first, then processing the netif_list): I'm not sure it works without this: for the udp.c patch to work, it is necessary that the netif is correct. So When having multiple netifs on the same network, the code in ip_input might select the wrong netif to pass to udp_input...

But this is a generic problem in ip_input (selecting a netif from the list instead of passing the 'inp' netif after it is checked). I'm sure there is a reason for this code, but I just don't see it: it seems kind of unsafe to accept a packet from one netif and tell UDP it is from another netif?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 29 Nov 2007 07:49:07 AM UTC, comment #7: 

> Uh, don't you have this the wrong way round? It will make DHCP
> faster, but everything else slower because it will be doing the
> partial packet decoding and checks every time for every packet.


You're right of course. See how it can make you blind working on one thing all the time ;-)

>> Next, it would be good to prevent user-applications to bind to
>> DHCP_CLIENT_PORT when LWIP_DHCP is enabled
> I don't think we should worry about this.


Maybe you're right there, too. I wouldn't know a decent solution to check this, anyway.

I'll apply the first patch once I found a way to test this at home (Vmware shold somehow allow me to test on multilple networks).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 29 Nov 2007 01:33:16 AM UTC, comment #6: 

The first patch looks good, thanks (although I can't test it :-| ).

> BTW: ip_input could be sped up if checking for DHCP_CLIENT_PORT
> before walking the netif list (second file).


Uh, don't you have this the wrong way round? It will make DHCP faster, but everything else slower because it will be doing the partial packet decoding and checks every time for every packet.

DHCP is a very infrequent event, and everything else should be optimised.

> Next, it would be good to prevent user-applications to bind to
> DHCP_CLIENT_PORT when LWIP_DHCP is enabled, but there's no way
> of knowing whether the pcb belongs to dhcp.c or a user app in
> udp_bind :-(


I don't think we should worry about this. It would have to be a very deliberate act, so may even be intentional. I wouldn't think it would be good to add code to prevent something that's most likely to be irrelevant, and if anyone did do it, it's probably no a coincidence, and so they would have to remove the check!

Jonathan Larmour <jifl>
Project Member
Wed 28 Nov 2007 09:48:30 PM UTC, comment #5: 

Here's patch for udp_input (first file).
Couldn't test it much since I don't have two networks to connect to... But it works with ethernet-netif and loopif.

BTW: ip_input could be sped up if checking for DHCP_CLIENT_PORT before walking the netif list (second file).

Next, it would be good to prevent user-applications to bind to DHCP_CLIENT_PORT when LWIP_DHCP is enabled, but there's no way of knowing whether the pcb belongs to dhcp.c or a user app in udp_bind :-(

(file #14504, file #14505)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 28 Nov 2007 12:07:43 PM UTC, comment #4: 

> And keep in mind that udp_input also includes a hack to receive DHCP messages


Damn, I was wrong: the hack to receive DHCP messages on a netif that is down is in ip_input, not udp_input :-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 28 Nov 2007 08:54:26 AM UTC, comment #3: 

But we already have the netif in udp_input. Searching it based on the transaction ID seems kind of weird... We only have to find the correct pcb. And keep in mind that udp_input also includes a hack to receive DHCP messages, I only would make that hack a little bit 'better' :-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 28 Nov 2007 08:49:03 AM UTC, comment #2: 

Can't we use the dhcp's transaction id (which is 32bits) to initialize it with the netif* value or a netif index ? (but I doesn't read the code)

Frédéric Bernon <fbernon>
Project Member
Wed 28 Nov 2007 06:51:53 AM UTC, comment #1: 

Just an idea (as a reminder for later):

In udp_input, before looping over udp_pcbs to find the correct pcb, check (when LWIP_DHCP) if src-port == DHCP_SERVER and dst-port == DHCP_CLIENT, then check if inp->dhcp is != NULL and inp->dhcp->pcb is != NULL and then use that pcb. If not, don't loop over udp_pcbs -> no client software can use the DHCP port if DHCP is enabled.

Binding multiple dhcp pcbs to dhcp client port doesn't work yet...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 27 Nov 2007 09:21:24 PM UTC, original submission:  

This is from reading the code only but I'm pretty sure:

DHCP pcbs can't be bound to a specific netif (since at the time DHCP starts, that netif has no IP address and binding is done using IP addresses). That means when having multiple IP addresses, any of the active DHCP pcbs will get an incoming DHCP packet.

For that to be solved, the DHCP input code (parts of it in udp.c) should be changed to identify the netif a DHCP pcb wants to use. In other words, DHCP UDP pcbs should somehow bind to a specific netif rather then receive on all netifs.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.

 

Attached Files
file #14504:  udp.c_dhcpinput.patch added by goldsimon (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-12-04 goldsimon StatusIn Progress => Fixed
        Open/ClosedOpen => Closed
    2007-12-02 goldsimon Attached File- => Added ip_input_search_netif.patch, #14535
    2007-11-29 goldsimon StatusNone => In Progress
        Assigned toNone => goldsimon
    2007-11-28 goldsimon Attached File- => Added udp.c_dhcpinput.patch, #14504
        Attached File- => Added ip.c_ip_input_dhcp_faster.patch, #14505

    Back to the top


    Powered by Savane 3.5