Introduce locking around use of ifindex_table, whose use was previously

unsynchronized.  While races were extremely rare, we've now had a
couple of reports of panics in environments involving large numbers of
IPSEC tunnels being added very quickly on an active system.

- Add accessor functions ifnet_byindex(), ifaddr_byindex(),
  ifdev_byindex() to replace existing accessor macros.  These functions
  now acquire the ifnet lock before derefencing the table.
- Add IFNET_WLOCK_ASSERT().
- Add static accessor functions ifnet_setbyindex(), ifdev_setbyindex(),
  which set values in the table either asserting of acquiring the ifnet
  lock.
- Use accessor functions throughout if.c to modify and read
  ifindex_table.
- Rework ifnet attach/detach to lock around ifindex_table modification.

Note that these changes simply close races around use of ifindex_table,
and make no attempt to solve the probem of disappearing ifnets.  Further
refinement of this work, including with respect to ifindex_table
resizing, is still required.

In a future change, the ifnet lock should be converted from a mutex to an
rwlock in order to reduce contention.

Reviewed and tested by:	brooks

1 files changed, 69 insertions, 12 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index f124e7e..afc8243 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -133,7 +133,6 @@ extern void	nd6_setmtu(struct ifnet *);
 #endif
 
 int	if_index = 0;
-struct	ifindex_entry *ifindex_table = NULL;
 int	ifqmaxlen = IFQ_MAXLEN;
 struct	ifnethead ifnet;	/* depend on static init XXX */
 struct	ifgrouphead ifg_head;
@@ -144,6 +143,11 @@ static	if_com_free_t *if_com_free[256];
 static int	if_indexlim = 8;
 static struct	knlist ifklist;
 
+/*
+ * Table of ifnet/cdev by index.  Locked with ifnet_lock.
+ */
+static struct ifindex_entry *ifindex_table = NULL;
+
 static void	filt_netdetach(struct knote *kn);
 static int	filt_netdev(struct knote *kn, long hint);
 
@@ -160,6 +164,57 @@ MALLOC_DEFINE(M_IFNET, "ifnet", "interface internals");
 MALLOC_DEFINE(M_IFADDR, "ifaddr", "interface address");
 MALLOC_DEFINE(M_IFMADDR, "ether_multi", "link-level multicast address");
 
+struct ifnet *
+ifnet_byindex(u_short idx)
+{
+	struct ifnet *ifp;
+
+	IFNET_RLOCK();
+	ifp = ifindex_table[idx].ife_ifnet;
+	IFNET_RUNLOCK();
+	return (ifp);
+}
+
+static void
+ifnet_setbyindex(u_short idx, struct ifnet *ifp)
+{
+
+	IFNET_WLOCK_ASSERT();
+
+	ifindex_table[idx].ife_ifnet = ifp;
+}
+
+struct ifaddr *
+ifaddr_byindex(u_short idx)
+{
+	struct ifaddr *ifa;
+
+	IFNET_RLOCK();
+	ifa = ifnet_byindex(idx)->if_addr;
+	IFNET_RUNLOCK();
+	return (ifa);
+}
+
+struct cdev *
+ifdev_byindex(u_short idx)
+{
+	struct cdev *cdev;
+
+	IFNET_RLOCK();
+	cdev = ifindex_table[idx].ife_dev;
+	IFNET_RUNLOCK();
+	return (cdev);
+}
+
+static void
+ifdev_setbyindex(u_short idx, struct cdev *cdev)
+{
+
+	IFNET_WLOCK();
+	ifindex_table[idx].ife_dev = cdev;
+	IFNET_WUNLOCK();
+}
+
 static d_open_t		netopen;
 static d_close_t	netclose;
 static d_ioctl_t	netioctl;
@@ -298,8 +353,8 @@ if_init(void *dummy __unused)
 	TAILQ_INIT(&ifg_head);
 	knlist_init(&ifklist, NULL, NULL, NULL, NULL);
 	if_grow();				/* create initial table */
-	ifdev_byindex(0) = make_dev(&net_cdevsw, 0,
-	    UID_ROOT, GID_WHEEL, 0600, "network");
+	ifdev_setbyindex(0, make_dev(&net_cdevsw, 0, UID_ROOT, GID_WHEEL,
+	    0600, "network"));
 	if_clone_init();
 }
 
@@ -360,7 +415,9 @@ if_alloc(u_char type)
 			return (NULL);
 		}
 	}
-	ifnet_byindex(ifp->if_index) = ifp;
+	IFNET_WLOCK();
+	ifnet_setbyindex(ifp->if_index, ifp);
+	IFNET_WUNLOCK();
 	IF_ADDR_LOCK_INIT(ifp);
 
 	return (ifp);
@@ -394,17 +451,18 @@ if_free_type(struct ifnet *ifp, u_char type)
 		return;
 	}
 
-	IF_ADDR_LOCK_DESTROY(ifp);
-
-	ifnet_byindex(ifp->if_index) = NULL;
+	IFNET_WLOCK();
+	ifnet_setbyindex(ifp->if_index, NULL);
 
 	/* XXX: should be locked with if_findindex() */
 	while (if_index > 0 && ifnet_byindex(if_index) == NULL)
 		if_index--;
+	IFNET_WUNLOCK();
 
 	if (if_com_free[type] != NULL)
 		if_com_free[type](ifp->if_l2com, type);
 
+	IF_ADDR_LOCK_DESTROY(ifp);
 	free(ifp, M_IFNET);
 };
 
@@ -454,10 +512,9 @@ if_attach(struct ifnet *ifp)
 	mac_ifnet_create(ifp);
 #endif
 
-	ifdev_byindex(ifp->if_index) = make_dev(&net_cdevsw,
-	    unit2minor(ifp->if_index),
-	    UID_ROOT, GID_WHEEL, 0600, "%s/%s",
-	    net_cdevsw.d_name, ifp->if_xname);
+	ifdev_setbyindex(ifp->if_index, make_dev(&net_cdevsw,
+	    unit2minor(ifp->if_index), UID_ROOT, GID_WHEEL, 0600, "%s/%s",
+	    net_cdevsw.d_name, ifp->if_xname));
 	make_dev_alias(ifdev_byindex(ifp->if_index), "%s%d",
 	    net_cdevsw.d_name, ifp->if_index);
 
@@ -706,7 +763,7 @@ if_detach(struct ifnet *ifp)
 	 */
 	ifp->if_addr = NULL;
 	destroy_dev(ifdev_byindex(ifp->if_index));
-	ifdev_byindex(ifp->if_index) = NULL;
+	ifdev_setbyindex(ifp->if_index, NULL);	
 
 	/* We can now free link ifaddr. */
 	if (!TAILQ_EMPTY(&ifp->if_addrhead)) {