Rework locking and reference counting in ipx_control to be consistent with

the model used in in_control().

MFC after:	6 weeks

1 files changed, 60 insertions, 58 deletions

diff --git a/sys/netipx/ipx.c b/sys/netipx/ipx.c
index 4eb9570..776b4ab 100644
--- a/sys/netipx/ipx.c
+++ b/sys/netipx/ipx.c
@@ -100,11 +100,10 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 {
 	struct ifreq *ifr = (struct ifreq *)data;
 	struct ipx_aliasreq *ifra = (struct ipx_aliasreq *)data;
-	struct ipx_ifaddr *ia;
+	struct ipx_ifaddr *ia, *ia_temp, *oia;
 	struct ifaddr *ifa;
-	struct ipx_ifaddr *oia;
 	int dstIsNew, hostIsNew;
-	int error = 0, priv;
+	int error, priv;
 
 	/*
 	 * Find address for this interface, if it exists.
@@ -112,11 +111,15 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 	if (ifp == NULL)
 		return (EADDRNOTAVAIL);
 
-	IPX_IFADDR_WLOCK();
+	IPX_IFADDR_RLOCK();
 	for (ia = ipx_ifaddr; ia != NULL; ia = ia->ia_next)
 		if (ia->ia_ifp == ifp)
 			break;
+	if (ia != NULL)
+		ifa_ref(&ia->ia_ifa);
+	IPX_IFADDR_RUNLOCK();
 
+	error = 0;
 	switch (cmd) {
 	case SIOCGIFADDR:
 		if (ia == NULL) {
@@ -158,13 +161,21 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 		    PRIV_NET_DELIFADDR;
 		if (td && (error = priv_check(td, priv)) != 0)
 			goto out;
-		if (ifra->ifra_addr.sipx_family == AF_IPX)
-		    for (oia = ia; ia != NULL; ia = ia->ia_next) {
-			if (ia->ia_ifp == ifp  &&
-			    ipx_neteq(ia->ia_addr.sipx_addr,
-				  ifra->ifra_addr.sipx_addr))
-			    break;
-		    }
+
+		IPX_IFADDR_RLOCK();
+		if (ifra->ifra_addr.sipx_family == AF_IPX) {
+			for (oia = ia; ia != NULL; ia = ia->ia_next) {
+				if (ia->ia_ifp == ifp  &&
+				    ipx_neteq(ia->ia_addr.sipx_addr,
+				    ifra->ifra_addr.sipx_addr))
+					break;
+			}
+			if (oia != NULL && oia != ia)
+				ifa_free(&oia->ia_ifa);
+			if (ia != NULL && oia != ia)
+				ifa_ref(&ia->ia_ifa);
+		}
+		IPX_IFADDR_RUNLOCK();
 		if (cmd == SIOCDIFADDR && ia == NULL) {
 			error = EADDRNOTAVAIL;
 			goto out;
@@ -176,20 +187,11 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 		if (td && (error = priv_check(td, PRIV_NET_SETLLADDR)) != 0)
 			goto out;
 		if (ia == NULL) {
-			oia = (struct ipx_ifaddr *)
-				malloc(sizeof(*ia), M_IFADDR,
-				M_NOWAIT | M_ZERO);
-			if (oia == NULL) {
+			ia = malloc(sizeof(*ia), M_IFADDR, M_NOWAIT | M_ZERO);
+			if (ia == NULL) {
 				error = ENOBUFS;
 				goto out;
 			}
-			if ((ia = ipx_ifaddr) != NULL) {
-				for ( ; ia->ia_next != NULL; ia = ia->ia_next)
-					;
-				ia->ia_next = oia;
-			} else
-				ipx_ifaddr = oia;
-			ia = oia;
 			ifa = (struct ifaddr *)ia;
 			ifa_init(ifa);
 			ia->ia_ifp = ifp;
@@ -203,6 +205,18 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 				ia->ia_broadaddr.sipx_addr.x_host =
 				    ipx_broadhost;
 			}
+			ifa_ref(&ia->ia_ifa);		/* ipx_ifaddr */
+			IPX_IFADDR_WLOCK();
+			if ((ia_temp = ipx_ifaddr) != NULL) {
+				for (; ia_temp->ia_next != NULL;
+				    ia_temp = ia_temp->ia_next)
+					;
+				ia_temp->ia_next = ia;
+			} else
+				ipx_ifaddr = ia;
+			IPX_IFADDR_WUNLOCK();
+
+			ifa_ref(&ia->ia_ifa);		/* if_addrhead */
 			IF_ADDR_LOCK(ifp);
 			TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link);
 			IF_ADDR_UNLOCK(ifp);
