Rename IFF_RUNNING to IFF_DRV_RUNNING, IFF_OACTIVE to IFF_DRV_OACTIVE,

and move both flags from ifnet.if_flags to ifnet.if_drv_flags, making
and documenting the locking of these flags the responsibility of the
device driver, not the network stack.  The flags for these two fields
will be mutually exclusive so that they can be exposed to user space as
though they were stored in the same variable.

Provide #defines to provide the old names #ifndef _KERNEL, so that user
applications (such as ifconfig) can use the old flag names.  Using the
old names in a device driver will result in a compile error in order to
help device driver writers adopt the new model.

When exposing the interface flags to user space, via interface ioctls
or routing sockets, or the two fields together.  Since the driver flags
cannot currently be set for user space, no new logic is currently
required to handle this case.

Add some assertions that general purpose network stack routines, such
as if_setflags(), are not improperly used on driver-owned flags.

With this change, a large number of very minor network stack races are
closed, subject to correct device driver locking.  Most were likely
never triggered.

Driver sweep to follow; many thanks to pjd and bz for the line-by-line
review they gave this patch.

Reviewed by:	pjd, bz
MFC after:	7 days

4 files changed, 55 insertions, 11 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index f51759b..e88b2bc 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1029,6 +1029,8 @@ if_unroute(struct ifnet *ifp, int flag, int fam)
 {
 	struct ifaddr *ifa;
 
+	KASSERT(flag == IFF_UP, ("if_unroute: flag != IFF_UP"));
+
 	ifp->if_flags &= ~flag;
 	getmicrotime(&ifp->if_lastchange);
 	TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link)
@@ -1052,6 +1054,8 @@ if_route(struct ifnet *ifp, int flag, int fam)
 {
 	struct ifaddr *ifa;
 
+	KASSERT(flag == IFF_UP, ("if_route: flag != IFF_UP"));
+
 	ifp->if_flags |= flag;
 	getmicrotime(&ifp->if_lastchange);
 	TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link)
@@ -1229,7 +1233,7 @@ ifhwioctl(u_long cmd, struct ifnet *ifp, caddr_t data, struct thread *td)
 	struct ifreq *ifr;
 	struct ifstat *ifs;
 	int error = 0;
-	int new_flags;
+	int new_flags, temp_flags;
 	size_t namelen, onamelen;
 	char new_name[IFNAMSIZ];
 	struct ifaddr *ifa;
@@ -1242,8 +1246,9 @@ ifhwioctl(u_long cmd, struct ifnet *ifp, caddr_t data, struct thread *td)
 		break;
 
 	case SIOCGIFFLAGS:
-		ifr->ifr_flags = ifp->if_flags & 0xffff;
-		ifr->ifr_flagshigh = ifp->if_flags >> 16;
+		temp_flags = ifp->if_flags | ifp->if_drv_flags;
+		ifr->ifr_flags = temp_flags & 0xffff;
+		ifr->ifr_flagshigh = temp_flags >> 16;
 		break;
 
 	case SIOCGIFCAP:
@@ -1273,6 +1278,10 @@ ifhwioctl(u_long cmd, struct ifnet *ifp, caddr_t data, struct thread *td)
 		error = suser(td);
 		if (error)
 			return (error);
+		/*
+		 * Currently, no driver owned flags pass the IFF_CANTCHANGE
+		 * check, so we don't need special handling here yet.
+		 */
 		new_flags = (ifr->ifr_flags & 0xffff) |
 		    (ifr->ifr_flagshigh << 16);
 		if (ifp->if_flags & IFF_SMART) {
@@ -1610,10 +1619,12 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct thread *td)
 }
 
 /*
- * The code common to hadling reference counted flags,
+ * The code common to handling reference counted flags,
  * e.g., in ifpromisc() and if_allmulti().
  * The "pflag" argument can specify a permanent mode flag,
  * such as IFF_PPROMISC for promiscuous mode; should be 0 if none.
+ *
+ * Only to be used on stack-owned flags, not driver-owned flags.
  */
 static int
 if_setflag(struct ifnet *ifp, int flag, int pflag, int *refcount, int onswitch)
@@ -1622,6 +1633,9 @@ if_setflag(struct ifnet *ifp, int flag, int pflag, int *refcount, int onswitch)
 	int error;
 	int oldflags, oldcount;
 
