buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46506, nd6_new_router() should check if...

 
 

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

bug #46506: nd6_new_router() should check if router exists in router list first

Submitter:  Zach Smith <zsmith>
Submitted:  Mon 23 Nov 2015 10:32:50 PM UTC
   
 
Category:  IPv6 Severity:  1 - Wish
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Thu 22 Dec 2016 05:15:11 PM UTC, comment #5: 

I tested this and it seems to work for me. I replaced our nd6_new_router() function with your new one and I do not get duplicate routers in the list. Thank you!

Zach Smith <zsmith>
Thu 15 Dec 2016 08:26:57 AM UTC, comment #4: 

I've decided to fix this (although the fix is slightly different than suggested) since it should not make much difference in code size and speed.

Zach, could you verify this works for you?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 16 Mar 2016 03:40:21 PM UTC, comment #3: 


> I must admit I don't really know all the problems that could/would/will arise from
> having 2 netifs in the same subnet


For lwIP, the problem is that all traffic which is not sent explicitly to an interface (e.g. via udp_sendto_if()) is sent to the first interface in the list that matches the network. Source-based routing (i.e. send all packets from an interface's IP via the corresponding interface) is only supported by you implementing the correct hook.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Mar 2016 10:21:26 PM UTC, comment #2: 

Thanks for looking at this.

We don't have any great way of dealing with the 2 netifs in the same subnet. We have just implemented a way to attempt to keep track of which netif packets from ip X come in on and send responses back on the same netif - I think this has been done by others. I must admit I don't really know all the problems that could/would/will arise from having 2 netifs in the same subnet. I would love to be educated on that.

Anyway, thanks for pointing out that the fix isn't quite right. Good catch. I see why we need to first iterate the whole router list before the "Look for empty entry" loop. We were attempting to do it in the same loop, but I see now that you must take into account that routers can be removed at any time from any spot in that list. Just because you find a NULL entry doesn't mean that the router does not already exist further down in the array.

So, instead you'd do this before the "Look for empty" loop:

  /* Check if router is already in the list before adding it */
  for (router_index = 0; router_index < LWIP_ND6_NUM_ROUTERS; router_index++) {
    if(default_router_list[router_index].neighbor_entry == &(neighbor_cache[neighbor_index])){
      return router_index;
    }
  }

Zach Smith <zsmith>
Sun 06 Mar 2016 01:58:56 PM UTC, comment #1: 

Well, strictly spoken, what you are doing is not supported by lwIP (having 2 netifs in the same subnet). In the past, we have not added code that was only needed for this case (and you should not have a problem in the 'one-netif-per-net' case).

But then again, if there is a simple way to allow this, I'm not opposed to do it.

The problem I see is that for TX packets, you would use the netif where you find the router first, which is rather random. So your problem might seem to be fixed, but you still don't have a decent implementation of multiple netifs on the same net.

Before trying to fix anything, I'd first like to know how you'd fix the rest...

PS: your fix is incorrect, by the way. You'd have to first iterate the whole router array before starting the "Look for empty entry" loop.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 Nov 2015 10:32:50 PM UTC, original submission:  

I think that nd6_new_router() should possibly check the default_router_list[] to see if the router already exists in the list before adding it. It first checks if there is a neighbor_cache[] entry for the router address passed in. If not, it creates one. Then it marks it as a router and adds an entry to the default_router_list[] (w/o checking if it is already there).

We experienced a problem in our (potentially unique) use case where we have 2 netifs (Ethernet & Wi-Fi) that are on the same network. The same router was discovered on both netifs. It was first discovered on Ethernet and it was added to the router list. Then when discovered again on the Wi-Fi netif (received a Router Advertisement) it was interesting what happened:

In nd6_input(), in the router advertisement case (ICMP6_TYPE_RA), nd6_get_router() is called to check to see if the router already exists - but this function checks against the netif. Since the netif is different, this function returns none found. So nd6_new_router() is called. A new neighbor_cache[] entry is not created since it already exists (with the Eth netif), and another entry is added in the default_router_list[] that points to that neighbor_entry again. So, the new router entry is a duplicate of what is already there - the entry with the Eth netif.

The process repeats every time a Router Advertisement is received and the default_router_list[] fills up with duplicate entries.

Because of the nd6_get_router() check that is done when RA is received this problem is avoided in the more usual case that the same router is not discovered on more than one netif.

I included a patch with a potential fix for this. This patch just adds a check in nd6_new_router() to check that the router is not already in the list before adding it again.

Thanks

Zach Smith <zsmith>

 

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

Attached Files
file #35533:  nd6_new_router_check_exists.patch added by zsmith (952B - 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 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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-12-15 goldsimon StatusNeed Info Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2016-03-06 goldsimon Severity3 - Normal 1 - Wish
        Item GroupFaulty Behaviour Change Request
        StatusNone Need Info
    2015-11-23 zsmith Attached File- Added nd6_new_router_check_exists.patch, #35533

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code