@@ -224,54 +238,43 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 			rtinit(&(ia->ia_ifa), (int)RTM_DELETE, RTF_HOST);
 			ia->ia_flags &= ~IFA_ROUTE;
 		}
-		ifa_ref(&ia->ia_ifa);
-		IPX_IFADDR_WUNLOCK();
 		if (ifp->if_ioctl) {
 			error = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR,
 			    (void *)ia);
-			if (error) {
-				ifa_free(&ia->ia_ifa);
-				return (error);
-			}
+			if (error)
+				goto out;
 		}
 		*(struct sockaddr *)&ia->ia_dstaddr = ifr->ifr_dstaddr;
-		ifa_free(&ia->ia_ifa);
-		return (0);
+		goto out;
 
 	case SIOCSIFADDR:
-		ifa_ref(&ia->ia_ifa);
-		IPX_IFADDR_WUNLOCK();
 		error = ipx_ifinit(ifp, ia,
 		    (struct sockaddr_ipx *)&ifr->ifr_addr, 1);
-		ifa_free(&ia->ia_ifa);
-		return (error);
+		goto out;
 
 	case SIOCDIFADDR:
-		/* XXXRW: Potential race here while ipx_ifaddr_rw is dropped. */
-		ifa_ref(&ia->ia_ifa);
-		IPX_IFADDR_WUNLOCK();
 		ipx_ifscrub(ifp, ia);
-		IPX_IFADDR_WLOCK();
-		ifa_free(&ia->ia_ifa);
 		ifa = (struct ifaddr *)ia;
+
 		IF_ADDR_LOCK(ifp);
 		TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link);
 		IF_ADDR_UNLOCK(ifp);
-		oia = ia;
-		if (oia == (ia = ipx_ifaddr)) {
+		ifa_free(ifa);				/* if_addrhead */
+
+		IPX_IFADDR_WLOCK();
+		if (ia == (ia_temp = ipx_ifaddr)) {
 			ipx_ifaddr = ia->ia_next;
 		} else {
-			while (ia->ia_next && (ia->ia_next != oia)) {
-				ia = ia->ia_next;
-			}
-			if (ia->ia_next)
-			    ia->ia_next = oia->ia_next;
+			while (ia_temp->ia_next && (ia_temp->ia_next != ia))
+				ia_temp = ia_temp->ia_next;
+			if (ia_temp->ia_next)
+			    ia_temp->ia_next = ia->ia_next;
 			else
-				printf("Didn't unlink ipxifadr from list\n");
+				panic("Didn't unlink ipxifadr from list\n");
 		}
-		ifa_free(&oia->ia_ifa);
 		IPX_IFADDR_WUNLOCK();
-		return (0);
+		ifa_free(&ia->ia_ifa);			/* ipx_ifaddr */
+		goto out;
 
 	case SIOCAIFADDR:
 		dstIsNew = 0;
@@ -284,8 +287,6 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 					 ia->ia_addr.sipx_addr))
 				hostIsNew = 0;
 		}
-		ifa_ref(&ia->ia_ifa);
-		IPX_IFADDR_WUNLOCK();
 		if ((ifp->if_flags & IFF_POINTOPOINT) &&
 		    (ifra->ifra_dstaddr.sipx_family == AF_IPX)) {
 			if (hostIsNew == 0)
@@ -296,18 +297,19 @@ ipx_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
 		if (ifra->ifra_addr.sipx_family == AF_IPX &&
 					    (hostIsNew || dstIsNew))
 			error = ipx_ifinit(ifp, ia, &ifra->ifra_addr, 0);
-		ifa_free(&ia->ia_ifa);
-		return (error);
+		goto out;
 
 	default:
-		IPX_IFADDR_WUNLOCK();
-		if (ifp->if_ioctl == NULL)
-			return (EOPNOTSUPP);
-		return ((*ifp->if_ioctl)(ifp, cmd, data));
+		if (ifp->if_ioctl == NULL) {
+			error = EOPNOTSUPP;
+			goto out;
+		}
+		error = ((*ifp->if_ioctl)(ifp, cmd, data));
 	}
 
 out:
-	IPX_IFADDR_WUNLOCK();
+	if (ia != NULL)
+		ifa_free(&ia->ia_ifa);
 	return (error);
 }