+	KASSERT((flag & (IFF_DRV_OACTIVE|IFF_DRV_RUNNING)) == 0,
+	    ("if_setflag: setting driver-ownded flag %d", flag));
+
 	/* Sanity checks to catch programming errors */
 	if (onswitch) {
 		if (*refcount < 0) {
@@ -2258,7 +2272,7 @@ if_handoff(struct ifqueue *ifq, struct mbuf *m, struct ifnet *ifp, int adjust)
 		ifp->if_obytes += m->m_pkthdr.len + adjust;
 		if (m->m_flags & (M_BCAST|M_MCAST))
 			ifp->if_omcasts++;
-		active = ifp->if_flags & IFF_OACTIVE;
+		active = ifp->if_drv_flags & IFF_DRV_OACTIVE;
 	}
 	_IF_ENQUEUE(ifq, m);
 	IF_UNLOCK(ifq);
diff --git a/sys/net/if.h b/sys/net/if.h
index 39bff63..2227a97 100644
--- a/sys/net/if.h
+++ b/sys/net/if.h
@@ -109,17 +109,33 @@ struct if_data {
 	struct	timeval ifi_lastchange;	/* time of last administrative change */
 };
 
+/*
+ * Interface flags are of two types: network stack owned flags, and driver
+ * owned flags.  Historically, these values were stored in the same ifnet
+ * flags field, but with the advent of fine-grained locking, they have been
+ * broken out such that the network stack is responsible for synchronizing
+ * the stack-owned fields, and the device driver the device-owned fields.
+ * Both halves can perform lockless reads of the other half's field, subject
+ * to accepting the involved races.
+ *
+ * Both sets of flags come from the same number space, and should not be
+ * permitted to conflict, as they are exposed to user space via a single
+ * field.
+ *
+ * For historical reasons, the old flag names for driver flags are exposed to
+ * user space.
+ */
 #define	IFF_UP		0x1		/* interface is up */
 #define	IFF_BROADCAST	0x2		/* broadcast address valid */
 #define	IFF_DEBUG	0x4		/* turn on debugging */
 #define	IFF_LOOPBACK	0x8		/* is a loopback net */
 #define	IFF_POINTOPOINT	0x10		/* interface is point-to-point link */
 #define	IFF_SMART	0x20		/* interface manages own routes */
-#define	IFF_RUNNING	0x40		/* resources allocated */
+#define	IFF_DRV_RUNNING	0x40		/* resources allocated */
 #define	IFF_NOARP	0x80		/* no address resolution protocol */
 #define	IFF_PROMISC	0x100		/* receive all packets */
 #define	IFF_ALLMULTI	0x200		/* receive all multicast packets */
-#define	IFF_OACTIVE	0x400		/* tx hardware queue is full */
+#define	IFF_DRV_OACTIVE	0x400		/* tx hardware queue is full */
 #define	IFF_SIMPLEX	0x800		/* can't hear own transmissions */
 #define	IFF_LINK0	0x1000		/* per link layer defined bit */
 #define	IFF_LINK1	0x2000		/* per link layer defined bit */
@@ -132,9 +148,18 @@ struct if_data {
 #define	IFF_STATICARP	0x80000		/* static ARP */
 #define	IFF_NEEDSGIANT	0x100000	/* hold Giant over if_start calls */
 
+/*
+ * Old names for driver flags so that user space tools can continue to use
+ * the old names.
+ */
+#ifndef _KERNEL
+#define	IFF_RUNNING	IFF_DRV_RUNNING
+#define	IFF_OACTIVE	IFF_DRV_OACTIVE
+#endif
+
 /* flags set internally only: */
 #define	IFF_CANTCHANGE \
-	(IFF_BROADCAST|IFF_POINTOPOINT|IFF_RUNNING|IFF_OACTIVE|\
+	(IFF_BROADCAST|IFF_POINTOPOINT|IFF_DRV_RUNNING|IFF_DRV_OACTIVE|\
 	    IFF_SIMPLEX|IFF_MULTICAST|IFF_ALLMULTI|IFF_SMART|IFF_PROMISC|\
 	    IFF_POLLING)
 
diff --git a/sys/net/if_var.h b/sys/net/if_var.h
index 6b3c56c..1f0431e 100644
--- a/sys/net/if_var.h
+++ b/sys/net/if_var.h
@@ -422,6 +422,10 @@ do {									\
 #define	IFQ_INC_DROPS(ifq)		((ifq)->ifq_drops++)
 #define	IFQ_SET_MAXLEN(ifq, len)	((ifq)->ifq_maxlen = (len))
 
+/*
+ * The IFF_DRV_OACTIVE test should really occur in the device driver, not in
+ * the handoff logic, as that flag is locked by the device driver.
+ */
 #define	IFQ_HANDOFF_ADJ(ifp, m, adj, err)				\
 do {									\
 	int len;							\
@@ -434,7 +438,7 @@ do {									\
 		(ifp)->if_obytes += len + (adj);			\
 		if (mflags & M_MCAST)					\
 			(ifp)->if_omcasts++;				\
-		if (((ifp)->if_flags & IFF_OACTIVE) == 0)		\
+		if (((ifp)->if_drv_flags & IFF_DRV_OACTIVE) == 0)	\
 			if_start(ifp);					\
 	}								\
 } while (0)
diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
index a1e759b..f014af1 100644
--- a/sys/net/rtsock.c
+++ b/sys/net/rtsock.c
@@ -859,7 +859,7 @@ rt_ifmsg(struct ifnet *ifp)
 		return;
 	ifm = mtod(m, struct if_msghdr *);
 	ifm->ifm_index = ifp->if_index;
-	ifm->ifm_flags = ifp->if_flags;
+	ifm->ifm_flags = ifp->if_flags | ifp->if_drv_flags;
 	ifm->ifm_data = ifp->if_data;
 	ifm->ifm_addrs = 0;
 	rt_dispatch(m, NULL);
@@ -1124,7 +1124,7 @@ sysctl_iflist(int af, struct walkarg *w)
 
 			ifm = (struct if_msghdr *)w->w_tmem;
 			ifm->ifm_index = ifp->if_index;
-			ifm->ifm_flags = ifp->if_flags;
+			ifm->ifm_flags = ifp->if_flags | ifp->if_drv_flags;
 			ifm->ifm_data = ifp->if_data;
 			ifm->ifm_addrs = info.rti_addrs;
 			error = SYSCTL_OUT(w->w_req,(caddr_t)ifm, len);
@@ -1178,6 +1178,7 @@ sysctl_ifmalist(int af, struct walkarg *w)
 			continue;
 		ifa = ifaddr_byindex(ifp->if_index);
 		info.rti_info[RTAX_IFP] = ifa ? ifa->ifa_addr : NULL;
+
 		TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
 			if (af && af != ifma->ifma_addr->sa_family)
 				continue;