netlink: fix accessing freed memory

[bz]

Summary

The check for if_addrlen in dump_iface() is not sufficient to determine
if we still have a valid if_addr. Rather than directly accessing if_addr
check the STAILQ (for the first entry).
This avoids panics when destroying cloned interfaces as experienced with
net80211 wlan ones.

Sponsored by: The FreeBSD Foundation
MFC after: 3 days

[pstef]

I'm not sure but it seems that on the changed lines you replaced space indentation with tabs.

[bz]

In D42027#958534, @pstef wrote:

I'm not sure but it seems that on the changed lines you replaced space indentation with tabs.

Yes, there seems to be copy & pasted code around which lacks tab indentation.

[kp]

sys/netlink/route/iface.c

328

ifa = ifa?

Also, I think this should be structured as:

struct ifaddr *ifa = CK_STAILQ_FIRST(...);
if (ifa != NULL)
 ....

because the entry might be removed between the !EMTPY test and the FIRST() retrieval. The epoch system will ensure that the object itself is not deleted until we're done with it, but there's no guarantee that it'll remain in the list during the epoch.

[mjg]

sys/netlink/route/iface.c

328

grep shows:

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);

this works thanks to the lock, but perhaps would be best to restructure to avoid giving people ideas of the sort

[kp]

sys/netlink/route/iface.c

328

Isn't the IF_ADDR_WLOCK() there only to ensure there are no simultaneous modifications to ifp->if_addrhead (as CK_STAILQ requires)?

We don't hold the lock here, so we're still at risk of having the entry removed while we're doing work here.

(And ifa_free() does NET_EPOCH_CALL(), it doesn't directly free the entry.)

[bz]

Update based on comments from kp@

(Historically) if_addr is always the first, so ifa == ifa.

[mjg]

sys/netlink/route/iface.c

328

for example:

IF_ADDR_WLOCK(ifp);
ifa = CK_STAILQ_FIRST(&ifp->if_addrhead);
if (ifa == NULL) {
        IF_ADDR_WUNLOCK(ifp);
} else {
        CK_STAILQ_REMOVE(&ifp->if_addrhead, ifa, ifaddr, ifa_link);
        IF_ADDR_WUNLOCK(ifp);
        ifa_free(ifa);
}

328

i'm rather confused by this response.

i'm proposing removal of a sample which rolls with STAILQ_EMPTY + STAILQ_FIRST, so that bugs like the one initially present in the patch under review would be avoided

[kp]

sys/netlink/route/iface.c

328

Oh, I see. I had misunderstood your first comment.

Yes, it might be better to rework that too, but that's not blocking for this patch.