Date: Tue, 3 Jan 2012 16:45:32 -0500 From: John Baldwin <jhb@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: freebsd-net@freebsd.org, Robert Watson <rwatson@freebsd.org> Subject: Re: Transitioning if_addr_lock to an rwlock Message-ID: <201201031645.32590.jhb@freebsd.org> In-Reply-To: <20111229225539.GD12721@FreeBSD.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> <20111229225539.GD12721@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
>
> 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)
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.
Index: mld6.c
===================================================================
--- 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;
uri_fasthz = 0;
@@ -1401,24 +1402,14 @@ mld_fasttimo_vnet(void)
}
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 != AF_INET6 ||
ifma->ifma_protospec == NULL)
continue;
inm = (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);
- if (mli->mli_version == 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);
@@ -1444,6 +1451,7 @@ mld_fasttimo_vnet(void)
in6m_nrele);
in6m_release_locked(inm);
}
+ break;
}
}
@@ -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;
@@ -1484,8 +1492,8 @@ static void
case MLD_REPORTING_MEMBER:
if (report_timer_expired) {
inm->in6m_state = 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:
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201031645.32590.jhb>