From owner-freebsd-net@FreeBSD.ORG Tue Jan 3 16:23:54 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 11B9E10656AE; Tue, 3 Jan 2012 16:23:54 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id BF84B8FC1E; Tue, 3 Jan 2012 16:23:53 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 5C5B746B0A; Tue, 3 Jan 2012 11:23:53 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id C4489B960; Tue, 3 Jan 2012 11:23:52 -0500 (EST) From: John Baldwin To: Gleb Smirnoff Date: Tue, 3 Jan 2012 11:23:46 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> <20111229225539.GD12721@FreeBSD.org> In-Reply-To: <20111229225539.GD12721@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201201031123.46973.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 03 Jan 2012 11:23:52 -0500 (EST) Cc: freebsd-net@freebsd.org, Robert Watson Subject: Re: Transitioning if_addr_lock to an rwlock X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Jan 2012 16:23:54 -0000 On Thursday, December 29, 2011 5:55:39 pm Gleb Smirnoff wrote: > On Thu, Dec 29, 2011 at 03:27:26PM -0500, John Baldwin wrote: > J> - if_addr_uses.patch This changes callers of the existing macros to use > J> either read or write locks. This is the patch that > J> could use the most review. > > Reviewing your patch I've found several problems not introduced by it, > but already existing, and somewhat related to your patch: > > 1) Unneeded use of _SAFE version of TAILQ: > > igmp.c:3338 > in6.c:1339 > mld6.c:2993 I'll fix these. > 2) Potential race when dropping a lock inside FOREACH loop: > > igmp.c:2058 > mld6.c:1419 > mld6.c:1704 (this one isn't even a SAFE, so would crash earlier) These are not easy to fix. :( Dropping the lock is of course the wrong thing to do. However, there are a few layering violations that make this hard to fix properly. Actually, we might be able to use an approach similar to that used in mld_ifdetach() and igmp_ifdetach() to fix the cancel timers cases. Patch below > 3) I've found that in6_ifawithifp() doesn't do what it is supposed > to, as well as uses incorrect locking during this. As last resort > it should run through global list of addresses, not run throgh the > ifp one again. Patch attached. This looks good to me. Maybe you can get bz@ to review it? Index: netinet/igmp.c =================================================================== --- netinet/igmp.c (revision 229389) +++ netinet/igmp.c (working copy) @@ -2004,7 +2003,7 @@ { struct ifmultiaddr *ifma; struct ifnet *ifp; - struct in_multi *inm; + struct in_multi *inm, *tinm; CTR3(KTR_IGMPV3, "%s: cancel v3 timers on ifp %p(%s)", __func__, igi->igi_ifp, igi->igi_ifp->if_xname); @@ -2050,14 +2049,8 @@ * transition to REPORTING to ensure the host leave * message is sent upstream to the old querier -- * transition to NOT would lose the leave and race. - * - * SMPNG: Must drop and re-acquire IF_ADDR_LOCK - * around inm_release_locked(), as it is not - * a recursive mutex. */ - IF_ADDR_UNLOCK(ifp); - inm_release_locked(inm); - IF_ADDR_LOCK(ifp); + SLIST_INSERT_HEAD(&igi->igi_relinmhead, inm, inm_nrele); /* FALLTHROUGH */ case IGMP_G_QUERY_PENDING_MEMBER: case IGMP_SG_QUERY_PENDING_MEMBER: @@ -2076,6 +2069,10 @@ _IF_DRAIN(&inm->inm_scq); } IF_ADDR_UNLOCK(ifp); + SLIST_FOREACH_SAFE(inm, &igi->igi_relinmhead, inm_nrele, tinm) { + SLIST_REMOVE_HEAD(&igi->igi_relinmhead, inm_nrele); + inm_release_locked(inm); + } } /* Index: netinet6/mld6.c =================================================================== --- netinet6/mld6.c (revision 229389) +++ netinet6/mld6.c (working copy) @@ -1656,7 +1656,7 @@ { struct ifmultiaddr *ifma; struct ifnet *ifp; - struct in6_multi *inm; + struct in6_multi *inm, *tinm; CTR3(KTR_MLD, "%s: cancel v2 timers on ifp %p(%s)", __func__, mli->mli_ifp, mli->mli_ifp->if_xname); @@ -1695,14 +1695,9 @@ * If we are leaving the group and switching * version, we need to release the final * reference held for issuing the INCLUDE {}. - * - * SMPNG: Must drop and re-acquire IF_ADDR_LOCK - * around in6m_release_locked(), as it is not - * a recursive mutex. */ - IF_ADDR_UNLOCK(ifp); - in6m_release_locked(inm); - IF_ADDR_LOCK(ifp); + SLIST_INSERT_HEAD(&mli->mli_relinmhead, inm, + in6m_nrele); /* FALLTHROUGH */ case MLD_G_QUERY_PENDING_MEMBER: case MLD_SG_QUERY_PENDING_MEMBER: @@ -1720,6 +1715,10 @@ } } IF_ADDR_UNLOCK(ifp); + SLIST_FOREACH_SAFE(inm, &mli->mli_relinmhead, in6m_nrele, tinm) { + SLIST_REMOVE_HEAD(&mli->mli_relinmhead, in6m_nrele); + in6m_release_locked(inm); + } } /* -- John Baldwin