Date: Fri, 30 Dec 2011 02:55:39 +0400 From: Gleb Smirnoff <glebius@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: <20111229225539.GD12721@FreeBSD.org> In-Reply-To: <201112291527.26763.jhb@freebsd.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--3MwIy2ne0vdjdPXF
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
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
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)
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.
--
Totus tuus, Glebius.
--3MwIy2ne0vdjdPXF
Content-Type: text/x-diff; charset=koi8-r
Content-Disposition: attachment; filename="in6.c.diff"
Index: in6.c
===================================================================
--- in6.c (revision 228971)
+++ in6.c (working copy)
@@ -2188,10 +2188,10 @@
{
int dst_scope = in6_addrscope(dst), blen = -1, tlen;
struct ifaddr *ifa;
- struct in6_ifaddr *besta = 0;
+ struct in6_ifaddr *ia;
struct in6_ifaddr *dep[2]; /* last-resort: deprecated */
- dep[0] = dep[1] = NULL;
+ ia = dep[0] = dep[1] = NULL;
/*
* We first look for addresses in the same scope.
@@ -2219,45 +2219,43 @@
/*
* call in6_matchlen() as few as possible
*/
- if (besta) {
+ if (ia) {
if (blen == -1)
- blen = in6_matchlen(&besta->ia_addr.sin6_addr, dst);
+ blen = in6_matchlen(&ia->ia_addr.sin6_addr, dst);
tlen = in6_matchlen(IFA_IN6(ifa), dst);
if (tlen > blen) {
blen = tlen;
- besta = (struct in6_ifaddr *)ifa;
+ ia = (struct in6_ifaddr *)ifa;
}
} else
- besta = (struct in6_ifaddr *)ifa;
+ ia = (struct in6_ifaddr *)ifa;
}
}
- if (besta) {
- ifa_ref(&besta->ia_ifa);
+ if (ia) {
+ ifa_ref(&ia->ia_ifa);
IF_ADDR_UNLOCK(ifp);
- return (besta);
+ return (ia);
}
IF_ADDR_UNLOCK(ifp);
IN6_IFADDR_RLOCK();
- TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
- if (ifa->ifa_addr->sa_family != AF_INET6)
- continue;
- if (((struct in6_ifaddr *)ifa)->ia6_flags & IN6_IFF_ANYCAST)
+ TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) {
+ if (ia->ia6_flags & IN6_IFF_ANYCAST)
continue; /* XXX: is there any case to allow anycast? */
- if (((struct in6_ifaddr *)ifa)->ia6_flags & IN6_IFF_NOTREADY)
+ if (ia->ia6_flags & IN6_IFF_NOTREADY)
continue; /* don't use this interface */
- if (((struct in6_ifaddr *)ifa)->ia6_flags & IN6_IFF_DETACHED)
+ if (ia->ia6_flags & IN6_IFF_DETACHED)
continue;
- if (((struct in6_ifaddr *)ifa)->ia6_flags & IN6_IFF_DEPRECATED) {
+ if (ia->ia6_flags & IN6_IFF_DEPRECATED) {
if (V_ip6_use_deprecated)
dep[1] = (struct in6_ifaddr *)ifa;
continue;
}
- if (ifa != NULL)
- ifa_ref(ifa);
+ if (ia != NULL)
+ ifa_ref(&ia->ia_ifa);
IN6_IFADDR_RUNLOCK();
- return (struct in6_ifaddr *)ifa;
+ return (ia);
}
IN6_IFADDR_RUNLOCK();
--3MwIy2ne0vdjdPXF--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111229225539.GD12721>