Use the IPX PCB list mutex and IPX PCB mutexes to lock down the SPX

portion of IPX/SPX:

- Protect IPX PCB lists with the IPX PCB list mutex, in particular
  when calling PCB and PCB list manipulation routines in ipx_pcb.c.
- Protect both IPX PCB state and SPX PCB state using the IPX PCB
  mutex.
- Generally annotate locking, as well as adding liberal use of lock
  assertions to document locking requirements.
- Where possible, use unlocked reads when reading integer or smaller
  sized socket options on SPX sockets.
- De-spl throughout.

Notes:

- spx_input() expects both the list mutex and PCB mutex to be held
  on entry, but will release both on return.  Because sonewconn() is
  called from spx_input(), it may actually drop one PCB lock and
  acquire another during generation of a new connection, meaning the
  caller is not in a position to unlock the PCB mutex.

MFC after:	3 weeks

2 files changed, 110 insertions, 39 deletions

diff --git a/sys/netipx/ipx_input.c b/sys/netipx/ipx_input.c
index ee1ca23..53d8d9a 100644
--- a/sys/netipx/ipx_input.c
+++ b/sys/netipx/ipx_input.c
@@ -277,8 +277,9 @@ ours:
 		if ((ipxp->ipxp_flags & IPXP_ALL_PACKETS) == 0)
 			switch (ipx->ipx_pt) {
 			case IPXPROTO_SPX:
+				IPX_LOCK(ipxp);
+				/* Will release both locks. */
 				spx_input(m, ipxp);
-				IPX_LIST_UNLOCK();
 				return;
 			}
 		IPX_LOCK(ipxp);
diff --git a/sys/netipx/spx_usrreq.c b/sys/netipx/spx_usrreq.c
index 94e3e72..65901f2 100644
--- a/sys/netipx/spx_usrreq.c
+++ b/sys/netipx/spx_usrreq.c
@@ -165,12 +165,23 @@ spx_input(m, ipxp)
 	spxstat.spxs_rcvtotal++;
 	KASSERT(ipxp != NULL, ("spx_input: NULL ipxpcb"));
 
+	/*
+	 * spx_input() assumes that the caller will hold both the pcb list
+	 * lock and also the ipxp lock.  spx_input() will release both before
+	 * returning, and may in fact trade in the ipxp lock for another pcb
+	 * lock following sonewconn().
+	 */
+	IPX_LIST_LOCK_ASSERT();
+	IPX_LOCK_ASSERT(ipxp);
+
 	cb = ipxtospxpcb(ipxp);
 	if (cb == NULL)
 		goto bad;
 
 	if (m->m_len < sizeof(*si)) {
 		if ((m = m_pullup(m, sizeof(*si))) == NULL) {
+			IPX_UNLOCK(ipxp);
+			IPX_LIST_UNLOCK();
 			spxstat.spxs_rcvshort++;
 			return;
 		}
@@ -205,7 +216,9 @@ spx_input(m, ipxp)
 		 * we're committed to it below in TCPS_LISTEN.
 		 */
 		dropsocket++;
+		IPX_UNLOCK(ipxp);
 		ipxp = (struct ipxpcb *)so->so_pcb;
+		IPX_LOCK(ipxp);
 		ipxp->ipxp_laddr = si->si_dna;
 		cb = ipxtospxpcb(ipxp);
 		cb->s_mtu = ocb->s_mtu;		/* preserve sockopts */
@@ -329,9 +342,12 @@ spx_input(m, ipxp)
 	if (cb->s_force || (cb->s_flags & (SF_ACKNOW|SF_WIN|SF_RXT)))
 		spx_output(cb, NULL);
 	cb->s_flags &= ~(SF_WIN|SF_RXT);
+	IPX_UNLOCK(ipxp);
+	IPX_LIST_UNLOCK();
 	return;
 
 dropwithreset:
+	IPX_UNLOCK(ipxp);
 	if (dropsocket) {
 		struct socket *head;
 		ACCEPT_LOCK();
@@ -346,6 +362,7 @@ dropwithreset:
 		soabort(so);
 		cb = NULL;
 	}
+	IPX_LIST_UNLOCK();
 	si->si_seq = ntohs(si->si_seq);
 	si->si_ack = ntohs(si->si_ack);
 	si->si_alo = ntohs(si->si_alo);
@@ -360,6 +377,8 @@ bad:
 	if (cb == NULL || cb->s_ipxpcb->ipxp_socket->so_options & SO_DEBUG ||
             traceallspxs)
 		spx_trace(SA_DROP, (u_char)ostate, cb, &spx_savesi, 0);
+	IPX_UNLOCK(ipxp);
+	IPX_LIST_UNLOCK();
 	m_freem(m);
 }
 
