Date: Tue, 17 Mar 2009 14:41:54 +0000 (UTC) From: Bruce M Simpson <bms@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r189931 - head/sys/netinet Message-ID: <200903171441.n2HEfsam097762@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bms Date: Tue Mar 17 14:41:54 2009 New Revision: 189931 URL: http://svn.freebsd.org/changeset/base/189931 Log: Deal with the case where ifma_protospec may be NULL, during any IPv4 multicast operations which reference it. There is a potential race because ifma_protospec is set to NULL when we discover the underlying ifnet has gone away. This write is not covered by the IF_ADDR_LOCK, and it's difficult to widen its scope without making it a recursive lock. It isn't clear why this manifests more quickly with 802.11 interfaces, but does not seem to manifest at all with wired interfaces. With this change, the 802.11 related panics reported by sam@ and cokane@ should go away. It is not the right fix, that requires more thought before 8.0. Idea from: sam Tested by: cokane Modified: head/sys/netinet/igmp.c head/sys/netinet/in.c head/sys/netinet/in_mcast.c Modified: head/sys/netinet/igmp.c ============================================================================== --- head/sys/netinet/igmp.c Tue Mar 17 14:29:25 2009 (r189930) +++ head/sys/netinet/igmp.c Tue Mar 17 14:41:54 2009 (r189931) @@ -183,6 +183,11 @@ static int vnet_igmp_idetach(const void * VIMAGE: Each in_multi corresponds to an ifp, and each ifp corresponds * to a vnet in ifp->if_vnet. * + * SMPng: XXX We may potentially race operations on ifma_protospec. + * The problem is that we currently lack a clean way of taking the + * IF_ADDR_LOCK() between the ifnet and in layers w/o recursing, + * as anything which modifies ifma needs to be covered by that lock. + * So check for ifma_protospec being NULL before proceeding. */ struct mtx igmp_mtx; int mpsafe_igmp = 0; @@ -601,6 +606,7 @@ out: * is detached, but also before the link layer does its cleanup. * * SMPNG: igmp_ifdetach() needs to take IF_ADDR_LOCK(). + * XXX This is also bitten by unlocked ifma_protospec access. * * VIMAGE: curvnet should have been set by caller, but let's not assume * that for now. @@ -623,8 +629,13 @@ igmp_ifdetach(struct ifnet *ifp) if (igi->igi_version == IGMP_VERSION_3) { IF_ADDR_LOCK(ifp); TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { - if (ifma->ifma_addr->sa_family != AF_INET) + if (ifma->ifma_addr->sa_family != AF_INET || + ifma->ifma_protospec == NULL) continue; +#if 0 + KASSERT(ifma->ifma_protospec != NULL, + ("%s: ifma_protospec is NULL", __func__)); +#endif inm = (struct in_multi *)ifma->ifma_protospec; if (inm->inm_state == IGMP_LEAVING_MEMBER) { SLIST_INSERT_HEAD(&igi->igi_relinmhead, @@ -783,7 +794,8 @@ igmp_input_v1_query(struct ifnet *ifp, c */ IF_ADDR_LOCK(ifp); TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { - if (ifma->ifma_addr->sa_family != AF_INET) + if (ifma->ifma_addr->sa_family != AF_INET || + ifma->ifma_protospec == NULL) continue; inm = (struct in_multi *)ifma->ifma_protospec; if (inm->inm_timer != 0) @@ -880,7 +892,8 @@ igmp_input_v2_query(struct ifnet *ifp, c IF_ADDR_LOCK(ifp); TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { - if (ifma->ifma_addr->sa_family != AF_INET) + if (ifma->ifma_addr->sa_family != AF_INET || + ifma->ifma_protospec == NULL) continue; inm = (struct in_multi *)ifma->ifma_protospec; igmp_v2_update_group(inm, timer); @@ -1701,7 +1714,8 @@ igmp_fasttimo_vnet(void) IF_ADDR_LOCK(ifp); TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, tifma) { - if (ifma->ifma_addr->sa_family != AF_INET) + if (ifma->ifma_addr->sa_family != AF_INET || + ifma->ifma_protospec == NULL) continue; inm = (struct in_multi *)ifma->ifma_protospec; switch (igi->igi_version) { @@ -3311,7 +3325,8 @@ igmp_v3_dispatch_general_query(struct ig IF_ADDR_LOCK(ifp); TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, tifma) { - if (ifma->ifma_addr->sa_family != AF_INET) + if (ifma->ifma_addr->sa_family != AF_INET || + ifma->ifma_protospec == NULL) continue; inm = (struct in_multi *)ifma->ifma_protospec; Modified: head/sys/netinet/in.c ============================================================================== --- head/sys/netinet/in.c Tue Mar 17 14:29:25 2009 (r189930) +++ head/sys/netinet/in.c Tue Mar 17 14:41:54 2009 (r189931) @@ -1011,6 +1011,8 @@ in_ifdetach(struct ifnet *ifp) * Delete all IPv4 multicast address records, and associated link-layer * multicast address records, associated with ifp. * XXX It looks like domifdetach runs AFTER the link layer cleanup. + * XXX This should not race with ifma_protospec being set during + * a new allocation, if it does, we have bigger problems. */ static void in_purgemaddrs(struct ifnet *ifp) @@ -1031,8 +1033,13 @@ in_purgemaddrs(struct ifnet *ifp) */ IF_ADDR_LOCK(ifp); TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { - if (ifma->ifma_addr->sa_family != AF_INET) + if (ifma->ifma_addr->sa_family != AF_INET || + ifma->ifma_protospec == NULL) continue; +#if 0 + KASSERT(ifma->ifma_protospec != NULL, + ("%s: ifma_protospec is NULL", __func__)); +#endif inm = (struct in_multi *)ifma->ifma_protospec; LIST_INSERT_HEAD(&purgeinms, inm, inm_link); } Modified: head/sys/netinet/in_mcast.c ============================================================================== --- head/sys/netinet/in_mcast.c Tue Mar 17 14:29:25 2009 (r189930) +++ head/sys/netinet/in_mcast.c Tue Mar 17 14:41:54 2009 (r189931) @@ -432,6 +432,9 @@ in_getmulti(struct ifnet *ifp, const str if (error != 0) return (error); + /* XXX ifma_protospec must be covered by IF_ADDR_LOCK */ + IF_ADDR_LOCK(ifp); + /* * If something other than netinet is occupying the link-layer * group, print a meaningful error message and back out of @@ -454,9 +457,12 @@ in_getmulti(struct ifnet *ifp, const str #endif ++inm->inm_refcount; *pinm = inm; + IF_ADDR_UNLOCK(ifp); return (0); } + IF_ADDR_LOCK_ASSERT(ifp); + /* * A new in_multi record is needed; allocate and initialize it. * We DO NOT perform an IGMP join as the in_ layer may need to @@ -467,6 +473,7 @@ in_getmulti(struct ifnet *ifp, const str inm = malloc(sizeof(*inm), M_IPMADDR, M_NOWAIT | M_ZERO); if (inm == NULL) { if_delmulti_ifma(ifma); + IF_ADDR_UNLOCK(ifp); return (ENOMEM); } inm->inm_addr = *group; @@ -489,6 +496,7 @@ in_getmulti(struct ifnet *ifp, const str *pinm = inm; + IF_ADDR_UNLOCK(ifp); return (0); } @@ -522,6 +530,7 @@ inm_release_locked(struct in_multi *inm) ifma = inm->inm_ifma; + /* XXX this access is not covered by IF_ADDR_LOCK */ CTR2(KTR_IGMPV3, "%s: purging ifma %p", __func__, ifma); KASSERT(ifma->ifma_protospec == inm, ("%s: ifma_protospec != inm", __func__));
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200903171441.n2HEfsam097762>