Start to address a number of races relating to use of ifnet pointers

after the corresponding interface has been destroyed:

(1) Add an ifnet refcount, ifp->if_refcount.  Initialize it to 1 in
    if_alloc(), and modify if_free_type() to decrement and check the
    refcount.

(2) Add new if_ref() and if_rele() interfaces to allow kernel code
    walking global interface lists to release IFNET_[RW]LOCK() yet
    keep the ifnet stable.  Currently, if_rele() is a no-op wrapper
    around if_free(), but this may change in the future.

(3) Add new ifnet field, if_alloctype, which caches the type passed
    to if_alloc(), but unlike if_type, won't be changed by drivers.
    This allows asynchronous free's of the interface after the
    driver has released it to still use the right type.  Use that
    instead of the type passed to if_free_type(), but assert that
    they are the same (might have to rethink this if that doesn't
    work out).

(4) Add a new ifnet_byindex_ref(), which looks up an interface by
    index and returns a reference rather than a pointer to it.

(5) Fix if_alloc() to fully initialize the if_addr_mtx before hooking
    up the ifnet to global lists.

(6) Modify sysctls in if_mib.c to use ifnet_byindex_ref() and release
    the ifnet when done.

When this change is MFC'd, it will need to replace if_ispare fields
rather than adding new fields in order to avoid breaking the binary
interface.  Once this change is MFC'd, if_free_type() should be
removed, as its 'type' argument is now optional.

This refcount is not appropriate for counting mbuf pkthdr references,
and also not for counting entry into the device driver via ifnet
function pointers.  An rmlock may be appropriate for the latter.
Rather, this is about ensuring data structure stability when reaching
an ifnet via global ifnet lists and tables followed by copy in or out
of userspace.

MFC after:      3 weeks
Reported by:    mdtancsa
Reviewed by:    brooks

1 files changed, 54 insertions, 13 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index 457b763..4d4b676 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -52,6 +52,7 @@
 #include <sys/protosw.h>
 #include <sys/kernel.h>
 #include <sys/lock.h>
+#include <sys/refcount.h>
 #include <sys/rwlock.h>
 #include <sys/sockio.h>
 #include <sys/syslog.h>
@@ -204,19 +205,35 @@ static struct ifnet *
 ifnet_byindex_locked(u_short idx)
 {
 	INIT_VNET_NET(curvnet);
+
+	if (idx > V_if_index)
+		return (NULL);
+	return (V_ifindex_table[idx].ife_ifnet);
+}
+
+struct ifnet *
+ifnet_byindex(u_short idx)
+{
 	struct ifnet *ifp;
 
-	ifp = V_ifindex_table[idx].ife_ifnet;
+	IFNET_RLOCK();
+	ifp = ifnet_byindex_locked(idx);
+	IFNET_RUNLOCK();
 	return (ifp);
 }
 
 struct ifnet *
-ifnet_byindex(u_short idx)
+ifnet_byindex_ref(u_short idx)
 {
 	struct ifnet *ifp;
 
 	IFNET_RLOCK();
 	ifp = ifnet_byindex_locked(idx);
+	if (ifp == NULL) {
+		IFNET_RUNLOCK();
+		return (NULL);
+	}
+	if_ref(ifp);
 	IFNET_RUNLOCK();
 	return (ifp);
 }
@@ -490,6 +507,7 @@ if_alloc(u_char type)
 		if_grow();
 
 	ifp->if_type = type;
+	ifp->if_alloctype = type;
 
 	if (if_com_alloc[type] != NULL) {
 		ifp->if_l2com = if_com_alloc[type](type, ifp);
@@ -498,11 +516,12 @@ if_alloc(u_char type)
 			return (NULL);
 		}
 	}
+
+	IF_ADDR_LOCK_INIT(ifp);
+	refcount_init(&ifp->if_refcount, 1);	/* Index reference. */
 	IFNET_WLOCK();
 	ifnet_setbyindex(ifp->if_index, ifp);
 	IFNET_WUNLOCK();
-	IF_ADDR_LOCK_INIT(ifp);
-
 	return (ifp);
 }
 
@@ -516,7 +535,7 @@ void
 if_free(struct ifnet *ifp)
 {
 
-	if_free_type(ifp, ifp->if_type);
+	if_free_type(ifp, ifp->if_alloctype);
 }
 
 /*
@@ -529,28 +548,50 @@ if_free_type(struct ifnet *ifp, u_char type)
 {
 	INIT_VNET_NET(curvnet); /* ifp->if_vnet can be NULL here ! */
 
-	if (ifp != ifnet_byindex(ifp->if_index)) {
-		if_printf(ifp, "%s: value was not if_alloced, skipping\n",
-		    __func__);
+	/*
+	 * Some drivers modify if_type, so we can't rely on it being the
+	 * same in free as it was in alloc.  Now that we have if_alloctype,
+	 * we should just use that, but drivers expect to pass a type.
+	 */
+	KASSERT(ifp->if_alloctype == type,
+	    ("if_free_type: type (%d) != alloctype (%d)", type,
+	    ifp->if_alloctype));
+
+	if (!refcount_release(&ifp->if_refcount))
 		return;
-	}
 
 	IFNET_WLOCK();
+	KASSERT(ifp == ifnet_byindex_locked(ifp->if_index),
+	    ("%s: freeing unallocated ifnet", ifp->if_xname));
 	ifnet_setbyindex(ifp->if_index, NULL);
-
-	/* XXX: should be locked with if_findindex() */
 	while (V_if_index > 0 && ifnet_byindex_locked(V_if_index) == NULL)
 		V_if_index--;
 	IFNET_WUNLOCK();
 
-	if (if_com_free[type] != NULL)
-		if_com_free[type](ifp->if_l2com, type);
+	if (if_com_free[ifp->if_alloctype] != NULL)
+		if_com_free[ifp->if_alloctype](ifp->if_l2com,
+		    ifp->if_alloctype);
 
 	IF_ADDR_LOCK_DESTROY(ifp);
 	free(ifp, M_IFNET);
 }
 
 void
+if_ref(struct ifnet *ifp)
+{
+
+	/* We don't assert the ifnet list lock here, but arguably should. */
+	refcount_acquire(&ifp->if_refcount);
+}
+
+void
+if_rele(struct ifnet *ifp)
+{
+
+	if_free(ifp);
+}
+
+void
 ifq_attach(struct ifaltq *ifq, struct ifnet *ifp)
 {