I don't understand the change. The epoch protection is already right here. We can safely use CK_STAILQ_FIRST.

In D42972#980127, @glebius wrote:

I don't understand the change. The epoch protection is already right here. We can safely use CK_STAILQ_FIRST.

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.

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.

In D42972#980139, @glebius wrote:

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.

Looking, we do have if_getlladdr(). It looks like the correct fix in 7d482240 should have been to just wrap the existing call in the epoch. @bz?

In D42972#980141, @jhibbits wrote:

In D42972#980139, @glebius wrote:

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.

Looking, we do have if_getlladdr(). It looks like the correct fix in 7d482240 should have been to just wrap the existing call in the epoch. @bz?

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.

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.

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.

In D42972#1242756, @glebius wrote:

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.

Typically drivers want if_getlladdr() but not if_getifaddr().