Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 23 Jun 2009 21:24:28 +0100 (BST)
From:      Robert Watson <rwatson@FreeBSD.org>
To:        current@FreeBSD.org
Subject:   Warning: ifaddr refcount use patch (svn commit: r194760 - in head/sys: contrib/rdma net net80211 netinet netinet6 netipx (fwd))
Message-ID:  <alpine.BSF.2.00.0906232120400.13224@fledge.watson.org>

next in thread | raw e-mail | index | archive | help

FYI to all:

I've committed a large patch that makes greater use of struct ifaddr refcounts 
to avoid a range of reader-writer and writer-writer races affecting a range of 
uses from simultaneous ifconfigs to ppp/tunnel/vpn endpoint reconfiguration. 
There are still some more address list locking changes to go in in the next 48 
hours before the 8.0 freeze, which should in the medium-term significant 
improve stability in these areas.

However, because this modifies quite a few spots in the code, it's possible 
we'll see two classes of bugs:

- Reference leaks -- references acquired, but I missed a case and the
   reference is leaked.  This could lead to a gradual memory leak of ifaddr's.
   You can track ifaddr allocation using "vmstat -m | grep ifaddr" -- if you
   see something you think is a leak, let me know.

- Use-after-free -- in some case, a reference might not be properly
   acquired, but will be released, in which case memory might be used after
   free.  In -CURRENT where we have INVARIANTS enabled, memory scrubbing
   generally picks this up quickly but not immediately, but watch out for new
   kernel page faults involving 0xdeadc0de.

If you run into these problems, I'll likely send you some debugging patches 
that make it easier to track this down.

Robert N M Watson
Computer Laboratory
University of Cambridge

---------- Forwarded message ----------
Date: Tue, 23 Jun 2009 20:19:09 +0000 (UTC)
From: Robert Watson <rwatson@FreeBSD.org>
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
     svn-src-head@freebsd.org
Subject: svn commit: r194760 - in head/sys: contrib/rdma net net80211 netinet
     netinet6 netipx

Author: rwatson
Date: Tue Jun 23 20:19:09 2009
New Revision: 194760
URL: http://svn.freebsd.org/changeset/base/194760

Log:
   Modify most routines returning 'struct ifaddr *' to return references
   rather than pointers, requiring callers to properly dispose of those
   references.  The following routines now return references:

     ifaddr_byindex
     ifa_ifwithaddr
     ifa_ifwithbroadaddr
     ifa_ifwithdstaddr
     ifa_ifwithnet
     ifaof_ifpforaddr
     ifa_ifwithroute
     ifa_ifwithroute_fib
     rt_getifa
     rt_getifa_fib
     IFP_TO_IA
     ip_rtaddr
     in6_ifawithifp
     in6ifa_ifpforlinklocal
     in6ifa_ifpwithaddr
     in6_ifadd
     carp_iamatch6
     ip6_getdstifaddr

   Remove unused macro which didn't have required referencing:

     IFP_TO_IA6

   This closes many small races in which changes to interface
   or address lists while an ifaddr was in use could lead to use of freed
   memory (etc).  In a few cases, add missing if_addr_list locking
   required to safely acquire references.

   Because of a lack of deep copying support, we accept a race in which
   an in6_ifaddr pointed to by mbuf tags and extracted with
   ip6_getdstifaddr() doesn't hold a reference while in transmit.  Once
   we have mbuf tag deep copy support, this can be fixed.

   Reviewed by:	bz
   Obtained from:	Apple, Inc. (portions)
   MFC after:	6 weeks (portions)

Modified:
   head/sys/contrib/rdma/rdma_addr.c
   head/sys/contrib/rdma/rdma_cma.c
   head/sys/net/if.c
   head/sys/net/route.c
   head/sys/net/rtsock.c
   head/sys/net80211/ieee80211.c
   head/sys/netinet/igmp.c
   head/sys/netinet/in.c
   head/sys/netinet/in_mcast.c
   head/sys/netinet/in_pcb.c
   head/sys/netinet/in_var.h
   head/sys/netinet/ip_carp.c
   head/sys/netinet/ip_divert.c
   head/sys/netinet/ip_icmp.c
   head/sys/netinet/ip_input.c
   head/sys/netinet/ip_mroute.c
   head/sys/netinet/ip_options.c
   head/sys/netinet/ip_output.c
   head/sys/netinet/tcp_input.c
   head/sys/netinet6/frag6.c
   head/sys/netinet6/icmp6.c
   head/sys/netinet6/in6.c
   head/sys/netinet6/in6_ifattach.c
   head/sys/netinet6/in6_pcb.c
   head/sys/netinet6/in6_src.c
   head/sys/netinet6/in6_var.h
   head/sys/netinet6/ip6_input.c
   head/sys/netinet6/ip6_output.c
   head/sys/netinet6/mld6.c
   head/sys/netinet6/nd6.c
   head/sys/netinet6/nd6_nbr.c
   head/sys/netinet6/nd6_rtr.c
   head/sys/netinet6/raw_ip6.c
   head/sys/netipx/ipx_pcb.c

Modified: head/sys/contrib/rdma/rdma_addr.c
==============================================================================
--- head/sys/contrib/rdma/rdma_addr.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/contrib/rdma/rdma_addr.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -129,13 +129,16 @@ int rdma_translate_ip(struct sockaddr *a
  	struct ifaddr *ifa;
  	struct sockaddr_in *sin = (struct sockaddr_in *)addr;
  	uint16_t port = sin->sin_port;
+	int ret;

  	sin->sin_port = 0;
  	ifa = ifa_ifwithaddr(addr);
  	sin->sin_port = port;
  	if (!ifa)
  		return (EADDRNOTAVAIL);
-	return rdma_copy_addr(dev_addr, ifa->ifa_ifp, NULL);
+	ret = rdma_copy_addr(dev_addr, ifa->ifa_ifp, NULL);
+	ifa_free(ifa);
+	return (ret);
  }

  static void queue_req(struct addr_req *req)

