[lwip-users] Re: [lwip] ARP bug (compare MAC instead of IP)

[John C. Toman]

Hi Leon,

It is very possible for one MAC address to be configured for two or more 
IP addresses! In my own office environment, this is done for various 
reasons. Proxy ARP (often used for PPP and firewalls) is another case 
where this is done. Thus it is appropriate to use IP addresses as a 
comparison point, not MAC addresses.

With an ARP cache, and the scenario you mentioned (where an IP address 
is changed), the old entry should quickly time out when it isn't used. 
Your scenario is one of the key arguments for quick ARP cache timeouts. 
Timeouts aren't in the current lwIP ARP implementation but will be at 
some point. Another stack I've used sets ARP cache timeouts for five 
minutes. Other than not implementing timeouts, I believe Adam's 
implementation of this is correct.

Regards,

John

address@hidden wrote:

>Hello Adam,
>
>in the ARP code, when adding an (MAC, IP) entry, you use IP
>addresses to compare. You should, however, use the MAC
>addresses to find an old entry.
>
>----
>To see what goes wrong, using IP address compare, an example:
>
>Suppose client with MAC 01:02:03:04:05:06 has obtained
>IP address 192.168.0.2, and by whatever means, is configured
>to use 192.168.0.3 now.
>
>Adding this (MAC, IP) mapping will not find the old entry, and create
>a new entry in the ARP cache.  The ARP cache will now hold two
>mappings with the same MAC address.
>----
>
>static void
>add_arp_entry(struct ip_addr *ipaddr, struct eth_addr *ethaddr)
>{
>  u8_t i, j, k;
>  u8_t maxage;
>
>  /* Walk through the ARP mapping table and try to find an entry to
>     update. If none is found, the IP -> MAC address mapping is
>     inserted in the ARP table. */
>  for(i = 0; i < ARPTABLE_SIZE; ++i) {
>
>    /* Only check those entries that are actually in use. */
>    if(!ip_addr_isany(&arp_table[i].ipaddr)) {
>      /* Check if the source IP address of the incoming packet matches
>         the IP address in this ARP table entry. */
>      if(ip_addr_cmp(ipaddr, &arp_table[i].ipaddr)) {
><--------------------------- should test MAC bytes here, not IP address
>     /* An old entry found, update this and return. */
>
>[This message was sent through the lwip discussion list.]
>



[This message was sent through the lwip discussion list.]