From owner-freebsd-current@FreeBSD.ORG Tue Jun 23 20:24:29 2009 Return-Path: Delivered-To: current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 55AEC1065673 for ; Tue, 23 Jun 2009 20:24:29 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id EA0FF8FC1F for ; Tue, 23 Jun 2009 20:24:28 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id 8AEA646B17 for ; Tue, 23 Jun 2009 16:24:28 -0400 (EDT) Date: Tue, 23 Jun 2009 21:24:28 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: current@FreeBSD.org Message-ID: User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Subject: Warning: ifaddr refcount use patch (svn commit: r194760 - in head/sys: contrib/rdma net net80211 netinet netinet6 netipx (fwd)) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Jun 2009 20:24:30 -0000 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 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 #include #include +#include #include #include #include @@ -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 ***