tasklwIP - A Lightweight TCP/IP stack - Tasks: task #14724, thread safe ARP cache APIs

 
 

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

task #14724: thread safe ARP cache APIs

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Fri 10 Nov 2017 03:25:42 AM UTC
   
 
Category:  socket/netconn Should Start On:  Fri 10 Nov 2017 12:00:00 AM UTC
Should be Finished on:  Fri 10 Nov 2017 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  jcunningham Percent Complete:  100%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Thu 16 Nov 2017 08:33:00 PM UTC, comment #13: 

Committed in c47d161d4a8946028fff6143160e9f58d1a4be49

Joel Cunningham <jcunningham>
Group Member
Thu 16 Nov 2017 07:33:45 PM UTC, comment #12: 

Right, push it!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 16 Nov 2017 07:31:20 PM UTC, comment #11: 


> Hmm, to me, everything but sockets and the like (e.g. netdb, if_api) is meant to use lwIP structures (e.g. ip_addr_t, struct eth_addr, etc). In this context, a netif is identified by its struct netif.


Your point makes sense, the one thing I think about is that the struct netif pointer can really only safely be used by the code that owns that instance of struct netif (which is typically the link-layer) because as you've mentioned, there is a race holding a pointer when the netif could be removed by the link-layer code (even worse if struct netif was dynamically allocated and now it points to free memory).

Using some kind of opaque identifier like index or handle avoids these race problems.

> Given that, we probably should leave out the netif pointer altogether for now, since it's not used right now, anyway. Or do you need it?


Nope don't need it, I was just exploring your idea of including it :)  I'll remove it and get my patch submitted.

Joel Cunningham <jcunningham>
Group Member
Thu 16 Nov 2017 06:45:27 PM UTC, comment #10: 

Hmm, to me, everything but sockets and the like (e.g. netdb, if_api) is meant to use lwIP structures (e.g. ip_addr_t, struct eth_addr, etc). In this context, a netif is identified by its struct netif.

Netif indices are used on a level above this. It's OK to write applications on that level, but I don't want to add functions supporting the highest API level only, leaving out the middle.

We've had that already and people are coming and asking how to do this and that at netconn level. We then see that things are implemented at socket level and raw level but not at netconn level. As long as we support the netconn level (i.e. threaded, lwIP-specific API), we should do it correctly.

I thought of doing something like 'netifapi_arp_add(netifapi_get_netif_by_index(0), ipaddr, ethaddr)', but that would introduce a race if a netif is removed. We'd probably have no choice but to add both functions.

Given that, we probably should leave out the netif pointer altogether for now, since it's not used right now, anyway. Or do you need it?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Nov 2017 10:42:02 PM UTC, comment #9: 


> Sounds like this is missing then? Netif index is really a level above netifapi, isn't it?


I should clarify my comment of "no way to go from interface name/index to struct netif", I can write code that calls LOCK_TCPIP_CORE() and then calls netif_find().  What I meant was that its an architecture layering violation for network application to be directly interacting with struct netif.

To me, the netifapi APIs are mainly link level operations that require a struct netif pointer. The one exception is the recent additions of:

err_t netifapi_netif_name_to_index(const char *name, u8_t *index);
err_t netifapi_netif_index_to_name(u8_t index, char *name);

Which are meant to be used by applications.  So my thought would be if the ARP APIs are intended to be used by applications (as opposed to link-layer management code), then they should take index




Joel Cunningham <jcunningham>
Group Member
Wed 15 Nov 2017 08:03:56 PM UTC, comment #8: 

Ok, this looks good.

> there are no way to go from interface name/index to struct netif


Sounds like this is missing then? Netif index is really a level above netifapi, isn't it?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Nov 2017 03:25:27 PM UTC, comment #7: 

Simon,

I've attached a patch that implements these APIs as part of netifapi.  I think this works out a lot better than netconn.

I've added the netif parameter, though currently it's unused.  For the delete case, it seems that passing NULL could delete all instances because there could be duplicates (one per netif) if a device has two netifs on the same subnet

One additional discussion point: I'm wondering if the APIs should take interface index rather than struct netif * because the places I intend on using the APIs are network applications that sit above the socket/netconn layer.  These applications have no idea of a 'struct netif' and there are no way to go from interface name/index to struct netif.  The inner ARP APIs would still take netif, but we'd just convert from index once inside the core context, thoughts?

