Document the various flavors of NET_LOCK() and rename the reader version.

Since our last concurrency mistake only ioctl(2) ans sysctl(2) code path
take the reader lock.  This is mostly for documentation purpose as long as
the softnet thread is converted back to use a read lock.

dlg@ said that comments should be good enough.

ok sashan@

8 files changed, 62 insertions, 53 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index b3a4e857598..aaf7e032a0c 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if.c,v 1.604 2020/05/26 14:58:55 visa Exp $	*/
+/*	$OpenBSD: if.c,v 1.605 2020/05/27 11:19:28 mpi Exp $	*/
 /*	$NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $	*/
 
 /*
@@ -937,7 +937,7 @@ if_input_process(struct ifnet *ifp, struct mbuf_list *ml)
 	 *
 	 * Since we have a NET_LOCK() we also use it to serialize access
 	 * to PF globals, pipex globals, unicast and multicast addresses
-	 * lists.
+	 * lists and the socket layer.
 	 */
 	NET_LOCK();
 	while ((m = ml_dequeue(ml)) != NULL)
@@ -2339,27 +2339,27 @@ ifioctl_get(u_long cmd, caddr_t data)
 
 	switch(cmd) {
 	case SIOCGIFCONF:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		error = ifconf(data);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		return (error);
 	case SIOCIFGCLONERS:
 		error = if_clone_list((struct if_clonereq *)data);
 		return (error);
 	case SIOCGIFGMEMB:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		error = if_getgroupmembers(data);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		return (error);
 	case SIOCGIFGATTR:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		error = if_getgroupattribs(data);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		return (error);
 	case SIOCGIFGLIST:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		error = if_getgrouplist(data);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		return (error);
 	}
 
@@ -2367,7 +2367,7 @@ ifioctl_get(u_long cmd, caddr_t data)
 	if (ifp == NULL)
 		return (ENXIO);
 
-	NET_RLOCK();
+	NET_RLOCK_IN_IOCTL();
 
 	switch(cmd) {
 	case SIOCGIFFLAGS:
@@ -2435,7 +2435,7 @@ ifioctl_get(u_long cmd, caddr_t data)
 		panic("invalid ioctl %lu", cmd);
 	}
 
-	NET_RUNLOCK();
+	NET_RUNLOCK_IN_IOCTL();
 
 	return (error);
 }
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 58480308bf2..931bdcb98cd 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf_ioctl.c,v 1.351 2020/04/19 22:31:06 sashan Exp $ */
+/*	$OpenBSD: pf_ioctl.c,v 1.352 2020/05/27 11:19:28 mpi Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -2924,12 +2924,12 @@ pf_sysctl(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
 {
 	struct pf_status	pfs;
 
-	NET_LOCK();
+	NET_RLOCK_IN_IOCTL();
 	PF_LOCK();
 	memcpy(&pfs, &pf_status, sizeof(struct pf_status));
 	pfi_update_status(pfs.ifname, &pfs);
 	PF_UNLOCK();
-	NET_UNLOCK();
+	NET_RUNLOCK_IN_IOCTL();
 
 	return sysctl_rdstruct(oldp, oldlenp, newp, &pfs, sizeof(pfs));
 }
diff --git a/sys/netinet/in.c b/sys/netinet/in.c
index b494b72e844..a7c3b7724b7 100644
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: in.c,v 1.169 2020/03/15 05:34:13 visa Exp $	*/
+/*	$OpenBSD: in.c,v 1.170 2020/05/27 11:19:28 mpi Exp $	*/
 /*	$NetBSD: in.c,v 1.26 1996/02/13 23:41:39 christos Exp $	*/
 
 /*
@@ -569,7 +569,7 @@ in_ioctl_get(u_long cmd, caddr_t data, struct ifnet *ifp)
 			return (error);
 	}
 
-	NET_RLOCK();
+	NET_RLOCK_IN_IOCTL();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
 		if (ifa->ifa_addr->sa_family != AF_INET)
@@ -620,7 +620,7 @@ in_ioctl_get(u_long cmd, caddr_t data, struct ifnet *ifp)
 	}
 
 err:
-	NET_RUNLOCK();
+	NET_RUNLOCK_IN_IOCTL();
 	return (error);
 }
 
diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.c
index c6aae18b8db..3cefb2a2596 100644
--- a/sys/netinet/ip_mroute.c
+++ b/sys/netinet/ip_mroute.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_mroute.c,v 1.129 2020/03/15 05:34:13 visa Exp $	*/
+/*	$OpenBSD: ip_mroute.c,v 1.130 2020/05/27 11:19:29 mpi Exp $	*/
 /*	$NetBSD: ip_mroute.c,v 1.85 2004/04/26 01:31:57 matt Exp $	*/
 
 /*
@@ -267,16 +267,16 @@ mrt_ioctl(struct socket *so, u_long cmd, caddr_t data)
 	else
 		switch (cmd) {
 		case SIOCGETVIFCNT:
-			NET_RLOCK();
+			NET_RLOCK_IN_IOCTL();
 			error = get_vif_cnt(inp->inp_rtableid,
 			    (struct sioc_vif_req *)data);
-			NET_RUNLOCK();
+			NET_RUNLOCK_IN_IOCTL();
 			break;
 		case SIOCGETSGCNT:
-			NET_RLOCK();
+			NET_RLOCK_IN_IOCTL();
 			error = get_sg_cnt(inp->inp_rtableid,
 			    (struct sioc_sg_req *)data);
-			NET_RUNLOCK();
+			NET_RUNLOCK_IN_IOCTL();
 			break;
 		default:
 			error = ENOTTY;
diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c
index 2bd9a74b8ae..ca8c78c7b9f 100644
--- a/sys/netinet6/in6.c
+++ b/sys/netinet6/in6.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: in6.c,v 1.235 2020/03/15 05:34:14 visa Exp $	*/
+/*	$OpenBSD: in6.c,v 1.236 2020/05/27 11:19:29 mpi Exp $	*/
 /*	$KAME: in6.c,v 1.372 2004/06/14 08:14:21 itojun Exp $	*/
 
 /*
@@ -412,7 +412,7 @@ in6_ioctl_get(u_long cmd, caddr_t data, struct ifnet *ifp)
 			return (error);
 	}
 
-	NET_RLOCK();
+	NET_RLOCK_IN_IOCTL();
 
 	if (sa6 != NULL) {
 		error = in6_check_embed_scope(sa6, ifp->if_index);
@@ -506,7 +506,7 @@ in6_ioctl_get(u_long cmd, caddr_t data, struct ifnet *ifp)
 	}
 
 err:
-	NET_RUNLOCK();
+	NET_RUNLOCK_IN_IOCTL();
 	return (error);
 }
 
diff --git a/sys/netinet6/ip6_mroute.c b/sys/netinet6/ip6_mroute.c
index 9203f5a8c80..ee14687b2ce 100644
--- a/sys/netinet6/ip6_mroute.c
+++ b/sys/netinet6/ip6_mroute.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip6_mroute.c,v 1.123 2020/03/15 05:34:14 visa Exp $	*/
+/*	$OpenBSD: ip6_mroute.c,v 1.124 2020/05/27 11:19:29 mpi Exp $	*/
 /*	$NetBSD: ip6_mroute.c,v 1.59 2003/12/10 09:28:38 itojun Exp $	*/
 /*	$KAME: ip6_mroute.c,v 1.45 2001/03/25 08:38:51 itojun Exp $	*/
 
@@ -250,16 +250,16 @@ mrt6_ioctl(struct socket *so, u_long cmd, caddr_t data)
 
 	switch (cmd) {
 	case SIOCGETSGCNT_IN6:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		error = get_sg6_cnt((struct sioc_sg_req6 *)data,
 		    inp->inp_rtableid);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		break;
 	case SIOCGETMIFCNT_IN6:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		error = get_mif6_cnt((struct sioc_mif_req6 *)data,
 		    inp->inp_rtableid);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		break;
 	default:
 		error = ENOTTY;
diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index 2b90caa37c7..118afe3c916 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nd6.c,v 1.231 2020/04/22 07:51:38 mpi Exp $	*/
+/*	$OpenBSD: nd6.c,v 1.232 2020/05/27 11:19:29 mpi Exp $	*/
 /*	$KAME: nd6.c,v 1.280 2002/06/08 19:52:07 itojun Exp $	*/
 
 /*
@@ -1022,9 +1022,9 @@ nd6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp)
 
 	switch (cmd) {
 	case SIOCGIFINFO_IN6:
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		ndi->ndi = *ND_IFINFO(ifp);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		return (0);
 	case SIOCGNBRINFO_IN6:
 	{
@@ -1032,7 +1032,7 @@ nd6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp)
 		struct in6_addr nb_addr = nbi->addr; /* make local for safety */
 		time_t expire;
 
-		NET_RLOCK();
+		NET_RLOCK_IN_IOCTL();
 		/*
 		 * XXX: KAME specific hack for scoped addresses
 		 *      XXXX: for other scopes than link-local?
@@ -1049,7 +1049,7 @@ nd6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp)
 		if (rt == NULL ||
 		    (ln = (struct llinfo_nd6 *)rt->rt_llinfo) == NULL) {
 			rtfree(rt);
-			NET_RUNLOCK();
+			NET_RUNLOCK_IN_IOCTL();
 			return (EINVAL);
 		}
 		expire = ln->ln_rt->rt_expire;
@@ -1064,7 +1064,7 @@ nd6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp)
 		nbi->expire = expire;
 
 		rtfree(rt);
-		NET_RUNLOCK();
+		NET_RUNLOCK_IN_IOCTL();
 		return (0);
 	}
 	}
diff --git a/sys/sys/systm.h b/sys/sys/systm.h
index 5dec67280de..814f9d58cc2 100644
--- a/sys/sys/systm.h
+++ b/sys/sys/systm.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: systm.h,v 1.145 2020/03/20 03:37:08 cheloha Exp $	*/
+/*	$OpenBSD: systm.h,v 1.146 2020/05/27 11:19:29 mpi Exp $	*/
 /*	$NetBSD: systm.h,v 1.50 1996/06/09 04:55:09 briggs Exp $	*/
 
 /*-
@@ -315,31 +315,40 @@ int	uiomove(void *, size_t, struct uio *);
 
 extern struct rwlock netlock;
 
-#define	NET_LOCK()		NET_WLOCK()
-#define	NET_UNLOCK()		NET_WUNLOCK()
-#define	NET_ASSERT_UNLOCKED()	NET_ASSERT_WUNLOCKED()
+/*
+ * Network stack data structures are, unless stated otherwise, protected
+ * by the NET_LOCK().  It's a single non-recursive lock for the whole
+ * subsystem.
+ */
+#define	NET_LOCK()	do { rw_enter_write(&netlock); } while (0)
+#define	NET_UNLOCK()	do { rw_exit_write(&netlock); } while (0)
 
+/*
+ * Reader version of NET_LOCK() to be used in "softnet" thread only.
 
-#define	NET_WLOCK()	do { rw_enter_write(&netlock); } while (0)
-#define	NET_WUNLOCK()	do { rw_exit_write(&netlock); } while (0)
+ * The "softnet" thread should be the only thread processing packets
+ * without holding an exclusive lock.  This is done to allow read-only
+ * ioctl(2) to not block.
+ */
+#define	NET_RLOCK_IN_SOFTNET()	do { rw_enter_read(&netlock); } while (0)
+#define	NET_RUNLOCK_IN_SOFTNET()do { rw_exit_read(&netlock); } while (0)
 
-#define	NET_ASSERT_WLOCKED()						\
-do {									\
-	int _s = rw_status(&netlock);					\
-	if ((splassert_ctl > 0) && (_s != RW_WRITE))			\
-		splassert_fail(RW_WRITE, _s, __func__);			\
-} while (0)
+/*
+ * Reader version of NET_LOCK() to be used in ioctl/sysctl path only.
+ *
+ * Can be grabbed instead of the exclusive version when no field
+ * protected by the NET_LOCK() is modified by the ioctl/sysctl.
+ */
+#define	NET_RLOCK_IN_IOCTL()	do { rw_enter_read(&netlock); } while (0)
+#define	NET_RUNLOCK_IN_IOCTL()	do { rw_exit_read(&netlock); } while (0)
 
-#define	NET_ASSERT_WUNLOCKED()						\
+#define	NET_ASSERT_UNLOCKED()						\
 do {									\
 	int _s = rw_status(&netlock);					\
 	if ((splassert_ctl > 0) && (_s == RW_WRITE))			\
 		splassert_fail(0, RW_WRITE, __func__);			\
 } while (0)
 
-#define	NET_RLOCK()	do { rw_enter_read(&netlock); } while (0)
-#define	NET_RUNLOCK()	do { rw_exit_read(&netlock); } while (0)
-
 #define	NET_ASSERT_LOCKED()						\
 do {									\
 	int _s = rw_status(&netlock);					\