From owner-freebsd-net@FreeBSD.ORG Tue Jan 3 20:06:30 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 64EA31065670; Tue, 3 Jan 2012 20:06:30 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from mx1.sbone.de (mx1.sbone.de [IPv6:2a01:4f8:130:3ffc::401:25]) by mx1.freebsd.org (Postfix) with ESMTP id DDC808FC0C; Tue, 3 Jan 2012 20:06:29 +0000 (UTC) Received: from mail.sbone.de (mail.sbone.de [IPv6:fde9:577b:c1a9:31::2013:587]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.sbone.de (Postfix) with ESMTPS id 8D25925D385D; Tue, 3 Jan 2012 20:06:28 +0000 (UTC) Received: from content-filter.sbone.de (content-filter.sbone.de [IPv6:fde9:577b:c1a9:31::2013:2742]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPS id B70BFBD8682; Tue, 3 Jan 2012 20:06:27 +0000 (UTC) X-Virus-Scanned: amavisd-new at sbone.de Received: from mail.sbone.de ([IPv6:fde9:577b:c1a9:31::2013:587]) by content-filter.sbone.de (content-filter.sbone.de [fde9:577b:c1a9:31::2013:2742]) (amavisd-new, port 10024) with ESMTP id 3oTE4JI-63YH; Tue, 3 Jan 2012 20:06:25 +0000 (UTC) Received: from orange-en1.sbone.de (orange-en1.sbone.de [IPv6:fde9:577b:c1a9:31:cabc:c8ff:fecf:e8e3]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPSA id B4851BD8681; Tue, 3 Jan 2012 20:06:25 +0000 (UTC) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: "Bjoern A. Zeeb" In-Reply-To: <201201031123.46973.jhb@freebsd.org> Date: Tue, 3 Jan 2012 20:06:24 +0000 Content-Transfer-Encoding: quoted-printable Message-Id: <084220A8-F373-4344-9C9A-E0907C926CC0@FreeBSD.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> <20111229225539.GD12721@FreeBSD.org> <201201031123.46973.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1084) 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 20:06:30 -0000 On 3. Jan 2012, at 16:23 , John Baldwin wrote: > 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. >>=20 >> Reviewing your patch I've found several problems not introduced by = it, >> but already existing, and somewhat related to your patch: >>=20 >> 1) Unneeded use of _SAFE version of TAILQ: >>=20 >> igmp.c:3338 >> in6.c:1339 >> mld6.c:2993 >=20 > I'll fix these. >=20 >> 2) Potential race when dropping a lock inside FOREACH loop: >>=20 >> igmp.c:2058 >> mld6.c:1419 >> mld6.c:1704 (this one isn't even a SAFE, so would crash earlier) >=20 > 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 >=20 >> 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. >=20 > This looks good to me. Maybe you can get bz@ to review it? Will look at that one next. The two below seem fine. Would have loved the lock assertions that the mld6 one has on igmp as well but that's a different (unrelated) story:-) /bz > Index: netinet/igmp.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 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; >=20 > 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); > + } > } >=20 > /* > Index: netinet6/mld6.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 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; >=20 > 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); > + } > } >=20 > /* >=20 > --=20 > John Baldwin > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" --=20 Bjoern A. Zeeb You have to have visions! It does not matter how good you are. It matters what good you do!