From owner-freebsd-net@FreeBSD.ORG Thu Dec 29 22:55:41 2011 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 0249B1065672; Thu, 29 Dec 2011 22:55:41 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117]) by mx1.freebsd.org (Postfix) with ESMTP id 870748FC15; Thu, 29 Dec 2011 22:55:40 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.5/8.14.5) with ESMTP id pBTMtdAK020548; Fri, 30 Dec 2011 02:55:39 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.5/8.14.5/Submit) id pBTMtdOM020547; Fri, 30 Dec 2011 02:55:39 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Fri, 30 Dec 2011 02:55:39 +0400 From: Gleb Smirnoff To: John Baldwin Message-ID: <20111229225539.GD12721@FreeBSD.org> References: <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="3MwIy2ne0vdjdPXF" Content-Disposition: inline In-Reply-To: <201112291527.26763.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) 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: Thu, 29 Dec 2011 22:55:41 -0000 --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--