Add a global rwlock, at_ifaddr_rw, to protect the global netatalk

address lists, at_ifaddr_list.  Acquire the lock, and use ifaddr
refcounts where necessary, to close most known address-related
races in netatalk.

Annotate one potential race in at_control() where we acquire an
ifaddr reference, drop the global lock, and scrub the address from
the ifnet before re-acquiring the global lock, which could allow
for a writer-writer race.

MFC after:	3 weeks

1 files changed, 25 insertions, 21 deletions

diff --git a/sys/netatalk/ddp_input.c b/sys/netatalk/ddp_input.c
index 2e82a2a..49a1908 100644
--- a/sys/netatalk/ddp_input.c
+++ b/sys/netatalk/ddp_input.c
@@ -1,5 +1,5 @@
 /*-
- * Copyright (c) 2004 Robert N. M. Watson
+ * Copyright (c) 2004-2009 Robert N. M. Watson
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -161,6 +161,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		 * Make sure that we point to the phase1 ifaddr info and that
 		 * it's valid for this packet.
 		 */
+		AT_IFADDR_RLOCK();
 		for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) {
 			if ((aa->aa_ifp == ifp)
 			    && ((aa->aa_flags & AFA_PHASE2) == 0)
@@ -173,6 +174,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		 * maybe we got a broadcast not meant for us.. ditch it.
 		 */
 		if (aa == NULL) {
+			AT_IFADDR_RUNLOCK();
 			m_freem(m);
 			return;
 		}
@@ -187,6 +189,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 
 		if (m->m_len < sizeof(struct ddpehdr) &&
 		    ((m = m_pullup(m, sizeof(struct ddpehdr))) == NULL)) {
+			AT_IFADDR_RUNLOCK();
 			ddpstat.ddps_tooshort++;
 			return;
 		}
@@ -206,6 +209,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		to.sat_addr.s_node = ddpe.deh_dnode;
 		to.sat_port = ddpe.deh_dport;
 
+		AT_IFADDR_RLOCK();
 		if (to.sat_addr.s_net == ATADDR_ANYNET) {
 			/*
 			 * The TO address doesn't specify a net, so by
@@ -278,6 +282,9 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 			}
 		}
 	}
+	if (aa != NULL)
+		ifa_ref(&aa->aa_ifa);
+	AT_IFADDR_RUNLOCK();
 
 	/*
 	 * Adjust the length, removing any padding that may have been added
@@ -287,8 +294,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 	mlen = m->m_pkthdr.len;
 	if (mlen < dlen) {
 		ddpstat.ddps_toosmall++;
-		m_freem(m);
-		return;
+		goto out;
 	}
 	if (mlen > dlen)
 		m_adj(m, dlen - mlen);
@@ -304,10 +310,8 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		/* 
 		 * If we've explicitly disabled it, don't route anything.
 		 */
-		if (ddp_forward == 0) {
-			m_freem(m);
-			return;
-		}
+		if (ddp_forward == 0)
+			goto out;
 
 		/* 
 		 * If the cached forwarding route is still valid, use it.
@@ -346,10 +350,8 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		 */
 		if ((to.sat_addr.s_net !=
 		    satosat(&forwro.ro_dst)->sat_addr.s_net) &&
-		    (ddpe.deh_hops == DDP_MAXHOPS)) {
-			m_freem(m);
-			return;
-		}
+		    (ddpe.deh_hops == DDP_MAXHOPS))
+			goto out;
 
 		/*
 		 * A ddp router might use the same interface to forward the
@@ -357,10 +359,8 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		 * to cross from one interface to another however.
 		 */
 		if (ddp_firewall && ((forwro.ro_rt == NULL) ||
-		    (forwro.ro_rt->rt_ifp != ifp))) {
-			m_freem(m);
-			return;
-		}
+		    (forwro.ro_rt->rt_ifp != ifp)))
+			goto out;
 
 		/*
 		 * Adjust the header.  If it was a short header then it would
@@ -377,6 +377,8 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 			ddpstat.ddps_cantforward++;
 		else
 			ddpstat.ddps_forward++;
+		if (aa != NULL)
+			ifa_free(&aa->aa_ifa);
 		return;
 	}
 
@@ -393,8 +395,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		if (ddp_cksum && cksum && cksum !=
 		    at_cksum(m, sizeof(int))) {
 			ddpstat.ddps_badsum++;
-			m_freem(m);
-			return;
+			goto out;
 		}
 		m_adj(m, sizeof(struct ddpehdr));
 	} else
@@ -405,11 +406,11 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 	 */
 	DDP_LIST_SLOCK();
 	if ((ddp = ddp_search(&from, &to, aa)) == NULL)
-		goto out;
+		goto out_unlock;
 
 #ifdef MAC
 	if (mac_socket_check_deliver(ddp->ddp_socket, m) != 0)
-		goto out;
+		goto out_unlock;
 #endif
 
 	/* 
@@ -423,7 +424,7 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 		 * If the socket is full (or similar error) dump the packet.
 		 */
 		ddpstat.ddps_nosockspace++;
-		goto out;
+		goto out_unlock;
 	}
 
 	/*
@@ -431,8 +432,11 @@ ddp_input(struct mbuf *m, struct ifnet *ifp, struct elaphdr *elh, int phase)
 	 */
 	sorwakeup_locked(ddp->ddp_socket);
 	m = NULL;
-out:
+out_unlock:
 	DDP_LIST_SUNLOCK();
+out:
+	if (aa != NULL)
+		ifa_free(&aa->aa_ifa);
 	if (m != NULL)
 		m_freem(m);
 }