From owner-freebsd-net@FreeBSD.ORG Wed Jan 4 00:50:58 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 66EE01065675; Wed, 4 Jan 2012 00:50:58 +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 DBB3E8FC0A; Wed, 4 Jan 2012 00:50:57 +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 D888F25D3810; Wed, 4 Jan 2012 00:50:56 +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 F0456BD770A; Wed, 4 Jan 2012 00:50:55 +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 UKAmUWi2Y1Gx; Wed, 4 Jan 2012 00:50:53 +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 8D395BD7708; Wed, 4 Jan 2012 00:50:53 +0000 (UTC) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: "Bjoern A. Zeeb" In-Reply-To: <201201031645.32590.jhb@freebsd.org> Date: Wed, 4 Jan 2012 00:50:53 +0000 Content-Transfer-Encoding: quoted-printable Message-Id: <35C8CBEE-0C97-48B5-8E4D-633A35B34D66@FreeBSD.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> <20111229225539.GD12721@FreeBSD.org> <201201031645.32590.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: Wed, 04 Jan 2012 00:50:58 -0000 On 3. Jan 2012, at 21:45 , John Baldwin wrote: > On Thursday, December 29, 2011 5:55:39 pm Gleb Smirnoff wrote: >> Reviewing your patch I've found several problems not introduced by = it, >> but already existing, and somewhat related to your patch: >>=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 > Ok, I think I have a patch to fix the middle one. I've (ab)used the = list used > to defer calls to in6m_release_locked() and used it to defer calls to > mld_v1_transmit_report() from under the loop. Note that the v2 timers = in > the same loop already use this list to defer calls to = in6m_release_locked() > so it should be safe to use the temporary list for v1 as well. It seems to look fine indeed. > Index: 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 > --- mld6.c (revision 229420) > +++ mld6.c (working copy) > @@ -121,7 +121,8 @@ static int mld_v1_input_query(struct ifnet = *, cons > /*const*/ struct mld_hdr *); > static int mld_v1_input_report(struct ifnet *, const struct ip6_hdr = *, > /*const*/ struct mld_hdr *); > -static void mld_v1_process_group_timer(struct in6_multi *, const = int); > +static void mld_v1_process_group_timer(struct mld_ifinfo *, > + struct in6_multi *); > static void mld_v1_process_querier_timers(struct mld_ifinfo *); > static int mld_v1_transmit_report(struct in6_multi *, const int); > static void mld_v1_update_group(struct in6_multi *, const int); > @@ -1336,8 +1337,8 @@ mld_fasttimo_vnet(void) > struct ifqueue qrq; /* Query response packets */ > struct ifnet *ifp; > struct mld_ifinfo *mli; > - struct ifmultiaddr *ifma, *tifma; > - struct in6_multi *inm; > + struct ifmultiaddr *ifma; > + struct in6_multi *inm, *tinm; > int uri_fasthz; >=20 > uri_fasthz =3D 0; > @@ -1401,24 +1402,14 @@ mld_fasttimo_vnet(void) > } >=20 > IF_ADDR_LOCK(ifp); > - TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, > - tifma) { > + TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { > if (ifma->ifma_addr->sa_family !=3D AF_INET6 || > ifma->ifma_protospec =3D=3D NULL) > continue; > inm =3D (struct in6_multi = *)ifma->ifma_protospec; > switch (mli->mli_version) { > case MLD_VERSION_1: > - /* > - * XXX Drop IF_ADDR lock temporarily to > - * avoid recursion caused by a potential > - * call by in6ifa_ifpforlinklocal(). > - * rwlock candidate? > - */ > - IF_ADDR_UNLOCK(ifp); > - mld_v1_process_group_timer(inm, > - mli->mli_version); > - IF_ADDR_LOCK(ifp); > + mld_v1_process_group_timer(mli, inm); > break; > case MLD_VERSION_2: > mld_v2_process_group_timers(mli, &qrq, > @@ -1428,9 +1419,25 @@ mld_fasttimo_vnet(void) > } > IF_ADDR_UNLOCK(ifp); >=20 > - if (mli->mli_version =3D=3D MLD_VERSION_2) { > - struct in6_multi *tinm; > - > + switch (mli->mli_version) { > + case MLD_VERSION_1: > + /* > + * Transmit reports for this lifecycle. This > + * is done while not holding IF_ADDR_LOCK > + * since this can call > + * in6ifa_ifpforlinklocal() which locks > + * IF_ADDR_LOCK internally as well as > + * ip6_output() to transmit a packet. > + */ > + SLIST_FOREACH_SAFE(inm, &mli->mli_relinmhead, > + in6m_nrele, tinm) { > + SLIST_REMOVE_HEAD(&mli->mli_relinmhead, > + in6m_nrele); > + (void)mld_v1_transmit_report(inm, > + MLD_LISTENER_REPORT); > + } > + break; > + case MLD_VERSION_2: > mld_dispatch_queue(&qrq, 0); > mld_dispatch_queue(&scq, 0); >=20 > @@ -1444,6 +1451,7 @@ mld_fasttimo_vnet(void) > in6m_nrele); > in6m_release_locked(inm); > } > + break; > } > } >=20 > @@ -1457,7 +1465,7 @@ out_locked: > * Will update the global pending timer flags. > */ > static void > -mld_v1_process_group_timer(struct in6_multi *inm, const int version) > +mld_v1_process_group_timer(struct mld_ifinfo *mli, struct in6_multi = *inm) > { > int report_timer_expired; >=20 > @@ -1484,8 +1492,8 @@ static void > case MLD_REPORTING_MEMBER: > if (report_timer_expired) { > inm->in6m_state =3D MLD_IDLE_MEMBER; > - (void)mld_v1_transmit_report(inm, > - MLD_LISTENER_REPORT); > + SLIST_INSERT_HEAD(&mli->mli_relinmhead, inm, > + in6m_nrele); > } > break; > case MLD_G_QUERY_PENDING_MEMBER: >=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!