Modified: head/sys/contrib/rdma/rdma_cma.c
==============================================================================
--- head/sys/contrib/rdma/rdma_cma.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/contrib/rdma/rdma_cma.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -1337,6 +1337,7 @@ static int iw_conn_req_handler(struct iw
  	}
  	dev = ifa->ifa_ifp;
  	ret = rdma_copy_addr(&conn_id->id.route.addr.dev_addr, dev, NULL);
+	ifa_free(ifa);
  	if (ret) {
  		cma_enable_remove(conn_id);
  		rdma_destroy_id(new_cm_id);

Modified: head/sys/net/if.c
==============================================================================
--- head/sys/net/if.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/net/if.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -48,6 +48,7 @@
  #include <sys/socket.h>
  #include <sys/socketvar.h>
  #include <sys/protosw.h>
+#include <sys/kdb.h>
  #include <sys/kernel.h>
  #include <sys/lock.h>
  #include <sys/refcount.h>
@@ -261,6 +262,8 @@ ifaddr_byindex(u_short idx)

  	IFNET_RLOCK();
  	ifa = ifnet_byindex_locked(idx)->if_addr;
+	if (ifa != NULL)
+		ifa_ref(ifa);
  	IFNET_RUNLOCK();
  	return (ifa);
  }
@@ -1464,7 +1467,7 @@ ifa_free(struct ifaddr *ifa)
   */
  /*ARGSUSED*/
  static struct ifaddr *
-ifa_ifwithaddr_internal(struct sockaddr *addr)
+ifa_ifwithaddr_internal(struct sockaddr *addr, int getref)
  {
  	INIT_VNET_NET(curvnet);
  	struct ifnet *ifp;
@@ -1477,6 +1480,8 @@ ifa_ifwithaddr_internal(struct sockaddr
  			if (ifa->ifa_addr->sa_family != addr->sa_family)
  				continue;
  			if (sa_equal(addr, ifa->ifa_addr)) {
+				if (getref)
+					ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto done;
  			}
@@ -1485,6 +1490,8 @@ ifa_ifwithaddr_internal(struct sockaddr
  			    ifa->ifa_broadaddr &&
  			    ifa->ifa_broadaddr->sa_len != 0 &&
  			    sa_equal(ifa->ifa_broadaddr, addr)) {
+				if (getref)
+					ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto done;
  			}
@@ -1501,14 +1508,14 @@ struct ifaddr *
  ifa_ifwithaddr(struct sockaddr *addr)
  {

-	return (ifa_ifwithaddr_internal(addr));
+	return (ifa_ifwithaddr_internal(addr, 1));
  }

  int
  ifa_ifwithaddr_check(struct sockaddr *addr)
  {

-	return (ifa_ifwithaddr_internal(addr) != NULL);
+	return (ifa_ifwithaddr_internal(addr, 0) != NULL);
  }

  /*
@@ -1532,6 +1539,7 @@ ifa_ifwithbroadaddr(struct sockaddr *add
  			    ifa->ifa_broadaddr &&
  			    ifa->ifa_broadaddr->sa_len != 0 &&
  			    sa_equal(ifa->ifa_broadaddr, addr)) {
+				ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto done;
  			}
@@ -1565,6 +1573,7 @@ ifa_ifwithdstaddr(struct sockaddr *addr)
  				continue;
  			if (ifa->ifa_dstaddr != NULL &&
  			    sa_equal(addr, ifa->ifa_dstaddr)) {
+				ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto done;
  			}
@@ -1587,7 +1596,7 @@ ifa_ifwithnet(struct sockaddr *addr)
  	INIT_VNET_NET(curvnet);
  	struct ifnet *ifp;
  	struct ifaddr *ifa;
-	struct ifaddr *ifa_maybe = (struct ifaddr *) 0;
+	struct ifaddr *ifa_maybe = NULL;
  	u_int af = addr->sa_family;
  	char *addr_data = addr->sa_data, *cplim;

@@ -1602,8 +1611,10 @@ ifa_ifwithnet(struct sockaddr *addr)
  	}

  	/*
-	 * Scan though each interface, looking for ones that have
-	 * addresses in this address family.
+	 * Scan though each interface, looking for ones that have addresses
+	 * in this address family.  Maintain a reference on ifa_maybe once
+	 * we find one, as we release the IF_ADDR_LOCK() that kept it stable
+	 * when we move onto the next interface.
  	 */
  	IFNET_RLOCK();
  	TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
@@ -1624,6 +1635,7 @@ next:				continue;
  				 */
  				if (ifa->ifa_dstaddr != NULL &&
  				    sa_equal(addr, ifa->ifa_dstaddr)) {
+					ifa_ref(ifa);
  					IF_ADDR_UNLOCK(ifp);
  					goto done;
  				}
@@ -1634,6 +1646,7 @@ next:				continue;
  				 */
  				if (ifa->ifa_claim_addr) {
  					if ((*ifa->ifa_claim_addr)(ifa, addr)) {
+						ifa_ref(ifa);
  						IF_ADDR_UNLOCK(ifp);
  						goto done;
  					}
@@ -1664,17 +1677,24 @@ next:				continue;
  				 * before continuing to search
  				 * for an even better one.
  				 */
-				if (ifa_maybe == 0 ||
+				if (ifa_maybe == NULL ||
  				    rn_refines((caddr_t)ifa->ifa_netmask,
-				    (caddr_t)ifa_maybe->ifa_netmask))
+				    (caddr_t)ifa_maybe->ifa_netmask)) {
+					if (ifa_maybe != NULL)
+						ifa_free(ifa_maybe);
  					ifa_maybe = ifa;
+					ifa_ref(ifa_maybe);
+				}
  			}
  		}
  		IF_ADDR_UNLOCK(ifp);
  	}
  	ifa = ifa_maybe;