@@ -382,6 +401,8 @@ register struct spx *si;
 	int incr;
 	char wakeup = 0;
 
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	if (si == SI(0))
 		goto present;
 	/*
@@ -721,6 +742,8 @@ spx_output(cb, m0)
 #endif
 	struct mbuf *mprev;
 
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	if (m0 != NULL) {
 		int mtu = cb->s_mtu;
 		int datalen;
@@ -1120,6 +1143,8 @@ spx_setpersist(cb)
 {
 	register int t = ((cb->s_srtt >> 2) + cb->s_rttvar) >> 1;
 
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	if (cb->s_timer[SPXT_REXMT] && spx_do_persist_panics)
 		panic("spx_output REXMT");
 	/*
@@ -1166,21 +1191,25 @@ spx_ctloutput(so, sopt)
 		case SO_HEADERS_ON_OUTPUT:
 			mask = SF_HO;
 		get_flags:
+			/* Unlocked read. */
 			soptval = cb->s_flags & mask;
 			error = sooptcopyout(sopt, &soptval, sizeof soptval);
 			break;
 
 		case SO_MTU:
+			/* Unlocked read. */
 			usoptval = cb->s_mtu;
 			error = sooptcopyout(sopt, &usoptval, sizeof usoptval);
 			break;
 
 		case SO_LAST_HEADER:
+			/* Unlocked read. */
 			error = sooptcopyout(sopt, &cb->s_rhdr,
 					     sizeof cb->s_rhdr);
 			break;
 
 		case SO_DEFAULT_HEADERS:
+			/* Unlocked read. */
 			error = sooptcopyout(sopt, &cb->s_shdr,
 					     sizeof cb->s_shdr);
 			break;
@@ -1206,12 +1235,14 @@ spx_ctloutput(so, sopt)
 			if (error)
 				break;
 
+			IPX_LOCK(ipxp);
 			if (cb->s_flags & SF_PI) {
 				if (optval)
 					cb->s_flags |= mask;
 				else
 					cb->s_flags &= ~mask;
 			} else error = EINVAL;
+			IPX_UNLOCK(ipxp);
 			break;
 
 		case SO_MTU:
@@ -1219,6 +1250,7 @@ spx_ctloutput(so, sopt)
 					    sizeof usoptval);
 			if (error)
 				break;
+			/* Unlocked write. */
 			cb->s_mtu = usoptval;
 			break;
 
@@ -1228,6 +1260,7 @@ spx_ctloutput(so, sopt)
 					    sizeof optval);
 			if (error)
 				break;
+			IPX_LOCK(ipxp);
 			if (optval) {
 				cb->s_flags2 |= SF_NEWCALL;
 				spx_newchecks[5]++;
@@ -1235,6 +1268,7 @@ spx_ctloutput(so, sopt)
 				cb->s_flags2 &= ~SF_NEWCALL;
 				spx_newchecks[6]++;
 			}
+			IPX_UNLOCK(ipxp);
 			break;
 #endif
 
@@ -1246,8 +1280,10 @@ spx_ctloutput(so, sopt)
 						    sizeof sp);
 				if (error)
 					break;
+				IPX_LOCK(ipxp);
 				cb->s_dt = sp.spx_dt;
 				cb->s_cc = sp.spx_cc & SPX_EM;
+				IPX_UNLOCK(ipxp);
 			}
 			break;
 
@@ -1263,16 +1299,16 @@ static int
 spx_usr_abort(so)
 	struct socket *so;
 {
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
-	s = splnet();
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
 	spx_drop(cb, ECONNABORTED);
-	splx(s);
+	IPX_LIST_UNLOCK();
 	return (0);
 }
 
@@ -1294,8 +1330,10 @@ spx_accept(so, nam)
 	bzero(sipx, sizeof *sipx);
 	sipx->sipx_len = sizeof *sipx;
 	sipx->sipx_family = AF_IPX;
+	IPX_LOCK(ipxp);
 	sipx->sipx_addr = ipxp->ipxp_faddr;
-	*nam = sodupsockaddr((struct sockaddr *)sipx, M_NOWAIT);
+	IPX_UNLOCK(ipxp);
+	*nam = sodupsockaddr((struct sockaddr *)sipx, M_WAITOK);
 	return (0);
 }
 
@@ -1305,19 +1343,18 @@ spx_attach(so, proto, td)
 	int proto;
 	struct thread *td;
 {
-	int error;
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 	struct mbuf *mm;
 	struct sockbuf *sb;
+	int error;
 
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
 	if (ipxp != NULL)
 		return (EISCONN);
-	s = splnet();
+	IPX_LIST_LOCK();
 	error = ipx_pcballoc(so, &ipxpcb_list, td);
 	if (error)
 		goto spx_attach_end;
@@ -1361,7 +1398,7 @@ spx_attach(so, proto, td)
 	    SPXTV_MIN, SPXTV_REXMTMAX);
 	ipxp->ipxp_pcb = (caddr_t)cb;
 spx_attach_end:
