[pisa-src] Re: r2467 - trunk/openwrt/patches/120-dhcp-ma-ap.patch

  • From: René Hummen <rene.hummen@xxxxxxxxxxxxxxxxx>
  • To: pisa-src@xxxxxxxxxxxxx
  • Date: Mon, 11 Apr 2011 13:54:12 +0200

This is an incomplete review of the code adding Mobile ACcess-specific IP 
address assignment to ISC' DHCP server. I'm especially focusing on the 
dhcp-server integration here.

On 23.03.2011, at 11:14, Rene Hummen wrote:
> Author: hummen
> Date: Wed Mar 23 11:14:04 2011
> New Revision: 2467
> 
> Log:
> patch for dhcp-3.1.3 enabling static client leases
> 
> NOTE: This patch needs review and testing.
> 
> Added:
>   trunk/openwrt/patches/120-dhcp-ma-ap.patch
> 
> Added: trunk/openwrt/patches/120-dhcp-ma-ap.patch
> ==============================================================================

> +diff -ruN dhcp-3.1.3-orig/includes/site.h dhcp-3.1.3/includes/site.h
> +--- dhcp-3.1.3-orig/includes/site.h  2009-04-07 22:00:42.000000000 +0200
> ++++ dhcp-3.1.3/includes/site.h       2011-03-21 21:44:00.418827814 +0100
> +@@ -195,3 +195,11 @@
> +    traces. */
> + 
> + #define TRACING
> ++
> ++/* Define this if you want to be able to support class A/B networks. */

ISC's DHCP server only supports class C networks out of the box? Are larger 
subnets relevant for our demonstration scenario?

> ++
> ++#define LARGE_SUBNET

Has this patch been implemented by Max or is this a known issue that has been 
patched elsewhere?

> ++
> ++/* Define this if you use this dhcpd on a mobileACcess AP */
> ++
> ++#define MOBAC_MODE

Note that MOBAC_MODE can be enabled independently from LARGE_SUBNET. This will 
be important later on.

> +diff -ruN dhcp-3.1.3-orig/server/confpars.c dhcp-3.1.3/server/confpars.c
> +--- dhcp-3.1.3-orig/server/confpars.c        2009-09-01 22:32:28.000000000 
> +0200
> ++++ dhcp-3.1.3/server/confpars.c     2011-03-21 21:44:00.425769109 +0100
> +@@ -3341,8 +3341,64 @@
> +     }
> + #endif /* FAILOVER_PROTOCOL */
> + 
> ++#if defined (LARGE_SUBNET)
> ++    do {
> ++            unsigned min, max;
> ++            struct range *range;
> ++
> ++            /* Make sure that high and low addresses are in same subnet. */
> ++            net = subnet_number (low, subnet -> netmask);
> ++            if (!addr_eq (net, subnet_number (high, subnet -> netmask)))
> ++                    break;
> ++
> ++            /* Make sure that the addresses are on the correct subnet. */
> ++            if (!addr_eq (net, subnet -> net))
> ++                    break;
> ++
> ++            /* Get the high and low host addresses... */
> ++            max = host_addr (high, subnet -> netmask);
> ++            min = host_addr (low, subnet -> netmask);
> ++
> ++            /* Allow range to be specified high-to-low as well as 
> low-to-high. */
> ++            if (min > max) {
> ++                    max = min;
> ++                    min = host_addr (high, subnet -> netmask);
> ++            }
> ++
> ++            /* Allocate if range is small enough */
> ++            if (max - min + 1 <= MAX_ALLOCATE_RANGE)
> ++                    break;
> ++
> ++            /* Record the range for later use */
> ++            range = dmalloc (sizeof(struct range), MDL);
> ++            if (!range)
> ++                    log_fatal ("Can't allocate range");
> ++            low  = ip_addr (subnet -> net, subnet -> netmask, min);
> ++            high = ip_addr (subnet -> net, subnet -> netmask, max);
> ++            range -> low  = low;
> ++            range -> high = high;
> ++            range -> min  = min;
> ++            range -> max  = max;
> ++            range -> cnt  = max - min + 1;
> ++
> ++            range -> next = pool -> range;
> ++            pool -> range = range;
> ++
> ++            log_debug ("saved range: %u.%u.%u.%u ~ %u.%u.%u.%u, cnt=%u",
> ++                       low.iabuf[0], low.iabuf[1], low.iabuf[2], 
> low.iabuf[3],
> ++                       high.iabuf[0], high.iabuf[1], high.iabuf[2], 
> high.iabuf[3], range -> cnt);
> ++
> ++            pool_dereference (&pool, MDL);
> ++            return;
> ++    } while (0);

