Remove the final direct access of struct ifnet members from netlink.
Since only the first address is used, create the iterator and then free,
without fully iterating.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 54885 Build 51774: arc lint + arc unit
Event Timeline
| sys/netlink/route/iface.c | ||
|---|---|---|
| 329 | While it does nothing now I'd have to assume that there's some anticipated need for ifa_iter_finish() to either clean up the iterator or to release a lock (or epoch or ..). Wouldn't it belong more after the ifa != NULL check and dump_sa() call? | |
| sys/netlink/route/iface.c | ||
|---|---|---|
| 329 | I think ifa should survive until the epoch exits, ifa_iter_finish() is used to clean up the iterator itself. It's quite likely that I don't understand epoch, and I'm not opposed to moving it before the ifa_iter_finish() if that is the case. That said, on second thought I do see a possibility where ifa might point to just a static internal buffer in the iterator, overwritten each time through the loop and cleaned up at the end. | |
Comment Actions
I don't understand the change. The epoch protection is already right here. We can safely use CK_STAILQ_FIRST.
Comment Actions
It's not about safety, it's because netlink shouldn't know ifnet guts, it's a more generic interface. I'm trying to remove net/if_private.h from if_var.h as soon as possible, and this is the last netlink piece that touches directly.
Comment Actions
Got it. Looks like there is also an assumption here that the first address is always the link level address. Maybe we just need to provide KPI if_getlladdr? May I ask to wait for Alexander melifaro@ to reappear before we proceed forward with that?
P.S. I'm also waiting for him on my netlink reviews.
Comment Actions
I'm reaching this while trying to MFC some IfAPI related commits from stable/15 to stable/14.
Well I think you're right. if_getifaddr() ( wrapper around ifp->if_addr ) is the right accessor to get link level address. Using that directly is much simpler than a iterator.
Quote from the comment in if_private.h,
/* * if_addrhead is the list of all addresses associated to * an interface. * Some code in the kernel assumes that first element * of the list has type AF_LINK, and contains sockaddr_dl * addresses which store the link-level address and the name * of the interface. * However, access to the AF_LINK address through this * field is deprecated. Use if_addr instead.
Comment Actions
It appears the ifp->if_addr is not set to NULL ( atomically ) while the ifnet been detached.
if (!vmove) { /* * Prevent further calls into the device driver via ifnet. */ if_dead(ifp); /* * Clean up all addresses. */ IF_ADDR_WLOCK(ifp); if (!CK_STAILQ_EMPTY(&ifp->if_addrhead)) { ifa = CK_STAILQ_FIRST(&ifp->if_addrhead); CK_STAILQ_REMOVE(&ifp->if_addrhead, ifa, ifaddr, ifa_link); IF_ADDR_WUNLOCK(ifp); ifa_free(ifa); } else IF_ADDR_WUNLOCK(ifp); }
So the current approach is correct.
Comment Actions
Well I think you're right. if_getifaddr() ( wrapper around ifp->if_addr ) is the right accessor to get link level address. Using that directly is much simpler than a iterator.
I will assert that it is not the right accessor. The struct ifaddr has also a lot of kernel internals that drivers don't need and ideally drivers should not depend on ifaddr layout. It should be as opaque as struct ifnet. The if_getifaddr() was created with a logic "every field of ifnet needs an accessor", but proper opaque-ing of stack internals is not so straightforward.