(file #42407)

Joel Cunningham <jcunningham>
Group Member
Tue 14 Nov 2017 09:37:16 PM UTC, comment #6: 


> netconn_gethostbyname


Hmm, good point... however, DNS seems more widely used (and standardized) than ARP table access.

> Would you like to see this as part of the task?


Not necessarily. I just don't want us to make the same mistake with a new API. But if you find the time, why not add this to the old API (probably as a new function). I'm not sure how the "delete" call would behave though...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Nov 2017 09:30:08 PM UTC, comment #5: 


> - do we need the #if guards around including files?


Good point, since the header themselves are guarded internally with their own switches, I can always include them

> - the names of the functions are a bit misleading. There's nothing depending on netconns here. I'd probably move the functions to netifapi


I had the same thought about whether netconn was really the best place, but there was an existing precedent of netconn_gethostbyname() which doesn't not take a netconn parameter.  I will investigate netifapi

> Our static entries API seems to be missing a netif (as I think it's older than that). Should we add a 'netif' parameter to the new API that means old behaviour if NULL but replaces the 'ip_route' call if not NULL?


This would require an API change (or new API) in etharp.c since etharp_add_static_entry doesn't take netif as a parameter.  Would you like to see this as part of the task?

Joel Cunningham <jcunningham>
Group Member
Tue 14 Nov 2017 09:08:48 PM UTC, comment #4: 


> Request For Comment on the attached patch


- LWIP_TCPIP_CORE_LOCKING only is OK for me
- do we need the #if guards around including files?
- the names of the functions are a bit misleading. There's nothing depending on netconns here. I'd probably move the functions to netifapi

Our static entries API seems to be missing a netif (as I think it's older than that). Should we add a 'netif' parameter to the new API that means old behaviour if NULL but replaces the 'ip_route' call if not NULL?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Nov 2017 08:43:37 PM UTC, comment #3: 

Request For Comment on the attached patch.

I've added some simple netconn APIs for accessing ARP cache in the LWIP_TCPIP_CORE_LOCKING configuration. 

I put a little bit of thought into supporting other types of ARP cache entries besides the current 'static' (permanent) entries through an enumeration. Another use case that could be supported in the future is to add a normal ARP cache entry that expires

(file #42404)

Joel Cunningham <jcunningham>
Group Member
Fri 10 Nov 2017 08:36:29 PM UTC, comment #2: 

That's a good point.  The benefit of implementing the ARP ioctls would be that developers could share/reuse Linux based (or other old UNIX) application code that makes use of the ARP ioctls without having to modify it for the netconn API.

Given that I don't have any of this application code currently and maybe since it's not common given the preferred methods of routing/netlink sockets, a netconn API would be good enough for now.

Also, I'm thinking of only supporting this for LWIP_TCPIP_CORE_LOCKING to avoid the usual new API burden for !LWIP_TCPIP_CORE_LOCKING (which is a new struct message type and macros to abstract the  memory pool case for the LWIP_MPU_COMPATIBLE)

Joel Cunningham <jcunningham>
Group Member
Fri 10 Nov 2017 07:26:12 PM UTC, comment #1: 

If it's not standardized, why do you need this at socket level? Wouldn't it be enough to implement it at API (i.e. netconn level) so that everyone can use it from another thread? (not only the ones compiling in socket support)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 10 Nov 2017 03:25:42 AM UTC, original submission:  

I'm needing socket level ARP cache APIs to execute etharp_add_static_entry and etharp_remove_static_entry from outside of the LwIP context

I did some research and within UNIX there are ioctls (SIOCSARP, SIOCDARP and SIOCGARP) for interacting with the ARP cache. I wanted to get people's thoughts on implementing these before going forward with the implementation for the following reasons:

 1) This isn't an open group standard, but historic UNIX
 2) It appears that some UNIX OS's have deprecated/removed these in favor of using routing sockets. These appear unsupported on current FreeBSD and NetBSD. Linux has these implemented, but I believe using netlink via NETLINK_ARPD is the preferred approach

Linux implementations: http://man7.org/linux/man-pages/man7/arp.7.html

Steven's Unix Network Programming Vol. 1 has a good description of the original UNIX implementation for me to go off of including the only source which explains about struct arpreq: "arp_ha is a generic socket address structure with sa_family set to AF_UNSPEC and sa_data containing the hardware address (e.g., the 6-byte Ethernet address)."

Given that LwIP doesn't have anything fancy like netlink or routing sockets, these ioctls seem the most appropriate to me

Joel Cunningham <jcunningham>
Group Member

 

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

 

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 jcunningham (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-16 jcunningham StatusNeed Info Done
        Percent Complete0% 100%
        Open/ClosedOpen Closed
    2017-11-15 jcunningham Attached File- Added 0001-netifapi-add-thread-safe-ARP-APIs-task-14724.patch, #42407
    2017-11-14 jcunningham Attached File- Added 0001-netconn-Add-thread-safe-ARP-cache-APIs-task-14724.patch, #42404
    2017-11-14 jcunningham Summarysocket level ARP cache APIs thread safe ARP cache APIs

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code