do {} while (false)... So we just add just one class C network to our list 
here? How does this help with larger subnets?

> ++
> ++    /* Create the new address range... */
> ++    new_address_range (cfile, low, high, subnet, pool, lpchain, 0);

What does the value 0 represent here? 

> ++#else /*LARGE_SUBNET*/
> +     /* Create the new address range... */
> +     new_address_range (cfile, low, high, subnet, pool, lpchain);
> ++#endif/*LARGE_SUBNET*/

Why do we need to add that parameter to support large subnets?

> +diff -ruN dhcp-3.1.3-orig/server/dhcp.c dhcp-3.1.3/server/dhcp.c
> +--- dhcp-3.1.3-orig/server/dhcp.c    2009-09-01 22:32:28.000000000 +0200
> ++++ dhcp-3.1.3/server/dhcp.c 2011-03-21 21:44:00.433702018 +0100
> +@@ -39,11 +39,20 @@
> + 
> + #include "dhcpd.h"
> + 
> ++#if defined (MOBAC_MODE)
> ++#include "mac2ip.h"
> ++#include <time.h>
> ++#endif
> ++
> + int outstanding_pings;
> + 
> + static char dhcp_message [256];
> + static int site_code_min;
> + 
> ++#if defined (MOBAC_MODE)
> ++const char mobac_interface[] = "br0";
> ++#endif

Why do we need to hardcode this value? Can we have that as a configuration 
option? For the demo-scenario it might even be ok to restrict the dhcp service 
to an interface and remove this option here.

> +@@ -2102,6 +2111,26 @@
> +                             lease_time = default_lease_time;
> +             }
> + 
> ++#if defined (MOBAC_MODE)
> ++            /* Check the number of seconds till midnight. If this is less
> ++               than default_lease_time set it as lease_time */

Why is midnight an important point in time? How does the address-assignment 
work?