+	ifa_maybe = NULL;
  done:
  	IFNET_RUNLOCK();
+	if (ifa_maybe != NULL)
+		ifa_free(ifa_maybe);
  	return (ifa);
  }

@@ -1688,7 +1708,7 @@ ifaof_ifpforaddr(struct sockaddr *addr,
  	struct ifaddr *ifa;
  	char *cp, *cp2, *cp3;
  	char *cplim;
-	struct ifaddr *ifa_maybe = 0;
+	struct ifaddr *ifa_maybe = NULL;
  	u_int af = addr->sa_family;

  	if (af >= AF_MAX)
@@ -1697,7 +1717,7 @@ ifaof_ifpforaddr(struct sockaddr *addr,
  	TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
  		if (ifa->ifa_addr->sa_family != af)
  			continue;
-		if (ifa_maybe == 0)
+		if (ifa_maybe == NULL)
  			ifa_maybe = ifa;
  		if (ifa->ifa_netmask == 0) {
  			if (sa_equal(addr, ifa->ifa_addr) ||
@@ -1723,6 +1743,8 @@ ifaof_ifpforaddr(struct sockaddr *addr,
  	}
  	ifa = ifa_maybe;
  done:
+	if (ifa != NULL)
+		ifa_ref(ifa);
  	IF_ADDR_UNLOCK(ifp);
  	return (ifa);
  }
@@ -1748,7 +1770,6 @@ link_rtrequest(int cmd, struct rtentry *
  		return;
  	ifa = ifaof_ifpforaddr(dst, ifp);
  	if (ifa) {
-		ifa_ref(ifa);		/* XXX */
  		oifa = rt->rt_ifa;
  		rt->rt_ifa = ifa;
  		ifa_free(oifa);

Modified: head/sys/net/route.c
==============================================================================
--- head/sys/net/route.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/net/route.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -559,6 +559,7 @@ rtredirect_fib(struct sockaddr *dst,
  	struct ifaddr *ifa;
  	struct radix_node_head *rnh;

+	ifa = NULL;
  	rnh = rt_tables_get_rnh(fibnum, dst->sa_family);
  	if (rnh == NULL) {
  		error = EAFNOSUPPORT;
@@ -664,6 +665,8 @@ out:
  	info.rti_info[RTAX_NETMASK] = netmask;
  	info.rti_info[RTAX_AUTHOR] = src;
  	rt_missmsg(RTM_REDIRECT, &info, flags, error);
+	if (ifa != NULL)
+		ifa_free(ifa);
  }

  int
@@ -693,6 +696,9 @@ rtioctl_fib(u_long req, caddr_t data, u_
  #endif /* INET */
  }

+/*
+ * For both ifa_ifwithroute() routines, 'ifa' is returned referenced.
+ */
  struct ifaddr *
  ifa_ifwithroute(int flags, struct sockaddr *dst, struct sockaddr *gateway)
  {
@@ -749,11 +755,13 @@ ifa_ifwithroute_fib(int flags, struct so
  		default:
  			break;
  		}
+		if (!not_found && rt->rt_ifa != NULL) {
+			ifa = rt->rt_ifa;
+			ifa_ref(ifa);
+		}
  		RT_REMREF(rt);
  		RT_UNLOCK(rt);
-		if (not_found)
-			return (NULL);
-		if ((ifa = rt->rt_ifa) == NULL)
+		if (not_found || ifa == NULL)
  			return (NULL);
  	}
  	if (ifa->ifa_addr->sa_family != dst->sa_family) {
@@ -761,6 +769,8 @@ ifa_ifwithroute_fib(int flags, struct so
  		ifa = ifaof_ifpforaddr(dst, ifa->ifa_ifp);
  		if (ifa == NULL)
  			ifa = oifa;
+		else
+			ifa_free(oifa);
  	}
  	return (ifa);
  }
@@ -819,6 +829,10 @@ rt_getifa(struct rt_addrinfo *info)
  	return (rt_getifa_fib(info, 0));
  }

+/*
+ * Look up rt_addrinfo for a specific fib.  Note that if rti_ifa is defined,
+ * it will be referenced so the caller must free it.
+ */
  int
  rt_getifa_fib(struct rt_addrinfo *info, u_int fibnum)
  {
@@ -831,8 +845,10 @@ rt_getifa_fib(struct rt_addrinfo *info,
  	 */
  	if (info->rti_ifp == NULL && ifpaddr != NULL &&
  	    ifpaddr->sa_family == AF_LINK &&
-	    (ifa = ifa_ifwithnet(ifpaddr)) != NULL)
+	    (ifa = ifa_ifwithnet(ifpaddr)) != NULL) {
  		info->rti_ifp = ifa->ifa_ifp;
+		ifa_free(ifa);
+	}
  	if (info->rti_ifa == NULL && ifaaddr != NULL)
  		info->rti_ifa = ifa_ifwithaddr(ifaaddr);
  	if (info->rti_ifa == NULL) {
@@ -1123,12 +1139,19 @@ rtrequest1_fib(int req, struct rt_addrin
  		    (gateway->sa_family != AF_UNSPEC) && (gateway->sa_family != AF_LINK))
  			senderr(EINVAL);

-		if (info->rti_ifa == NULL && (error = rt_getifa_fib(info, fibnum)))
-			senderr(error);
+		if (info->rti_ifa == NULL) {
+			error = rt_getifa_fib(info, fibnum);
+			if (error)
+				senderr(error);
+		} else
+			ifa_ref(info->rti_ifa);
  		ifa = info->rti_ifa;
  		rt = uma_zalloc(V_rtzone, M_NOWAIT | M_ZERO);
-		if (rt == NULL)
+		if (rt == NULL) {
+			if (ifa != NULL)
+				ifa_free(ifa);
  			senderr(ENOBUFS);
+		}
  		RT_LOCK_INIT(rt);
  		rt->rt_flags = RTF_UP | flags;
  		rt->rt_fibnum = fibnum;
@@ -1139,6 +1162,8 @@ rtrequest1_fib(int req, struct rt_addrin
  		RT_LOCK(rt);
  		if ((error = rt_setgate(rt, dst, gateway)) != 0) {
  			RT_LOCK_DESTROY(rt);
+			if (ifa != NULL)
+				ifa_free(ifa);
  			uma_zfree(V_rtzone, rt);
  			senderr(error);
  		}
@@ -1157,11 +1182,10 @@ rtrequest1_fib(int req, struct rt_addrin
  			bcopy(dst, ndst, dst->sa_len);

  		/*
-		 * Note that we now have a reference to the ifa.
+		 * We use the ifa reference returned by rt_getifa_fib().
  		 * This moved from below so that rnh->rnh_addaddr() can
  		 * examine the ifa and  ifa->ifa_ifp if it so desires.
  		 */
-		ifa_ref(ifa);
  		rt->rt_ifa = ifa;
  		rt->rt_ifp = ifa->ifa_ifp;
  		rt->rt_rmx.rmx_weight = 1;

Modified: head/sys/net/rtsock.c
==============================================================================
--- head/sys/net/rtsock.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/net/rtsock.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -683,6 +683,13 @@ route_output(struct mbuf *m, struct sock
  				RT_UNLOCK(rt);
  				RADIX_NODE_HEAD_LOCK(rnh);
  				error = rt_getifa_fib(&info, rt->rt_fibnum);
+				/*
+				 * XXXRW: Really we should release this
+				 * reference later, but this maintains
+				 * historical behavior.
+				 */
+				if (info.rti_ifa != NULL)
+					ifa_free(info.rti_ifa);
  				RADIX_NODE_HEAD_UNLOCK(rnh);
  				if (error != 0)
  					senderr(error);

Modified: head/sys/net80211/ieee80211.c
==============================================================================
--- head/sys/net80211/ieee80211.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/net80211/ieee80211.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -301,6 +301,7 @@ ieee80211_ifattach(struct ieee80211com *
  	sdl->sdl_type = IFT_ETHER;		/* XXX IFT_IEEE80211? */
  	sdl->sdl_alen = IEEE80211_ADDR_LEN;
  	IEEE80211_ADDR_COPY(LLADDR(sdl), macaddr);
+	ifa_free(ifa);
  }

  /*

Modified: head/sys/netinet/igmp.c
==============================================================================
--- head/sys/netinet/igmp.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/igmp.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -1233,8 +1233,10 @@ igmp_input_v1_report(struct ifnet *ifp,
  	 */
  	if (V_igmp_recvifkludge && in_nullhost(ip->ip_src)) {
  		IFP_TO_IA(ifp, ia);
-		if (ia != NULL)
+		if (ia != NULL) {
  			ip->ip_src.s_addr = htonl(ia->ia_subnet);
+			ifa_free(&ia->ia_ifa);
+		}
  	}

  	CTR3(KTR_IGMPV3, "process v1 report %s on ifp %p(%s)",
@@ -1326,16 +1328,23 @@ igmp_input_v2_report(struct ifnet *ifp,
  	 * group.
  	 */
  	IFP_TO_IA(ifp, ia);
-	if (ia != NULL && in_hosteq(ip->ip_src, IA_SIN(ia)->sin_addr))
+	if (ia != NULL && in_hosteq(ip->ip_src, IA_SIN(ia)->sin_addr)) {
+		ifa_free(&ia->ia_ifa);
  		return (0);
+	}

  	IGMPSTAT_INC(igps_rcv_reports);

-	if (ifp->if_flags & IFF_LOOPBACK)
+	if (ifp->if_flags & IFF_LOOPBACK) {
+		if (ia != NULL)
+			ifa_free(&ia->ia_ifa);
  		return (0);
+	}

  	if (!IN_MULTICAST(ntohl(igmp->igmp_group.s_addr)) ||
  	    !in_hosteq(igmp->igmp_group, ip->ip_dst)) {
+		if (ia != NULL)
+			ifa_free(&ia->ia_ifa);
  		IGMPSTAT_INC(igps_rcv_badreports);
  		return (EINVAL);
  	}
@@ -1351,6 +1360,8 @@ igmp_input_v2_report(struct ifnet *ifp,
  		if (ia != NULL)
  			ip->ip_src.s_addr = htonl(ia->ia_subnet);
  	}
+	if (ia != NULL)
+		ifa_free(&ia->ia_ifa);

  	CTR3(KTR_IGMPV3, "process v2 report %s on ifp %p(%s)",
  	     inet_ntoa(igmp->igmp_group), ifp, ifp->if_xname);
@@ -3534,8 +3545,10 @@ igmp_v3_encap_report(struct ifnet *ifp,
  		struct in_ifaddr *ia;

  		IFP_TO_IA(ifp, ia);
-		if (ia != NULL)
+		if (ia != NULL) {
  			ip->ip_src = ia->ia_addr.sin_addr;
+			ifa_free(&ia->ia_ifa);
+		}
  	}

  	ip->ip_dst.s_addr = htonl(INADDR_ALLRPTS_GROUP);

Modified: head/sys/netinet/in.c
==============================================================================
--- head/sys/netinet/in.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/in.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -219,7 +219,6 @@ in_control(struct socket *so, u_long cmd
  	register struct ifaddr *ifa;
  	struct in_addr allhosts_addr;
  	struct in_addr dst;
-	struct in_ifaddr *oia;
  	struct in_ifinfo *ii;
  	struct in_aliasreq *ifra = (struct in_aliasreq *)data;
  	struct sockaddr_in oldaddr;
@@ -323,8 +322,10 @@ in_control(struct socket *so, u_long cmd
  			break;
  		}
  	}
-	IF_ADDR_LOCK(ifp);
+	if (ia != NULL)
+		ifa_ref(&ia->ia_ifa);
  	if (ia == NULL) {
+		IF_ADDR_LOCK(ifp);
  		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
  			iap = ifatoia(ifa);
  			if (iap->ia_addr.sin_family == AF_INET) {
@@ -336,6 +337,9 @@ in_control(struct socket *so, u_long cmd
  				break;
  			}
  		}
+		if (ia != NULL)
+			ifa_ref(&ia->ia_ifa);
+		IF_ADDR_UNLOCK(ifp);
  	}
  	if (ia == NULL)
  		iaIsFirst = 1;
@@ -345,23 +349,29 @@ in_control(struct socket *so, u_long cmd
  	case SIOCAIFADDR:
  	case SIOCDIFADDR:
  		if (ifra->ifra_addr.sin_family == AF_INET) {
+			struct in_ifaddr *oia;
+
  			for (oia = ia; ia; ia = TAILQ_NEXT(ia, ia_link)) {
  				if (ia->ia_ifp == ifp  &&
  				    ia->ia_addr.sin_addr.s_addr ==
  				    ifra->ifra_addr.sin_addr.s_addr)
  					break;
  			}
+			if (ia != NULL && ia != oia)
+				ifa_ref(&ia->ia_ifa);
+			if (oia != NULL && ia != oia)
+				ifa_free(&oia->ia_ifa);
  			if ((ifp->if_flags & IFF_POINTOPOINT)
  			    && (cmd == SIOCAIFADDR)
  			    && (ifra->ifra_dstaddr.sin_addr.s_addr
  				== INADDR_ANY)) {
  				error = EDESTADDRREQ;
-				goto out_unlock;
+				goto out;
  			}
  		}
  		if (cmd == SIOCDIFADDR && ia == NULL) {
  			error = EADDRNOTAVAIL;
-			goto out_unlock;
+			goto out;
  		}
  		/* FALLTHROUGH */
  	case SIOCSIFADDR:
@@ -373,7 +383,7 @@ in_control(struct socket *so, u_long cmd
  				    M_ZERO);
  			if (ia == NULL) {
  				error = ENOBUFS;
-				goto out_unlock;
+				goto out;
  			}

  			ifa = &ia->ia_ifa;
@@ -390,7 +400,11 @@ in_control(struct socket *so, u_long cmd
  			}
  			ia->ia_ifp = ifp;

+			ifa_ref(ifa);			/* if_addrhead */
+			IF_ADDR_LOCK(ifp);
  			TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link);
+			IF_ADDR_UNLOCK(ifp);
+			ifa_ref(ifa);			/* in_ifaddrhead */
  			s = splnet();
  			TAILQ_INSERT_TAIL(&V_in_ifaddrhead, ia, ia_link);
  			splx(s);
@@ -405,64 +419,53 @@ in_control(struct socket *so, u_long cmd
  	case SIOCGIFBRDADDR:
  		if (ia == NULL) {
  			error = EADDRNOTAVAIL;
-			goto out_unlock;
+			goto out;
  		}
  		break;
  	}

  	/*
-	 * Most paths in this switch return directly or via out_unlock.  Only
-	 * paths that remove the address break in order to hit common removal
-	 * code.
-	 *
-	 * XXXRW: We enter the switch with IF_ADDR_LOCK() held, but leave
-	 * without it.  This is a bug.
+	 * Most paths in this switch return directly or via out.  Only paths
+	 * that remove the address break in order to hit common removal code.
  	 */
-	IF_ADDR_LOCK_ASSERT(ifp);
  	switch (cmd) {
  	case SIOCGIFADDR:
  		*((struct sockaddr_in *)&ifr->ifr_addr) = ia->ia_addr;
-		goto out_unlock;
+		goto out;

  	case SIOCGIFBRDADDR:
  		if ((ifp->if_flags & IFF_BROADCAST) == 0) {
  			error = EINVAL;
-			goto out_unlock;
+			goto out;
  		}
  		*((struct sockaddr_in *)&ifr->ifr_dstaddr) = ia->ia_broadaddr;
-		goto out_unlock;
+		goto out;

  	case SIOCGIFDSTADDR:
  		if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
  			error = EINVAL;
-			goto out_unlock;
+			goto out;
  		}
  		*((struct sockaddr_in *)&ifr->ifr_dstaddr) = ia->ia_dstaddr;
-		goto out_unlock;
+		goto out;

  	case SIOCGIFNETMASK:
  		*((struct sockaddr_in *)&ifr->ifr_addr) = ia->ia_sockmask;
-		goto out_unlock;
+		goto out;

  	case SIOCSIFDSTADDR:
  		if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
  			error = EINVAL;
-			goto out_unlock;
+			goto out;
  		}
  		oldaddr = ia->ia_dstaddr;
  		ia->ia_dstaddr = *(struct sockaddr_in *)&ifr->ifr_dstaddr;
-		IF_ADDR_UNLOCK(ifp);
-
-		/*
-		 * XXXRW: Locks dropped for if_ioctl and rtinit, but ia is
-		 * still being used.
-		 */
  		if (ifp->if_ioctl != NULL) {
  			error = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR,
  			    (caddr_t)ia);
  			if (error) {
  				ia->ia_dstaddr = oldaddr;
-				return (error);
+				goto out;
  			}
  		}
  		if (ia->ia_flags & IFA_ROUTE) {
@@ -472,23 +475,17 @@ in_control(struct socket *so, u_long cmd
  					(struct sockaddr *)&ia->ia_dstaddr;
  			rtinit(&(ia->ia_ifa), (int)RTM_ADD, RTF_HOST|RTF_UP);
  		}
-		return (0);
+		goto out;

  	case SIOCSIFBRDADDR:
  		if ((ifp->if_flags & IFF_BROADCAST) == 0) {
  			error = EINVAL;
-			goto out_unlock;
+			goto out;
  		}
  		ia->ia_broadaddr = *(struct sockaddr_in *)&ifr->ifr_broadaddr;
-		goto out_unlock;
+		goto out;

  	case SIOCSIFADDR:
-		IF_ADDR_UNLOCK(ifp);
-
-		/*
-		 * XXXRW: Locks dropped for in_ifinit and in_joingroup, but ia
-		 * is still being used.
-		 */
  		error = in_ifinit(ifp, ia,
  		    (struct sockaddr_in *) &ifr->ifr_addr, 1);
  		if (error != 0 && iaIsNew)
@@ -502,12 +499,13 @@ in_control(struct socket *so, u_long cmd
  			}
  			EVENTHANDLER_INVOKE(ifaddr_event, ifp);
  		}
-		return (0);
+		error = 0;
+		goto out;

  	case SIOCSIFNETMASK:
  		ia->ia_sockmask.sin_addr = ifra->ifra_addr.sin_addr;
  		ia->ia_subnetmask = ntohl(ia->ia_sockmask.sin_addr.s_addr);
-		goto out_unlock;
+		goto out;

  	case SIOCAIFADDR:
  		maskIsNew = 0;
@@ -521,12 +519,6 @@ in_control(struct socket *so, u_long cmd
  					       ia->ia_addr.sin_addr.s_addr)
  				hostIsNew = 0;
  		}
-		IF_ADDR_UNLOCK(ifp);
-
-		/*
-		 * XXXRW: Locks dropped for in_ifscrub and in_ifinit, but ia
-		 * is still being used.
-		 */
  		if (ifra->ifra_mask.sin_len) {
  			in_ifscrub(ifp, ia);
  			ia->ia_sockmask = ifra->ifra_mask;
@@ -545,7 +537,7 @@ in_control(struct socket *so, u_long cmd
  		    (hostIsNew || maskIsNew))
  			error = in_ifinit(ifp, ia, &ifra->ifra_addr, 0);
  		if (error != 0 && iaIsNew)
-			break;
+			goto out;

  		if ((ifp->if_flags & IFF_BROADCAST) &&
  		    (ifra->ifra_broadaddr.sin_family == AF_INET))
@@ -559,15 +551,10 @@ in_control(struct socket *so, u_long cmd
  			}
  			EVENTHANDLER_INVOKE(ifaddr_event, ifp);
  		}
-		return (error);
+		goto out;

  	case SIOCDIFADDR:
-		IF_ADDR_UNLOCK(ifp);
-
  		/*
-		 * XXXRW: Locks dropped for in_ifscrub and in_ifadown, but ia
-		 * is still being used.
-		 *
  		 * in_ifscrub kills the interface route.
  		 */
  		in_ifscrub(ifp, ia);
@@ -587,25 +574,25 @@ in_control(struct socket *so, u_long cmd
  		panic("in_control: unsupported ioctl");
  	}

-	/*
-	 * XXXRW: In a more ideal world, we would still be holding
-	 * IF_ADDR_LOCK here.
-	 */
  	IF_ADDR_LOCK(ifp);
  	TAILQ_REMOVE(&ifp->if_addrhead, &ia->ia_ifa, ifa_link);
  	IF_ADDR_UNLOCK(ifp);
+	ifa_free(&ia->ia_ifa);				/* if_addrhead */
  	s = splnet();
  	TAILQ_REMOVE(&V_in_ifaddrhead, ia, ia_link);
+	ifa_free(&ia->ia_ifa);				/* in_ifaddrhead */
  	if (ia->ia_addr.sin_family == AF_INET) {
+		struct in_ifaddr *if_ia;
+
  		LIST_REMOVE(ia, ia_hash);
  		/*
  		 * If this is the last IPv4 address configured on this
  		 * interface, leave the all-hosts group.
  		 * No state-change report need be transmitted.
  		 */
-		oia = NULL;
-		IFP_TO_IA(ifp, oia);
-		if (oia == NULL) {
+		if_ia = NULL;
+		IFP_TO_IA(ifp, if_ia);
+		if (if_ia == NULL) {
  			ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]);
  			IN_MULTI_LOCK();
  			if (ii->ii_allhosts) {
@@ -614,15 +601,13 @@ in_control(struct socket *so, u_long cmd
  				ii->ii_allhosts = NULL;
  			}
  			IN_MULTI_UNLOCK();
-		}
+		} else
+			ifa_free(&if_ia->ia_ifa);
  	}
-	ifa_free(&ia->ia_ifa);
  	splx(s);
-
-	return (error);
-
-out_unlock:
-	IF_ADDR_UNLOCK(ifp);
+out:
+	if (ia != NULL)
+		ifa_free(&ia->ia_ifa);
  	return (error);
  }


Modified: head/sys/netinet/in_mcast.c
==============================================================================
--- head/sys/netinet/in_mcast.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/in_mcast.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -1722,6 +1722,7 @@ inp_getmoptions(struct inpcb *inp, struc
  				if (ia != NULL) {
  					mreqn.imr_address =
  					    IA_SIN(ia)->sin_addr;
+					ifa_free(&ia->ia_ifa);
  				}
  			}
  		}

Modified: head/sys/netinet/in_pcb.c
==============================================================================
--- head/sys/netinet/in_pcb.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/in_pcb.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -549,7 +549,6 @@ static int
  in_pcbladdr(struct inpcb *inp, struct in_addr *faddr, struct in_addr *laddr,
      struct ucred *cred)
  {
-	struct in_ifaddr *ia;
  	struct ifaddr *ifa;
  	struct sockaddr *sa;
  	struct sockaddr_in *sin;
@@ -559,7 +558,6 @@ in_pcbladdr(struct inpcb *inp, struct in
  	KASSERT(laddr != NULL, ("%s: laddr NULL", __func__));

  	error = 0;
-	ia = NULL;
  	bzero(&sro, sizeof(sro));

  	sin = (struct sockaddr_in *)&sro.ro_dst;
@@ -585,6 +583,7 @@ in_pcbladdr(struct inpcb *inp, struct in
  	 * the source address from.
  	 */
  	if (sro.ro_rt == NULL || sro.ro_rt->rt_ifp == NULL) {
+		struct in_ifaddr *ia;
  		struct ifnet *ifp;

  		ia = ifatoia(ifa_ifwithdstaddr((struct sockaddr *)sin));
@@ -597,10 +596,12 @@ in_pcbladdr(struct inpcb *inp, struct in

  		if (cred == NULL || !prison_flag(cred, PR_IP4)) {
  			laddr->s_addr = ia->ia_addr.sin_addr.s_addr;
+			ifa_free(&ia->ia_ifa);
  			goto done;
  		}

  		ifp = ia->ia_ifp;
+		ifa_free(&ia->ia_ifa);
  		ia = NULL;
  		IF_ADDR_LOCK(ifp);
  		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
@@ -636,6 +637,7 @@ in_pcbladdr(struct inpcb *inp, struct in
  	 * 3. as a last resort return the 'default' jail address.
  	 */
  	if ((sro.ro_rt->rt_ifp->if_flags & IFF_LOOPBACK) == 0) {
+		struct in_ifaddr *ia;
  		struct ifnet *ifp;

  		/* If not jailed, use the default returned. */
@@ -658,10 +660,10 @@ in_pcbladdr(struct inpcb *inp, struct in
  		 * 2. Check if we have any address on the outgoing interface
  		 *    belonging to this jail.
  		 */
+		ia = NULL;
  		ifp = sro.ro_rt->rt_ifp;
  		IF_ADDR_LOCK(ifp);
  		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
-
  			sa = ifa->ifa_addr;
  			if (sa->sa_family != AF_INET)
  				continue;
@@ -694,6 +696,7 @@ in_pcbladdr(struct inpcb *inp, struct in
  	 */
  	if ((sro.ro_rt->rt_ifp->if_flags & IFF_LOOPBACK) != 0) {
  		struct sockaddr_in sain;
+		struct in_ifaddr *ia;

  		bzero(&sain, sizeof(struct sockaddr_in));
  		sain.sin_family = AF_INET;
@@ -710,6 +713,7 @@ in_pcbladdr(struct inpcb *inp, struct in
  				goto done;
  			}
  			laddr->s_addr = ia->ia_addr.sin_addr.s_addr;
+			ifa_free(&ia->ia_ifa);
  			goto done;
  		}

@@ -718,6 +722,7 @@ in_pcbladdr(struct inpcb *inp, struct in
  			struct ifnet *ifp;

  			ifp = ia->ia_ifp;
+			ifa_free(&ia->ia_ifa);
  			ia = NULL;
  			IF_ADDR_LOCK(ifp);
  			TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {

Modified: head/sys/netinet/in_var.h
==============================================================================
--- head/sys/netinet/in_var.h	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/in_var.h	Tue Jun 23 20:19:09 2009	(r194760)
@@ -146,14 +146,16 @@ do { \
   * Macro for finding the internet address structure (in_ifaddr) corresponding
   * to a given interface (ifnet structure).
   */
-#define IFP_TO_IA(ifp, ia) \
-	/* struct ifnet *ifp; */ \
-	/* struct in_ifaddr *ia; */ \
-{ \
-	for ((ia) = TAILQ_FIRST(&V_in_ifaddrhead); \
-	    (ia) != NULL && (ia)->ia_ifp != (ifp); \
-	    (ia) = TAILQ_NEXT((ia), ia_link)) \
-		continue; \
+#define IFP_TO_IA(ifp, ia)						\
+	/* struct ifnet *ifp; */					\
+	/* struct in_ifaddr *ia; */					\
+{									\
+	for ((ia) = TAILQ_FIRST(&V_in_ifaddrhead);			\
+	    (ia) != NULL && (ia)->ia_ifp != (ifp);			\
+	    (ia) = TAILQ_NEXT((ia), ia_link))				\
+		continue;						\
+	if ((ia) != NULL)						\
+		ifa_ref(&(ia)->ia_ifa);					\
  }
  #endif


Modified: head/sys/netinet/ip_carp.c
==============================================================================
--- head/sys/netinet/ip_carp.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/ip_carp.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -1239,6 +1239,7 @@ carp_iamatch6(void *v, struct in6_addr *
   			    (SC2IFP(vh)->if_flags & IFF_UP) &&
  			    (SC2IFP(vh)->if_drv_flags & IFF_DRV_RUNNING) &&
  			    vh->sc_state == MASTER) {
+				ifa_ref(ifa);
  				IF_ADDR_UNLOCK(SC2IFP(vh));
  			    	CARP_UNLOCK(cif);
  				return (ifa);

Modified: head/sys/netinet/ip_divert.c
==============================================================================
--- head/sys/netinet/ip_divert.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/ip_divert.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -464,6 +464,7 @@ div_output(struct socket *so, struct mbu
  				goto cantsend;
  			}
  			m->m_pkthdr.rcvif = ifa->ifa_ifp;
+			ifa_free(ifa);
  		}
  #ifdef MAC
  		mac_socket_create_mbuf(so, m);

Modified: head/sys/netinet/ip_icmp.c
==============================================================================
--- head/sys/netinet/ip_icmp.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/ip_icmp.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -536,10 +536,12 @@ icmp_input(struct mbuf *m, int off)
  		}
  		ia = (struct in_ifaddr *)ifaof_ifpforaddr(
  			    (struct sockaddr *)&icmpdst, m->m_pkthdr.rcvif);
-		if (ia == 0)
+		if (ia == NULL)
  			break;
-		if (ia->ia_ifp == 0)
+		if (ia->ia_ifp == NULL) {
+			ifa_free(&ia->ia_ifa);
  			break;
+		}
  		icp->icmp_type = ICMP_MASKREPLY;
  		if (V_icmpmaskfake == 0)
  			icp->icmp_mask = ia->ia_sockmask.sin_addr.s_addr;
@@ -551,6 +553,7 @@ icmp_input(struct mbuf *m, int off)
  			else if (ia->ia_ifp->if_flags & IFF_POINTOPOINT)
  			    ip->ip_src = satosin(&ia->ia_dstaddr)->sin_addr;
  		}
+		ifa_free(&ia->ia_ifa);
  reflect:
  		ip->ip_len += hlen;	/* since ip_input deducts this */
  		ICMPSTAT_INC(icps_reflect);
@@ -748,6 +751,7 @@ icmp_reflect(struct mbuf *m)
  		goto done;
  	}
  	t = IA_SIN(ia)->sin_addr;
+	ifa_free(&ia->ia_ifa);
  match:
  #ifdef MAC
  	mac_netinet_icmp_replyinplace(m);

Modified: head/sys/netinet/ip_input.c
==============================================================================
--- head/sys/netinet/ip_input.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/ip_input.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -622,8 +622,10 @@ passin:
  		 * enabled.
  		 */
  		if (IA_SIN(ia)->sin_addr.s_addr == ip->ip_dst.s_addr &&
-		    (!checkif || ia->ia_ifp == ifp))
+		    (!checkif || ia->ia_ifp == ifp)) {
+			ifa_ref(&ia->ia_ifa);
  			goto ours;
+		}
  	}
  	/*
  	 * Check for broadcast addresses.
@@ -641,15 +643,18 @@ passin:
  			ia = ifatoia(ifa);
  			if (satosin(&ia->ia_broadaddr)->sin_addr.s_addr ==
  			    ip->ip_dst.s_addr) {
+				ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto ours;
  			}
  			if (ia->ia_netbroadcast.s_addr == ip->ip_dst.s_addr) {
+				ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto ours;
  			}
  #ifdef BOOTP_COMPAT
  			if (IA_SIN(ia)->sin_addr.s_addr == INADDR_ANY) {
+				ifa_ref(ifa);
  				IF_ADDR_UNLOCK(ifp);
  				goto ours;
  			}
@@ -742,6 +747,7 @@ ours:
  	if (ia != NULL) {
  		ia->ia_ifa.if_ipackets++;
  		ia->ia_ifa.if_ibytes += m->m_pkthdr.len;
+		ifa_free(&ia->ia_ifa);
  	}

  	/*
@@ -1335,8 +1341,8 @@ ipproto_unregister(u_char ipproto)
  }

  /*
- * Given address of next destination (final or next hop),
- * return internet address info of interface to be used to get there.
+ * Given address of next destination (final or next hop), return (referenced)
+ * internet address info of interface to be used to get there.
   */
  struct in_ifaddr *
  ip_rtaddr(struct in_addr dst, u_int fibnum)
@@ -1356,6 +1362,7 @@ ip_rtaddr(struct in_addr dst, u_int fibn
  		return (NULL);

  	ifa = ifatoia(sro.ro_rt->rt_ifa);
+	ifa_ref(&ifa->ia_ifa);
  	RTFREE(sro.ro_rt);
  	return (ifa);
  }
@@ -1530,11 +1537,16 @@ ip_forward(struct mbuf *m, int srcrt)
  		else {
  			if (mcopy)
  				m_freem(mcopy);
+			if (ia != NULL)
+				ifa_free(&ia->ia_ifa);
  			return;
  		}
  	}
-	if (mcopy == NULL)
+	if (mcopy == NULL) {
+		if (ia != NULL)
+			ifa_free(&ia->ia_ifa);
  		return;
+	}

  	switch (error) {

@@ -1592,6 +1604,8 @@ ip_forward(struct mbuf *m, int srcrt)
  		 */
  		if (V_ip_sendsourcequench == 0) {
  			m_freem(mcopy);
+			if (ia != NULL)
+				ifa_free(&ia->ia_ifa);
  			return;
  		} else {
  			type = ICMP_SOURCEQUENCH;
@@ -1601,8 +1615,12 @@ ip_forward(struct mbuf *m, int srcrt)

  	case EACCES:			/* ipfw denied packet */
  		m_freem(mcopy);
+		if (ia != NULL)
+			ifa_free(&ia->ia_ifa);
  		return;
  	}
+	if (ia != NULL)
+		ifa_free(&ia->ia_ifa);
  	icmp_error(mcopy, type, code, dest.s_addr, mtu);
  }


Modified: head/sys/netinet/ip_mroute.c
==============================================================================
--- head/sys/netinet/ip_mroute.c	Tue Jun 23 20:19:02 2009	(r194759)
+++ head/sys/netinet/ip_mroute.c	Tue Jun 23 20:19:09 2009	(r194760)
@@ -883,6 +883,7 @@ add_vif(struct vifctl *vifcp)
  	    return EADDRNOTAVAIL;
  	}
  	ifp = ifa->ifa_ifp;
+	ifa_free(ifa);
      }

      if ((vifcp->vifc_flags & VIFF_TUNNEL) != 0) {

Modified: head/sys/netinet/ip_options.c
==============================================================================
--- head/sys/netinet/ip_options.c	Tue Jun 23 20:19:02 2009	(r194759)

*** DIFF OUTPUT TRUNCATED AT 1000 LINES ***



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.0906232120400.13224>