-	splx(s);
+	IPX_LIST_UNLOCK();
 	return (error);
 }
 
@@ -1372,10 +1409,16 @@ spx_bind(so, nam, td)
 	struct thread *td;
 {
 	struct ipxpcb *ipxp;
+	int error;
 
 	ipxp = sotoipxpcb(so);
 
-	return (ipx_pcbbind(ipxp, nam, td));
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
+	error = ipx_pcbbind(ipxp, nam, td);
+	IPX_UNLOCK(ipxp);
+	IPX_LIST_UNLOCK();
+	return (error);
 }
 
 /*
@@ -1390,15 +1433,15 @@ spx_connect(so, nam, td)
 	struct sockaddr *nam;
 	struct thread *td;
 {
-	int error;
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
+	int error;
 
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
-	s = splnet();
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
 	if (ipxp->ipxp_lport == 0) {
 		error = ipx_pcbbind(ipxp, NULL, td);
 		if (error)
@@ -1425,7 +1468,8 @@ spx_connect(so, nam, td)
 	ipxp->ipxp_fport = 0;
 	error = spx_output(cb, NULL);
 spx_connect_end:
-	splx(s);
+	IPX_UNLOCK(ipxp);
+	IPX_LIST_UNLOCK();
 	return (error);
 }
 
@@ -1433,7 +1477,6 @@ static int
 spx_detach(so)
 	struct socket *so;
 {
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 
@@ -1442,12 +1485,13 @@ spx_detach(so)
 
 	if (ipxp == NULL)
 		return (ENOTCONN);
-	s = splnet();
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
 	if (cb->s_state > TCPS_LISTEN)
 		spx_disconnect(cb);
 	else
 		spx_close(cb);
-	splx(s);
+	IPX_LIST_UNLOCK();
 	return (0);
 }
 
@@ -1460,16 +1504,16 @@ static int
 spx_usr_disconnect(so)
 	struct socket *so;
 {
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
-	s = splnet();
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
 	spx_disconnect(cb);
-	splx(s);
+	IPX_LIST_UNLOCK();
 	return (0);
 }
 
@@ -1486,10 +1530,14 @@ spx_listen(so, td)
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
 	if (ipxp->ipxp_lport == 0)
 		error = ipx_pcbbind(ipxp, NULL, td);
 	if (error == 0)
 		cb->s_state = TCPS_LISTEN;
+	IPX_UNLOCK(ipxp);
+	IPX_LIST_UNLOCK();
 	return (error);
 }
 
@@ -1502,18 +1550,17 @@ spx_rcvd(so, flags)
 	struct socket *so;
 	int flags;
 {
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
-	s = splnet();
+	IPX_LOCK(ipxp);
 	cb->s_flags |= SF_RVD;
 	spx_output(cb, NULL);
 	cb->s_flags &= ~SF_RVD;
-	splx(s);
+	IPX_UNLOCK(ipxp);
 	return (0);
 }
 
@@ -1534,6 +1581,7 @@ spx_rcvoob(so, m, flags)
 	    (so->so_rcv.sb_state & SBS_RCVATMARK)) {
 		SOCKBUF_UNLOCK(&so->so_rcv);
 		m->m_len = 1;
+		/* Unlocked read. */
 		*mtod(m, caddr_t) = cb->s_iobc;
 		return (0);
 	}
@@ -1551,7 +1599,6 @@ spx_send(so, flags, m, addr, controlp, td)
 	struct thread *td;
 {
 	int error;
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 
@@ -1559,7 +1606,7 @@ spx_send(so, flags, m, addr, controlp, td)
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
-	s = splnet();
+	IPX_LOCK(ipxp);
 	if (flags & PRUS_OOB) {
 		if (sbspace(&so->so_snd) < -512) {
 			error = ENOBUFS;
@@ -1580,11 +1627,11 @@ spx_send(so, flags, m, addr, controlp, td)
 	error = spx_output(cb, m);
 	m = NULL;
 spx_send_end:
+	IPX_UNLOCK(ipxp);
 	if (controlp != NULL)
 		m_freem(controlp);
 	if (m != NULL)
 		m_freem(m);
-	splx(s);
 	return (error);
 }
 
@@ -1592,17 +1639,17 @@ static int
 spx_shutdown(so)
 	struct socket *so;
 {
-	int s;
 	struct ipxpcb *ipxp;
 	struct spxpcb *cb;
 
 	ipxp = sotoipxpcb(so);
 	cb = ipxtospxpcb(ipxp);
 
-	s = splnet();
 	socantsendmore(so);
+	IPX_LIST_LOCK();
+	IPX_LOCK(ipxp);
 	spx_usrclosed(cb);
-	splx(s);
+	IPX_LIST_UNLOCK();
 	return (0);
 }
 
@@ -1638,6 +1685,8 @@ spx_template(cb)
 	register struct ipx *ipx = cb->s_ipx;
 	register struct sockbuf *sb = &(ipxp->ipxp_socket->so_snd);
 
+	IPX_LOCK_ASSERT(ipxp);
+
 	ipx->ipx_pt = IPXPROTO_SPX;
 	ipx->ipx_sna = ipxp->ipxp_laddr;
 	ipx->ipx_dna = ipxp->ipxp_faddr;
@@ -1668,6 +1717,9 @@ spx_close(cb)
 	struct socket *so = ipxp->ipxp_socket;
 	register struct mbuf *m;
 
+	IPX_LIST_LOCK_ASSERT();
+	IPX_LOCK_ASSERT(ipxp);
+
 	s = cb->s_q.si_next;
 	while (s != &(cb->s_q)) {
 		s = s->si_next;
@@ -1694,6 +1746,9 @@ spx_usrclosed(cb)
 	register struct spxpcb *cb;
 {
 
+	IPX_LIST_LOCK_ASSERT();
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	spx_close(cb);
 }
 
@@ -1705,6 +1760,9 @@ spx_disconnect(cb)
 	register struct spxpcb *cb;
 {
 
+	IPX_LIST_LOCK_ASSERT();
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	spx_close(cb);
 }
 
@@ -1720,6 +1778,9 @@ spx_drop(cb, errno)
 {
 	struct socket *so = cb->s_ipxpcb->ipxp_socket;
 
+	IPX_LIST_LOCK_ASSERT();
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	/*
 	 * someday, in the xerox world
 	 * we will generate error protocol packets
@@ -1741,11 +1802,12 @@ spx_drop(cb, errno)
 void
 spx_fasttimo()
 {
-	register struct ipxpcb *ipxp;
-	register struct spxpcb *cb;
-	int s = splnet();
+	struct ipxpcb *ipxp;
+	struct spxpcb *cb;
 
+	IPX_LIST_LOCK();
 	LIST_FOREACH(ipxp, &ipxpcb_list, ipxp_list) {
+		IPX_LOCK(ipxp);
 		if ((cb = (struct spxpcb *)ipxp->ipxp_pcb) != NULL &&
 		    (cb->s_flags & SF_DELACK)) {
 			cb->s_flags &= ~SF_DELACK;
@@ -1753,9 +1815,9 @@ spx_fasttimo()
 			spxstat.spxs_delack++;
 			spx_output(cb, NULL);
 		}
+		IPX_UNLOCK(ipxp);
 	}
-
-	splx(s);
+	IPX_LIST_UNLOCK();
 }
 
 /*
@@ -1766,19 +1828,23 @@ spx_fasttimo()
 void
 spx_slowtimo()
 {
-	register struct ipxpcb *ip, *ip_temp;
-	register struct spxpcb *cb;
-	int s = splnet();
-	register int i;
+	struct ipxpcb *ip, *ip_temp;
+	struct spxpcb *cb;
+	int i;
 
 	/*
 	 * Search through tcb's and update active timers.  Note that timers
 	 * may free the ipxpcb, so be sure to handle that case.
+	 *
+	 * spx_timers() may remove an ipxpcb entry, so we have to be ready to
+	 * continue despite that.  The logic here is a bit obfuscated.
 	 */
+	IPX_LIST_LOCK();
 	LIST_FOREACH_SAFE(ip, &ipxpcb_list, ipxp_list, ip_temp) {
 		cb = ipxtospxpcb(ip);
 		if (cb == NULL)
 			continue;
+		IPX_LOCK(cb->s_ipxpcb);
 		for (i = 0; i < SPXT_NTIMERS; i++) {
 			if (cb->s_timer[i] && --cb->s_timer[i] == 0) {
 				/*
@@ -1794,10 +1860,11 @@ spx_slowtimo()
 			cb->s_idle++;
 			if (cb->s_rtt)
 				cb->s_rtt++;
+			IPX_UNLOCK(cb->s_ipxpcb);
 		}
 	}
 	spx_iss += SPX_ISSINCR/PR_SLOWHZ;		/* increment iss */
-	splx(s);
+	IPX_LIST_UNLOCK();
 }
 
 /*
@@ -1811,6 +1878,9 @@ spx_timers(cb, timer)
 	long rexmt;
 	int win;
 
+	IPX_LIST_LOCK_ASSERT();
+	IPX_LOCK_ASSERT(cb->s_ipxpcb);
+
 	cb->s_force = 1 + timer;
 	switch (timer) {