> ++            if (strcmp(packet -> interface -> name, mobac_interface) == 0) {
> ++                    time_t cur_time = time(NULL);
> ++                    struct tm timestruct;
> ++                    gmtime_r(&cur_time, &timestruct);
> ++
> ++                    int seconds_to_midnight;
> ++
> ++                    seconds_to_midnight = (23 - timestruct.tm_hour) * 60 * 
> 60;
> ++                    seconds_to_midnight += (59 - timestruct.tm_min) * 60;
> ++                    seconds_to_midnight += 60 - timestruct.tm_sec;
> ++
> ++                    if ((int) lease_time > seconds_to_midnight)
> ++                            lease_time = seconds_to_midnight;
> ++            }
> ++#endif
> ++
> ++
> + #if defined (FAILOVER_PROTOCOL)
> +             /* Okay, we know the lease duration.   Now check the
> +                failover state, if any. */
> +@@ -3783,6 +3812,91 @@
> +             } else
> + #endif
> +             {
> ++
> ++#if defined (LARGE_SUBNET)
> ++                    while (!pool -> free && pool -> range) {
> ++                            struct range *range = pool -> range;
> ++                            unsigned cnt = range -> cnt;
> ++                            struct subnet *subnet = NULL;
> ++
> ++                            if (cnt < 0) cnt = 0;
> ++                            if (cnt > MAX_ALLOCATE_RANGE) cnt = 
> MAX_ALLOCATE_RANGE;
> ++
> ++                            /* find subnet from shared_network: 
> next_sibling? */
> ++                            if (cnt) {
> ++                                    struct shared_network *share = pool -> 
> shared_network;
> ++                                    for (subnet = share -> subnets; subnet; 
> subnet = subnet -> next_sibling) {
> ++                                            struct iaddr net = 
> subnet_number (range -> low, subnet -> netmask);
> ++                                            if (!addr_eq(net, subnet_number 
> (range -> high, subnet -> netmask)))
> ++                                                    continue;
> ++                                            if (addr_eq(net, subnet -> net))
> ++                                                    break;
> ++                                    }
> ++                            }
> ++                            if (!subnet)
> ++                                    break;
> ++
> ++                            if (cnt) {
> ++                                    
> ++#if defined (MOBAC_MODE)

The following code will only be executed if LARGE_SUBNET is defined as well. 
Bug or feature?

> ++                                    struct iaddr new_mobac_ip = {};
> ++                                    new_mobac_ip.len = 4;
> ++
> ++                                    /* Check if that unknown client is a 
> mobAC-client. If so, hash its MAC
> ++                                       to an IP without using the normal 
> IP-pool. */
> ++                                    if (strcmp(packet -> interface -> name, 
> mobac_interface) == 0) {
> ++                                            
> ++                                            log_debug("received 
> DHCPDISCOVER-packet on mobAC-interface!");
> ++                                            
> ++                                            mac2ip_mac_t clients_mac;
> ++                                            if (packet->haddr->hlen != 7) {

Magic number.

> ++                                                    log_error("clients 
> MAC-address length is not 6 byte!");

6 or 7?

> ++                                                    int j = 0;
> ++                                                    for (j; j < 
> packet->haddr->hlen; j++) {
> ++                                                            
> log_debug("HW[%d]: ", packet->haddr->hbuf[j]);
> ++                                                    }
> ++                                                    return 1;
> ++                                            } else {
> ++                                                    int iter = 1;
> ++                                                    for (iter; iter < 7; 
> iter++)
> ++                                                            
> clients_mac.x[iter-1] = packet->haddr->hbuf[iter];
> ++                                            }
> ++                                            
> ++                                            /* Now read the bounds for the 
> later generated IPs. */
> ++                                            mac2ip_ipv4_t calculated_ip;
> ++                                            if (mac_to_ip(&clients_mac, 
> &calculated_ip) != 0)
> ++                                                    return 1;

So this is where the IP addresses are assigned. However, I don't see where this 
code overwrites the default assignment done by the original DHCP server code.

> +diff -ruN dhcp-3.1.3-orig/server/mdb.c dhcp-3.1.3/server/mdb.c
> +--- dhcp-3.1.3-orig/server/mdb.c     2009-09-01 22:32:28.000000000 +0200
> ++++ dhcp-3.1.3/server/mdb.c  2011-03-21 21:44:00.447584608 +0100
> +@@ -525,12 +525,22 @@
> +     return 0;
> + }
> + 
> ++#if defined (LARGE_SUBNET)
> ++int new_address_range (cfile, low, high, subnet, pool, lpchain, instantiate)
> ++    struct parse *cfile;
> ++    struct iaddr low, high;
> ++    struct subnet *subnet;
> ++    struct pool *pool;
> ++    struct lease **lpchain;
> ++    int instantiate;

Is this new parameter (instantiate) really necessary?

> ++#else /*LARGE_SUBNET*/
> + void new_address_range (cfile, low, high, subnet, pool, lpchain)
> +     struct parse *cfile;
> +     struct iaddr low, high;
> +     struct subnet *subnet;
> +     struct pool *pool;
> +     struct lease **lpchain;
> ++#endif /*LARGE_SUBNET*/
> + {
> + #if defined(COMPACT_LEASES)
> +     struct lease *address_range;
> +@@ -594,6 +604,13 @@
> + #if defined (COMPACT_LEASES)
> +     address_range = new_leases (max - min + 1, MDL);
> +     if (!address_range) {
> ++
> ++#if defined (LARGE_SUBNET)
> ++            if (!cfile && instantiate) {
> ++                    return 0;
> ++            }

What kind of problem are we handling here? (more like this below)

> ++#endif /*LARGE_SUBNET*/
> ++
> +             strcpy (lowbuf, piaddr (low));
> +             strcpy (highbuf, piaddr (high));
> +             log_fatal ("No memory for address range %s-%s.",
> +@@ -611,12 +628,20 @@
> +             lease_reference (&lp, &address_range [i], MDL);
> + #else
> +             status = lease_allocate (&lp, MDL);

This seems like the right place to hook mac2ip functionality...rather than the 
place above.

René



--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

Other related posts: