Date: Tue, 3 Jan 2012 11:23:46 -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: <201201031123.46973.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:
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201031123.46973.jhb>