Avoid changesets r194760-r194736 (was: Re: Warning: ifaddr refcount
use patch (svn commit: r194760 - in head/sys: contrib/rdma net
net80211 netinet netinet6 netipx (fwd)))
Robert Watson
rwatson at FreeBSD.org
Wed Jun 24 15:11:17 UTC 2009
On Tue, 23 Jun 2009, Robert Watson wrote:
> However, because this modifies quite a few spots in the code, it's possible
> we'll see two classes of bugs:
Indeed, we've run into a couple of problems so far relating to multicast input
with these changes; symptoms consisted of panics when receiving multicast IP
input. Many users will want to skip stright to r194837 or later, and skip
revisions r194760-r194736. Thanks to Sam Leffler and Lawrence Stewart for
reports!
Robert N M Watson
Computer Laboratory
University of Cambridge
>
> - 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 at FreeBSD.org>
> To: src-committers at freebsd.org, svn-src-all at freebsd.org,
> svn-src-head at 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 ***
> _______________________________________________
> freebsd-current at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe at freebsd.org"
>
More information about the freebsd-current
mailing list