Date: Wed, 4 Jan 2012 00:50:53 +0000 From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-net@freebsd.org, Robert Watson <rwatson@freebsd.org> Subject: Re: Transitioning if_addr_lock to an rwlock Message-ID: <35C8CBEE-0C97-48B5-8E4D-633A35B34D66@FreeBSD.org> In-Reply-To: <201201031645.32590.jhb@freebsd.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> <20111229225539.GD12721@FreeBSD.org> <201201031645.32590.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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!
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?35C8CBEE-0C97-48B5-8E4D-633A35B34D66>